Message-ID: <CANpmjNMG_YZa4ZB2xPYbf=fq9=tgn+8TOwURDiHPZtgXCe=iPg@mail.gmail.com>
Date: Fri, 18 Oct 2024 09:52:09 +0200
From: Marco Elver <elver@...gle.com>
To: Andrey Konovalov <andreyknvl@...il.com>
Cc: Nihar Chaithanya <niharchaithanya@...il.com>, dvyukov@...gle.com, 
	Aleksandr Nogikh <nogikh@...gle.com>, ryabinin.a.a@...il.com, skhan@...uxfoundation.org, 
	kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org, 
	Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH] kasan:report: filter out kasan related stack entries

On Fri, 18 Oct 2024 at 02:44, Andrey Konovalov <andreyknvl@...il.com> wrote:
>
> On Thu, Oct 17, 2024 at 11:46 PM Nihar Chaithanya
> <niharchaithanya@...il.com> wrote:
> >
> > The reports of KASAN include KASAN related stack frames which are not
> > the point of interest in the stack-trace. KCSAN report filters out such
> > internal frames providing relevant stack trace. Currently, KASAN reports
> > are generated by dump_stack_lvl() which prints the entire stack.
> >
> > Add functionality to KASAN reports to save the stack entries and filter
> > out the kasan related stack frames in place of dump_stack_lvl().
> >
> > Within this new functionality:
> >         - A function save_stack_lvl_kasan() in place of dump_stack_lvl() is
> >           created which contains functionality for saving, filtering and printing
> >           the stack-trace.

save_stack_lvl_kasan() tells me that it's saving a stack trace
somewhere. But this is actually printing. So the name here is
misleading.

We usually name things as <subsys>_foo if it's a function similar to
foo but for that subsystem.

So you can name it kasan_dump_stack_lvl.

> >         - The stack-trace is saved to an array using stack_trace_save() similar to
> >           KCSAN reporting which is useful for filtering the stack-trace,
> >         - The sanitize_stack_entries() function is included to get the number of
> >           entries to be skipped for filtering similar to KCSAN reporting,
> >         - The dump_stack_print_info() which prints generic debug info is included
> >           from __dump_stack(),
> >         - And the function print_stack_trace() to print the stack-trace using the
> >           array containing stack entries as well as the number of entries to be
> >           skipped or filtered out is included.
> >
> > Signed-off-by: Nihar Chaithanya <niharchaithanya@...il.com>
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=215756
>
> Great start!
>
> One part that is missing is also filtering out KASAN frames in stack
> traces printed from print_track(). Right now it call
> stack_depot_print() to print the stack trace. I think the way to
> approach this would be to use stack_depot_fetch(), memcpy the frames
> to a local buffer, and then reuse the stack trace printing code you
> added.
>
> I've also left some comments below.
>
> Please address these points first and send v2. Then, I'll test the
> patch and see if there's more things to be done.
>
> On a related note, I wonder if losing the additional annotations about
> which part of the stack trace belongs with context (task, irq, etc)
> printed by dump_stack() would be a problem. But worst case, we can
> hide stack frame filtering under a CONFIG option.
>
> > ---
> >  mm/kasan/report.c | 92 +++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 90 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > index b48c768acc84..c180cd8b32ae 100644
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@ -39,6 +39,7 @@ static unsigned long kasan_flags;
> >
> >  #define KASAN_BIT_REPORTED     0
> >  #define KASAN_BIT_MULTI_SHOT   1
> > +#define NUM_STACK_ENTRIES 64
>
> If we keep this as 64, we can reuse KASAN_STACK_DEPTH.
>
> However, I wonder if 64 frames is enough. Marco, Alexander, Dmitry,
> IIRC you did some measurements on the length of stack traces in the
> kernel: would 64 frames be good enough for KASAN reports? Was this
> ever a problem for KCSAN?

It was never a problem and 64 was enough, even when unwinding through
interrupt handlers.

It should just use KASAN_STACK_DEPTH.

> >
> >  enum kasan_arg_fault {
> >         KASAN_ARG_FAULT_DEFAULT,
> > @@ -369,12 +370,99 @@ static inline bool init_task_stack_addr(const void *addr)
> >                         sizeof(init_thread_union.stack));
> >  }
> >
> > +/* Helper to skip KASAN-related functions in stack-trace. */
> > +static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries)
> > +{
> > +       char buf[64];
> > +       int len, skip;
> > +
> > +       for (skip = 0; skip < num_entries; ++skip) {
> > +               len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skip]);
> > +
> > +               /* Never show  kasan_* functions. */
> > +               if (strnstr(buf, "kasan_", len) == buf)
> > +                       continue;
> > +               /*
> > +                * No match for runtime functions -- @skip entries to skip to
> > +                * get to first frame of interest.
> > +                */
> > +               break;
> > +       }
> > +
> > +       return skip;
> > +}
> > +
>
> Please also copy the comment for this function, it's useful for
> understanding what's going on.
>
> > +static int
> > +replace_stack_entry(unsigned long stack_entries[], int num_entries, unsigned long ip,
> > +                   unsigned long *replaced)
> > +{
> > +       unsigned long symbolsize, offset;
> > +       unsigned long target_func;
> > +       int skip;
> > +
> > +       if (kallsyms_lookup_size_offset(ip, &symbolsize, &offset))
> > +               target_func = ip - offset;
> > +       else
> > +               goto fallback;
> > +
> > +       for (skip = 0; skip < num_entries; ++skip) {
> > +               unsigned long func = stack_entries[skip];
> > +
> > +               if (!kallsyms_lookup_size_offset(func, &symbolsize, &offset))
> > +                       goto fallback;
> > +               func -= offset;
> > +
> > +               if (func == target_func) {
> > +                       *replaced = stack_entries[skip];
> > +                       stack_entries[skip] = ip;
> > +                       return skip;
> > +               }
> > +       }
> > +
> > +fallback:
> > +       /* Should not happen; the resulting stack trace is likely misleading. */
> > +       WARN_ONCE(1, "Cannot find frame for %pS in stack trace", (void *)ip);
> > +       return get_stack_skipnr(stack_entries, num_entries);
> > +}
>
> Hm, There's some code duplication here between KCSAN and KASAN.
> Although, the function above is the only part dully duplicated, so I
> don't know whether it makes sense to try to factor it out into a
> common file.
>
> Marco, WDYT?

I would keep it separate, and get it working for KASAN first. There
may need to be fixes that only apply to one or the other, since this
code has been a little fiddly in the past. Once it's settled and
working, we can think about refactoring.

Powered by blists - more mailing lists