NACK: [SRU][F][PATCH v2 1/3] ftrace: Separate out functionality from ftrace_location_range()

[Koichiro Den]

On Mon, Oct 28, 2024 at 07:45:21PM +0800, Jian Hui Lee wrote:
> Hi Koichiro,
> 
> this backported patch does not look right. i found some unnecessary
> modifications.
> 
> On Fri, Oct 25, 2024 at 9:43 PM Koichiro Den <koichiro.den at canonical.com> wrote:
> >
> > From: "Steven Rostedt (VMware)" <rostedt at goodmis.org>
> >
> > Create a new function called lookup_rec() from the functionality of
> > ftrace_location_range(). The difference between lookup_rec() is that it
> > returns the record that it finds, where as ftrace_location_range() returns
> > only if it found a match or not.
> >
> > The lookup_rec() is static, and can be used for new functionality where
> > ftrace needs to find a record of a specific address.
> >
> > Signed-off-by: Steven Rostedt (VMware) <rostedt at goodmis.org>
> > (backported from commit 7e16f581a81759bafea04d049134b32d1a881226)
> > [koichiroden: Prerequisite for "ftrace: Fix possible use-after-free
> > issue in ftrace_location()". Adjusted context due to an extra commit
> > 170d0fe552d0 in this tree, backported from v5.4.238 ac58b88ccbbb
> > ("ftrace: Fix invalid address access in lookup_rec() when index is 0")]
> > CVE-2024-38588
> > Signed-off-by: Koichiro Den <koichiro.den at canonical.com>
> > ---
> >  kernel/trace/ftrace.c | 40 ++++++++++++++++++++++++----------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 412505d94865..d9992fef7287 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1535,22 +1535,10 @@ static int ftrace_cmp_recs(const void *a, const void *b)
> >         return 0;
> >  }
> >
> > -/**
> > - * ftrace_location_range - return the first address of a traced location
> > - *     if it touches the given ip range
> > - * @start: start of range to search.
> > - * @end: end of range to search (inclusive). @end points to the last byte
> > - *     to check.
> > - *
> > - * Returns rec->ip if the related ftrace location is a least partly within
> > - * the given address range. That is, the first address of the instruction
> > - * that is either a NOP or call to the function tracer. It checks the ftrace
> > - * internal tables to determine if the address belongs or not.
> > - */
> > -unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> 
> 7e16f581a817 (ftrace: Separate out functionality from
> ftrace_location_range()) does not have the above changes.
> 
> > +/**
> > + * ftrace_location_range - return the first address of a traced location
> > + *     if it touches the given ip range
> > + * @start: start of range to search.
> > + * @end: end of range to search (inclusive). @end points to the last byte
> > + *     to check.
> > + *
> > + * Returns rec->ip if the related ftrace location is a least partly within
> > + * the given address range. That is, the first address of the instruction
> > + * that is either a NOP or call to the function tracer. It checks the ftrace
> > + * internal tables to determine if the address belongs or not.
> > + */
> > +unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> > +{
> 
> 7e16f581a817 (ftrace: Separate out functionality from
> ftrace_location_range()) does not have the above changes.
> 
> looks like you adjusted the lines of code unintentionally while backporting.

Thank you for reviewing. If it looks different, please apply this patch
temporarily and see the changes with --diff-algorithm=patience (i.e.,
`git log -1 -p --patience`? You should observe a diff styled quite
similarly to what you see you run `git log -1 -p 7e16f581a817`.

> 
> > +       struct dyn_ftrace *rec;
> > +
> > +       rec = lookup_rec(start, end);
> > +       if (rec)
> > +               return rec->ip;
> >
> >         return 0;
> >  }
> > --
> > 2.43.0
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team