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

[Jian Hui Lee]

On Mon, Oct 28, 2024 at 8:40 PM Koichiro Den <koichiro.den at canonical.com> wrote:
>
> 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`.
>

yes, looks better.

from the perspective of the patch file, it's not straightforward to
review, compared to the version intended for backporting [1]. it would
be appreciated if the patch is well-structured in the first place.
thanks.

1. https://lore.kernel.org/all/20191108213449.887070634@goodmis.org/

> >
> > > +       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