[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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