| Message ID | nycvar.YFH.7.76.2111241839590.16505@cbobk.fhfr.pm (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kasan: distinguish kasan report from generic BUG() | expand |
On 24. 11. 21, 18:41, Jiri Kosina wrote: > From: Jiri Kosina <jkosina@suse.cz> > > The typical KASAN report always begins with > > BUG: KASAN: .... > > in kernel log. That 'BUG:' prefix creates a false impression that it's an > actual BUG() codepath being executed, and as such things like > 'panic_on_oops' etc. would work on it as expected; but that's obviously > not the case. > > Switch the order of prefixes to make this distinction clear and avoid > confusion. Thinking about it more in the scope of panic_on_oops above: wouldn't it make more sense to emit "KASAN: WARNING:" instead? All that provided the fact the code explicitly does "if (panic_on_warn) { panic(); }"? > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > --- > mm/kasan/report.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index 0bc10f452f7e..ead714c844e9 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -86,7 +86,7 @@ __setup("kasan_multi_shot", kasan_set_multi_shot); > > static void print_error_description(struct kasan_access_info *info) > { > - pr_err("BUG: KASAN: %s in %pS\n", > + pr_err("KASAN: BUG: %s in %pS\n", > kasan_get_bug_type(info), (void *)info->ip); > if (info->access_size) > pr_err("%s of size %zu at addr %px by task %s/%d\n", > @@ -366,7 +366,7 @@ void kasan_report_invalid_free(void *object, unsigned long ip) > #endif /* IS_ENABLED(CONFIG_KUNIT) */ > > start_report(&flags); > - pr_err("BUG: KASAN: double-free or invalid-free in %pS\n", (void *)ip); > + pr_err("KASAN: BUG: double-free or invalid-free in %pS\n", (void *)ip); > kasan_print_tags(tag, object); > pr_err("\n"); > print_address_description(object, tag); > @@ -386,7 +386,7 @@ void kasan_report_async(void) > #endif /* IS_ENABLED(CONFIG_KUNIT) */ > > start_report(&flags); > - pr_err("BUG: KASAN: invalid-access\n"); > + pr_err("KASAN: BUG: invalid-access\n"); > pr_err("Asynchronous mode enabled: no access details available\n"); > pr_err("\n"); > dump_stack_lvl(KERN_ERR); > >
On Wed, 24 Nov 2021 at 18:41, Jiri Kosina <jikos@kernel.org> wrote: > > From: Jiri Kosina <jkosina@suse.cz> > > The typical KASAN report always begins with > > BUG: KASAN: .... > > in kernel log. That 'BUG:' prefix creates a false impression that it's an > actual BUG() codepath being executed, and as such things like > 'panic_on_oops' etc. would work on it as expected; but that's obviously > not the case. > > Switch the order of prefixes to make this distinction clear and avoid > confusion. > > Signed-off-by: Jiri Kosina <jkosina@suse.cz> I'm afraid writing "KASAN: BUG: " doesn't really tell me this is a non-BUG() vs. "BUG: KASAN". Using this ordering ambiguity to try and resolve human confusion just adds more confusion. The bigger problem is a whole bunch of testing tools rely on the existing order, which has been like this for years -- changing it now just adds unnecessary churn. For example syzkaller, which looks for "BUG: <tool>: report". Changing the order would have to teach all kinds of testing tools to look for different strings. The same format is also used by other dynamic analysis tools, such as KCSAN, and KFENCE, for the simple reason that it's an established format and testing tools don't need to be taught new tricks. Granted, there is a subtle inconsistency wrt. panic_on_oops, in that the debugging tools do use panic_on_warn instead, since their reporting behaviour is more like a WARN. But I'd also not want to prefix them with "WARNING" either, since all reports are serious bugs and shouldn't be ignored. KASAN has more fine-grained control on when to panic, see Documentation/dev-tools/kasan.rst. If the problem is potentially confusing people, I think the better solution is to simply document all kernel error reports and their panic-behaviour (and flags affecting panic-behaviour) in a central place in Documentation/. Thanks, -- Marco
On Wed, 24 Nov 2021 at 19:06, Marco Elver <elver@google.com> wrote: > > On Wed, 24 Nov 2021 at 18:41, Jiri Kosina <jikos@kernel.org> wrote: > > > > From: Jiri Kosina <jkosina@suse.cz> > > > > The typical KASAN report always begins with > > > > BUG: KASAN: .... > > > > in kernel log. That 'BUG:' prefix creates a false impression that it's an > > actual BUG() codepath being executed, and as such things like > > 'panic_on_oops' etc. would work on it as expected; but that's obviously > > not the case. > > > > Switch the order of prefixes to make this distinction clear and avoid > > confusion. > > > > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > > I'm afraid writing "KASAN: BUG: " doesn't really tell me this is a > non-BUG() vs. "BUG: KASAN". Using this ordering ambiguity to try and > resolve human confusion just adds more confusion. > > The bigger problem is a whole bunch of testing tools rely on the > existing order, which has been like this for years -- changing it now > just adds unnecessary churn. For example syzkaller, which looks for > "BUG: <tool>: report". > > Changing the order would have to teach all kinds of testing tools to > look for different strings. The same format is also used by other > dynamic analysis tools, such as KCSAN, and KFENCE, for the simple > reason that it's an established format and testing tools don't need to > be taught new tricks. Yes, lots of kernel testing systems may be looking just for "BUG:" and start missing KASAN bugs. Or they may be doing more special things when they see the current "BUG: KASAN:". > Granted, there is a subtle inconsistency wrt. panic_on_oops, in that > the debugging tools do use panic_on_warn instead, since their > reporting behaviour is more like a WARN. But I'd also not want to > prefix them with "WARNING" either, since all reports are serious bugs > and shouldn't be ignored. KASAN has more fine-grained control on when > to panic, see Documentation/dev-tools/kasan.rst. > > If the problem is potentially confusing people, I think the better > solution is to simply document all kernel error reports and their > panic-behaviour (and flags affecting panic-behaviour) in a central > place in Documentation/. > > Thanks, > -- Marco
diff --git a/mm/kasan/report.c b/mm/kasan/report.c index 0bc10f452f7e..ead714c844e9 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -86,7 +86,7 @@ __setup("kasan_multi_shot", kasan_set_multi_shot); static void print_error_description(struct kasan_access_info *info) { - pr_err("BUG: KASAN: %s in %pS\n", + pr_err("KASAN: BUG: %s in %pS\n", kasan_get_bug_type(info), (void *)info->ip); if (info->access_size) pr_err("%s of size %zu at addr %px by task %s/%d\n", @@ -366,7 +366,7 @@ void kasan_report_invalid_free(void *object, unsigned long ip) #endif /* IS_ENABLED(CONFIG_KUNIT) */ start_report(&flags); - pr_err("BUG: KASAN: double-free or invalid-free in %pS\n", (void *)ip); + pr_err("KASAN: BUG: double-free or invalid-free in %pS\n", (void *)ip); kasan_print_tags(tag, object); pr_err("\n"); print_address_description(object, tag); @@ -386,7 +386,7 @@ void kasan_report_async(void) #endif /* IS_ENABLED(CONFIG_KUNIT) */ start_report(&flags); - pr_err("BUG: KASAN: invalid-access\n"); + pr_err("KASAN: BUG: invalid-access\n"); pr_err("Asynchronous mode enabled: no access details available\n"); pr_err("\n"); dump_stack_lvl(KERN_ERR);