| Message ID | 20210126134603.49759-4-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ARMv8.5-A: MTE: Add async mode support | expand |
On Tue, Jan 26, 2021 at 2:46 PM Vincenzo Frascino <vincenzo.frascino@arm.com> wrote: > > KASAN provides an asynchronous mode of execution. > > Add reporting functionality for this mode. > > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: Alexander Potapenko <glider@google.com> > Cc: Andrey Konovalov <andreyknvl@google.com> > Reviewed-by: Andrey Konovalov <andreyknvl@google.com> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > --- > include/linux/kasan.h | 6 ++++++ > mm/kasan/report.c | 13 +++++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index bb862d1f0e15..b6c502dad54d 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -360,6 +360,12 @@ static inline void *kasan_reset_tag(const void *addr) > > #endif /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS*/ > > +#ifdef CONFIG_KASAN_HW_TAGS > + > +void kasan_report_async(void); > + > +#endif /* CONFIG_KASAN_HW_TAGS */ > + > #ifdef CONFIG_KASAN_SW_TAGS > void __init kasan_init_sw_tags(void); > #else > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index 87b271206163..69bad9c01aed 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -360,6 +360,19 @@ void kasan_report_invalid_free(void *object, unsigned long ip) > end_report(&flags, (unsigned long)object); > } > > +#ifdef CONFIG_KASAN_HW_TAGS > +void kasan_report_async(void) > +{ > + unsigned long flags; > + > + start_report(&flags); > + pr_err("BUG: KASAN: invalid-access\n"); > + pr_err("Asynchronous mode enabled: no access details available\n"); > + dump_stack(); > + end_report(&flags); This conflicts with "kasan: use error_report_end tracepoint" that's in mm. I suggest to call end_report(&flags, 0) here and check addr !=0 in end_report() before calling trace_error_report_end(). > +} > +#endif /* CONFIG_KASAN_HW_TAGS */ > + > static void __kasan_report(unsigned long addr, size_t size, bool is_write, > unsigned long ip) > { > -- > 2.30.0 >
On 1/29/21 5:40 PM, Andrey Konovalov wrote: > On Tue, Jan 26, 2021 at 2:46 PM Vincenzo Frascino > <vincenzo.frascino@arm.com> wrote: >> >> KASAN provides an asynchronous mode of execution. >> >> Add reporting functionality for this mode. >> >> Cc: Dmitry Vyukov <dvyukov@google.com> >> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> >> Cc: Alexander Potapenko <glider@google.com> >> Cc: Andrey Konovalov <andreyknvl@google.com> >> Reviewed-by: Andrey Konovalov <andreyknvl@google.com> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> >> --- >> include/linux/kasan.h | 6 ++++++ >> mm/kasan/report.c | 13 +++++++++++++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/include/linux/kasan.h b/include/linux/kasan.h >> index bb862d1f0e15..b6c502dad54d 100644 >> --- a/include/linux/kasan.h >> +++ b/include/linux/kasan.h >> @@ -360,6 +360,12 @@ static inline void *kasan_reset_tag(const void *addr) >> >> #endif /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS*/ >> >> +#ifdef CONFIG_KASAN_HW_TAGS >> + >> +void kasan_report_async(void); >> + >> +#endif /* CONFIG_KASAN_HW_TAGS */ >> + >> #ifdef CONFIG_KASAN_SW_TAGS >> void __init kasan_init_sw_tags(void); >> #else >> diff --git a/mm/kasan/report.c b/mm/kasan/report.c >> index 87b271206163..69bad9c01aed 100644 >> --- a/mm/kasan/report.c >> +++ b/mm/kasan/report.c >> @@ -360,6 +360,19 @@ void kasan_report_invalid_free(void *object, unsigned long ip) >> end_report(&flags, (unsigned long)object); >> } >> >> +#ifdef CONFIG_KASAN_HW_TAGS >> +void kasan_report_async(void) >> +{ >> + unsigned long flags; >> + >> + start_report(&flags); >> + pr_err("BUG: KASAN: invalid-access\n"); >> + pr_err("Asynchronous mode enabled: no access details available\n"); >> + dump_stack(); >> + end_report(&flags); > > This conflicts with "kasan: use error_report_end tracepoint" that's in mm. > > I suggest to call end_report(&flags, 0) here and check addr !=0 in > end_report() before calling trace_error_report_end(). > I just noticed and about to post a rebased version with end_report(&flags, 0). >> +} >> +#endif /* CONFIG_KASAN_HW_TAGS */ >> + >> static void __kasan_report(unsigned long addr, size_t size, bool is_write, >> unsigned long ip) >> { >> -- >> 2.30.0 >>
On Fri, Jan 29, 2021 at 6:44 PM Vincenzo Frascino <vincenzo.frascino@arm.com> wrote: > > > > On 1/29/21 5:40 PM, Andrey Konovalov wrote: > > On Tue, Jan 26, 2021 at 2:46 PM Vincenzo Frascino > > <vincenzo.frascino@arm.com> wrote: > >> > >> KASAN provides an asynchronous mode of execution. > >> > >> Add reporting functionality for this mode. > >> > >> Cc: Dmitry Vyukov <dvyukov@google.com> > >> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > >> Cc: Alexander Potapenko <glider@google.com> > >> Cc: Andrey Konovalov <andreyknvl@google.com> > >> Reviewed-by: Andrey Konovalov <andreyknvl@google.com> > >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > >> --- > >> include/linux/kasan.h | 6 ++++++ > >> mm/kasan/report.c | 13 +++++++++++++ > >> 2 files changed, 19 insertions(+) > >> > >> diff --git a/include/linux/kasan.h b/include/linux/kasan.h > >> index bb862d1f0e15..b6c502dad54d 100644 > >> --- a/include/linux/kasan.h > >> +++ b/include/linux/kasan.h > >> @@ -360,6 +360,12 @@ static inline void *kasan_reset_tag(const void *addr) > >> > >> #endif /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS*/ > >> > >> +#ifdef CONFIG_KASAN_HW_TAGS > >> + > >> +void kasan_report_async(void); > >> + > >> +#endif /* CONFIG_KASAN_HW_TAGS */ > >> + > >> #ifdef CONFIG_KASAN_SW_TAGS > >> void __init kasan_init_sw_tags(void); > >> #else > >> diff --git a/mm/kasan/report.c b/mm/kasan/report.c > >> index 87b271206163..69bad9c01aed 100644 > >> --- a/mm/kasan/report.c > >> +++ b/mm/kasan/report.c > >> @@ -360,6 +360,19 @@ void kasan_report_invalid_free(void *object, unsigned long ip) > >> end_report(&flags, (unsigned long)object); > >> } > >> > >> +#ifdef CONFIG_KASAN_HW_TAGS > >> +void kasan_report_async(void) > >> +{ > >> + unsigned long flags; > >> + > >> + start_report(&flags); > >> + pr_err("BUG: KASAN: invalid-access\n"); > >> + pr_err("Asynchronous mode enabled: no access details available\n"); Could you also add an empty line here before the stack trace while at it? > >> + dump_stack(); > >> + end_report(&flags); > > > > This conflicts with "kasan: use error_report_end tracepoint" that's in mm. > > > > I suggest to call end_report(&flags, 0) here and check addr !=0 in > > end_report() before calling trace_error_report_end(). > > > > I just noticed and about to post a rebased version with end_report(&flags, 0). > > > >> +} > >> +#endif /* CONFIG_KASAN_HW_TAGS */ > >> + > >> static void __kasan_report(unsigned long addr, size_t size, bool is_write, > >> unsigned long ip) > >> { > >> -- > >> 2.30.0 > >> > > -- > Regards, > Vincenzo
Hi Andrey, On 1/29/21 5:40 PM, Andrey Konovalov wrote: > I suggest to call end_report(&flags, 0) here and check addr !=0 in > end_report() before calling trace_error_report_end(). > Probably this is better as: if (!IS_ENABLED(CONFIG_KASAN_HW_TAGS)) Because that condition passes always addr == 0. What do you think?
On 1/29/21 5:56 PM, Andrey Konovalov wrote: > On Fri, Jan 29, 2021 at 6:44 PM Vincenzo Frascino > <vincenzo.frascino@arm.com> wrote: >> >> >> >> On 1/29/21 5:40 PM, Andrey Konovalov wrote: >>> On Tue, Jan 26, 2021 at 2:46 PM Vincenzo Frascino >>> <vincenzo.frascino@arm.com> wrote: >>>> >>>> KASAN provides an asynchronous mode of execution. >>>> >>>> Add reporting functionality for this mode. >>>> >>>> Cc: Dmitry Vyukov <dvyukov@google.com> >>>> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> >>>> Cc: Alexander Potapenko <glider@google.com> >>>> Cc: Andrey Konovalov <andreyknvl@google.com> >>>> Reviewed-by: Andrey Konovalov <andreyknvl@google.com> >>>> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> >>>> --- >>>> include/linux/kasan.h | 6 ++++++ >>>> mm/kasan/report.c | 13 +++++++++++++ >>>> 2 files changed, 19 insertions(+) >>>> >>>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h >>>> index bb862d1f0e15..b6c502dad54d 100644 >>>> --- a/include/linux/kasan.h >>>> +++ b/include/linux/kasan.h >>>> @@ -360,6 +360,12 @@ static inline void *kasan_reset_tag(const void *addr) >>>> >>>> #endif /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS*/ >>>> >>>> +#ifdef CONFIG_KASAN_HW_TAGS >>>> + >>>> +void kasan_report_async(void); >>>> + >>>> +#endif /* CONFIG_KASAN_HW_TAGS */ >>>> + >>>> #ifdef CONFIG_KASAN_SW_TAGS >>>> void __init kasan_init_sw_tags(void); >>>> #else >>>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c >>>> index 87b271206163..69bad9c01aed 100644 >>>> --- a/mm/kasan/report.c >>>> +++ b/mm/kasan/report.c >>>> @@ -360,6 +360,19 @@ void kasan_report_invalid_free(void *object, unsigned long ip) >>>> end_report(&flags, (unsigned long)object); >>>> } >>>> >>>> +#ifdef CONFIG_KASAN_HW_TAGS >>>> +void kasan_report_async(void) >>>> +{ >>>> + unsigned long flags; >>>> + >>>> + start_report(&flags); >>>> + pr_err("BUG: KASAN: invalid-access\n"); >>>> + pr_err("Asynchronous mode enabled: no access details available\n"); > > Could you also add an empty line here before the stack trace while at it? > Sure no problem. >>>> + dump_stack(); >>>> + end_report(&flags); >>> >>> This conflicts with "kasan: use error_report_end tracepoint" that's in mm. >>> >>> I suggest to call end_report(&flags, 0) here and check addr !=0 in >>> end_report() before calling trace_error_report_end(). >>> >> >> I just noticed and about to post a rebased version with end_report(&flags, 0). >> >> >>>> +} >>>> +#endif /* CONFIG_KASAN_HW_TAGS */ >>>> + >>>> static void __kasan_report(unsigned long addr, size_t size, bool is_write, >>>> unsigned long ip) >>>> { >>>> -- >>>> 2.30.0 >>>> >> >> -- >> Regards, >> Vincenzo
On Fri, Jan 29, 2021 at 6:56 PM Vincenzo Frascino <vincenzo.frascino@arm.com> wrote: > > Hi Andrey, > > On 1/29/21 5:40 PM, Andrey Konovalov wrote: > > I suggest to call end_report(&flags, 0) here and check addr !=0 in > > end_report() before calling trace_error_report_end(). > > > > Probably this is better as: > > if (!IS_ENABLED(CONFIG_KASAN_HW_TAGS)) > > Because that condition passes always addr == 0. Not sure I understand. Call report_end(&flags, 0) and then there do: if (addr) trace_error_report_end(...); Although maybe it makes sense to still trace all async bugs to address 0. Or to some magic address. Alex, WDYT?
On Fri, Jan 29, 2021 at 6:57 PM Vincenzo Frascino <vincenzo.frascino@arm.com> wrote: > >>>> +#ifdef CONFIG_KASAN_HW_TAGS > >>>> +void kasan_report_async(void) > >>>> +{ > >>>> + unsigned long flags; > >>>> + > >>>> + start_report(&flags); > >>>> + pr_err("BUG: KASAN: invalid-access\n"); > >>>> + pr_err("Asynchronous mode enabled: no access details available\n"); > > > > Could you also add an empty line here before the stack trace while at it? > > > > Sure no problem. Just to be clear: I mean adding an empty line into the report itself via pr_err("\n") :)
On 1/29/21 6:09 PM, Andrey Konovalov wrote: > On Fri, Jan 29, 2021 at 6:56 PM Vincenzo Frascino > <vincenzo.frascino@arm.com> wrote: >> >> Hi Andrey, >> >> On 1/29/21 5:40 PM, Andrey Konovalov wrote: >>> I suggest to call end_report(&flags, 0) here and check addr !=0 in >>> end_report() before calling trace_error_report_end(). >>> >> >> Probably this is better as: >> >> if (!IS_ENABLED(CONFIG_KASAN_HW_TAGS)) >> >> Because that condition passes always addr == 0. > > Not sure I understand. Call report_end(&flags, 0) and then there do: > > if (addr) trace_error_report_end(...); > > Although maybe it makes sense to still trace all async bugs to address > 0. Or to some magic address. > > Alex, WDYT? > What I meant is instead of: if (addr) trace_error_report_end(...); you might want to do: if (!IS_ENABLED(CONFIG_KASAN_HW_TAGS)) trace_error_report_end(...); because, could make sense to trace 0 in other cases? I could not find the implementation of trace_error_report_end() hence I am not really sure on what it does.
On 1/29/21 6:10 PM, Andrey Konovalov wrote: > On Fri, Jan 29, 2021 at 6:57 PM Vincenzo Frascino > <vincenzo.frascino@arm.com> wrote: >>>>>> +#ifdef CONFIG_KASAN_HW_TAGS >>>>>> +void kasan_report_async(void) >>>>>> +{ >>>>>> + unsigned long flags; >>>>>> + >>>>>> + start_report(&flags); >>>>>> + pr_err("BUG: KASAN: invalid-access\n"); >>>>>> + pr_err("Asynchronous mode enabled: no access details available\n"); >>> >>> Could you also add an empty line here before the stack trace while at it? >>> >> >> Sure no problem. > > Just to be clear: I mean adding an empty line into the report itself > via pr_err("\n") :) > Yes I got it ;) It is late here but I am not completely asleep yet ;)
On Fri, Jan 29, 2021 at 7:42 PM Vincenzo Frascino <vincenzo.frascino@arm.com> wrote: > > Hi Andrey, > > On 1/29/21 6:16 PM, Vincenzo Frascino wrote: > > What I meant is instead of: > > > > if (addr) trace_error_report_end(...); > > > > you might want to do: > > > > if (!IS_ENABLED(CONFIG_KASAN_HW_TAGS)) trace_error_report_end(...); > > > > because, could make sense to trace 0 in other cases? > > > > I could not find the implementation of trace_error_report_end() hence I am not > > really sure on what it does. > > I figured it out how trace_error_report_end() works. It's intended for collecting crashes for CONFIG_KASAN_HW_TAGS. > And in doing that I > realized that the problem is sync vs async, hence I agree with what you are > proposing: > > if (addr) trace_error_report_end(...); > > I will post v10 shortly. If we want to trace the async mode we can improve it in > -rc1. Sounds good, thanks!
Hi Andrey, On 1/29/21 6:16 PM, Vincenzo Frascino wrote: > What I meant is instead of: > > if (addr) trace_error_report_end(...); > > you might want to do: > > if (!IS_ENABLED(CONFIG_KASAN_HW_TAGS)) trace_error_report_end(...); > > because, could make sense to trace 0 in other cases? > > I could not find the implementation of trace_error_report_end() hence I am not > really sure on what it does. I figured it out how trace_error_report_end() works. And in doing that I realized that the problem is sync vs async, hence I agree with what you are proposing: if (addr) trace_error_report_end(...); I will post v10 shortly. If we want to trace the async mode we can improve it in -rc1.
diff --git a/include/linux/kasan.h b/include/linux/kasan.h index bb862d1f0e15..b6c502dad54d 100644 --- a/include/linux/kasan.h +++ b/include/linux/kasan.h @@ -360,6 +360,12 @@ static inline void *kasan_reset_tag(const void *addr) #endif /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS*/ +#ifdef CONFIG_KASAN_HW_TAGS + +void kasan_report_async(void); + +#endif /* CONFIG_KASAN_HW_TAGS */ + #ifdef CONFIG_KASAN_SW_TAGS void __init kasan_init_sw_tags(void); #else diff --git a/mm/kasan/report.c b/mm/kasan/report.c index 87b271206163..69bad9c01aed 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -360,6 +360,19 @@ void kasan_report_invalid_free(void *object, unsigned long ip) end_report(&flags, (unsigned long)object); } +#ifdef CONFIG_KASAN_HW_TAGS +void kasan_report_async(void) +{ + unsigned long flags; + + start_report(&flags); + pr_err("BUG: KASAN: invalid-access\n"); + pr_err("Asynchronous mode enabled: no access details available\n"); + dump_stack(); + end_report(&flags); +} +#endif /* CONFIG_KASAN_HW_TAGS */ + static void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) {