| Message ID | 20210118183033.41764-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 Mon, Jan 18, 2021 at 06:30:31PM +0000, Vincenzo Frascino 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> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > --- > include/linux/kasan.h | 3 +++ > mm/kasan/report.c | 16 ++++++++++++++-- > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index fe1ae73ff8b5..8f43836ccdac 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -336,6 +336,9 @@ static inline void *kasan_reset_tag(const void *addr) > bool kasan_report(unsigned long addr, size_t size, > bool is_write, unsigned long ip); > > +bool kasan_report_async(unsigned long addr, size_t size, > + bool is_write, unsigned long ip); We have no address, no size and no is_write information. Do we have a reason to pass all these arguments here? Not sure what SPARC ADI does but they may not have all this information either. We can pass ip as the point where we checked the TFSR reg but that's about it. > + > #else /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS */ > > static inline void *kasan_reset_tag(const void *addr) > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index c0fb21797550..946016ead6a9 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write, > start_report(&flags); > > print_error_description(&info); > - if (addr_has_metadata(untagged_addr)) > + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) > print_tags(get_tag(tagged_addr), info.first_bad_addr); > pr_err("\n"); > > - if (addr_has_metadata(untagged_addr)) { > + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) { > print_address_description(untagged_addr, get_tag(tagged_addr)); > pr_err("\n"); > print_memory_metadata(info.first_bad_addr); > @@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write, > return ret; > } > > +bool kasan_report_async(unsigned long addr, size_t size, > + bool is_write, unsigned long ip) > +{ > + pr_info("==================================================================\n"); > + pr_info("KASAN: set in asynchronous mode\n"); > + pr_info("KASAN: some information might not be accurate\n"); > + pr_info("KASAN: fault address is ignored\n"); > + pr_info("KASAN: write/read distinction is ignored\n"); > + > + return kasan_report(addr, size, is_write, ip); So just call kasan_report (0, 0, 0, ip) here.
On 1/19/21 1:04 PM, Catalin Marinas wrote: > On Mon, Jan 18, 2021 at 06:30:31PM +0000, Vincenzo Frascino 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> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> >> --- >> include/linux/kasan.h | 3 +++ >> mm/kasan/report.c | 16 ++++++++++++++-- >> 2 files changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/kasan.h b/include/linux/kasan.h >> index fe1ae73ff8b5..8f43836ccdac 100644 >> --- a/include/linux/kasan.h >> +++ b/include/linux/kasan.h >> @@ -336,6 +336,9 @@ static inline void *kasan_reset_tag(const void *addr) >> bool kasan_report(unsigned long addr, size_t size, >> bool is_write, unsigned long ip); >> >> +bool kasan_report_async(unsigned long addr, size_t size, >> + bool is_write, unsigned long ip); > > We have no address, no size and no is_write information. Do we have a > reason to pass all these arguments here? Not sure what SPARC ADI does > but they may not have all this information either. We can pass ip as the > point where we checked the TFSR reg but that's about it. > I kept the interface generic for future development and mainly to start a discussion. I do not have a strong opinion either way. If Andrey agrees as well I am happy to change it to what you are suggesting in v5. >> + >> #else /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS */ >> >> static inline void *kasan_reset_tag(const void *addr) >> diff --git a/mm/kasan/report.c b/mm/kasan/report.c >> index c0fb21797550..946016ead6a9 100644 >> --- a/mm/kasan/report.c >> +++ b/mm/kasan/report.c >> @@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write, >> start_report(&flags); >> >> print_error_description(&info); >> - if (addr_has_metadata(untagged_addr)) >> + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) >> print_tags(get_tag(tagged_addr), info.first_bad_addr); >> pr_err("\n"); >> >> - if (addr_has_metadata(untagged_addr)) { >> + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) { >> print_address_description(untagged_addr, get_tag(tagged_addr)); >> pr_err("\n"); >> print_memory_metadata(info.first_bad_addr); >> @@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write, >> return ret; >> } >> >> +bool kasan_report_async(unsigned long addr, size_t size, >> + bool is_write, unsigned long ip) >> +{ >> + pr_info("==================================================================\n"); >> + pr_info("KASAN: set in asynchronous mode\n"); >> + pr_info("KASAN: some information might not be accurate\n"); >> + pr_info("KASAN: fault address is ignored\n"); >> + pr_info("KASAN: write/read distinction is ignored\n"); >> + >> + return kasan_report(addr, size, is_write, ip); > > So just call kasan_report (0, 0, 0, ip) here. > Fine by me.
On Tue, Jan 19, 2021 at 02:23:03PM +0000, Vincenzo Frascino wrote: > On 1/19/21 1:04 PM, Catalin Marinas wrote: > > On Mon, Jan 18, 2021 at 06:30:31PM +0000, Vincenzo Frascino wrote: > >> +bool kasan_report_async(unsigned long addr, size_t size, > >> + bool is_write, unsigned long ip); > > > > We have no address, no size and no is_write information. Do we have a > > reason to pass all these arguments here? Not sure what SPARC ADI does > > but they may not have all this information either. We can pass ip as the > > point where we checked the TFSR reg but that's about it. > > I kept the interface generic for future development and mainly to start a > discussion. I do not have a strong opinion either way. If Andrey agrees as well > I am happy to change it to what you are suggesting in v5. For now, I think it's preferable that this only has parameters that we can actually provide. That way it's clearer what's going on in both callers and callees, and we can always rework the prototype later or add separate variants of the function that can take additional parameters. I don't think we even need to use __kasan_report() -- more on that below. [...] > >> @@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write, > >> start_report(&flags); > >> > >> print_error_description(&info); > >> - if (addr_has_metadata(untagged_addr)) > >> + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) > >> print_tags(get_tag(tagged_addr), info.first_bad_addr); > >> pr_err("\n"); > >> > >> - if (addr_has_metadata(untagged_addr)) { > >> + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) { > >> print_address_description(untagged_addr, get_tag(tagged_addr)); > >> pr_err("\n"); > >> print_memory_metadata(info.first_bad_addr); > >> @@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write, > >> return ret; > >> } > >> > >> +bool kasan_report_async(unsigned long addr, size_t size, > >> + bool is_write, unsigned long ip) > >> +{ > >> + pr_info("==================================================================\n"); > >> + pr_info("KASAN: set in asynchronous mode\n"); > >> + pr_info("KASAN: some information might not be accurate\n"); > >> + pr_info("KASAN: fault address is ignored\n"); > >> + pr_info("KASAN: write/read distinction is ignored\n"); > >> + > >> + return kasan_report(addr, size, is_write, ip); > > > > So just call kasan_report (0, 0, 0, ip) here. Given there's no information available, I think it's simpler and preferable to handle the logging separately, as is done for kasan_report_invalid_free(). For example, we could do something roughly like: void kasan_report_async(void) { unsigned long flags; start_report(&flags); pr_err("BUG: KASAN: Tag mismatch detected asynchronously\n"); pr_err("KASAN: no fault information available\n"); dump_stack(); end_report(&flags); } ... which is easier to consume, since there's no misleading output, avoids complicating the synchronous reporting path, and we could consider adding information that's only of use for debugging asynchronous faults here. Since the callside is logged in the backtrace, we don't even need the synthetic IP parameter. Thanks, Mark.
On 1/19/21 2:46 PM, Mark Rutland wrote: > On Tue, Jan 19, 2021 at 02:23:03PM +0000, Vincenzo Frascino wrote: >> On 1/19/21 1:04 PM, Catalin Marinas wrote: >>> On Mon, Jan 18, 2021 at 06:30:31PM +0000, Vincenzo Frascino wrote: > >>>> +bool kasan_report_async(unsigned long addr, size_t size, >>>> + bool is_write, unsigned long ip); >>> >>> We have no address, no size and no is_write information. Do we have a >>> reason to pass all these arguments here? Not sure what SPARC ADI does >>> but they may not have all this information either. We can pass ip as the >>> point where we checked the TFSR reg but that's about it. >> >> I kept the interface generic for future development and mainly to start a >> discussion. I do not have a strong opinion either way. If Andrey agrees as well >> I am happy to change it to what you are suggesting in v5. > > For now, I think it's preferable that this only has parameters that we > can actually provide. That way it's clearer what's going on in both > callers and callees, and we can always rework the prototype later or add > separate variants of the function that can take additional parameters. > > I don't think we even need to use __kasan_report() -- more on that > below. > > [...] > >>>> @@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write, >>>> start_report(&flags); >>>> >>>> print_error_description(&info); >>>> - if (addr_has_metadata(untagged_addr)) >>>> + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) >>>> print_tags(get_tag(tagged_addr), info.first_bad_addr); >>>> pr_err("\n"); >>>> >>>> - if (addr_has_metadata(untagged_addr)) { >>>> + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) { >>>> print_address_description(untagged_addr, get_tag(tagged_addr)); >>>> pr_err("\n"); >>>> print_memory_metadata(info.first_bad_addr); >>>> @@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write, >>>> return ret; >>>> } >>>> >>>> +bool kasan_report_async(unsigned long addr, size_t size, >>>> + bool is_write, unsigned long ip) >>>> +{ >>>> + pr_info("==================================================================\n"); >>>> + pr_info("KASAN: set in asynchronous mode\n"); >>>> + pr_info("KASAN: some information might not be accurate\n"); >>>> + pr_info("KASAN: fault address is ignored\n"); >>>> + pr_info("KASAN: write/read distinction is ignored\n"); >>>> + >>>> + return kasan_report(addr, size, is_write, ip); >>> >>> So just call kasan_report (0, 0, 0, ip) here. > > Given there's no information available, I think it's simpler and > preferable to handle the logging separately, as is done for > kasan_report_invalid_free(). For example, we could do something roughly > like: > > void kasan_report_async(void) > { > unsigned long flags; > > start_report(&flags); > pr_err("BUG: KASAN: Tag mismatch detected asynchronously\n"); > pr_err("KASAN: no fault information available\n"); > dump_stack(); > end_report(&flags); > } > > ... which is easier to consume, since there's no misleading output, > avoids complicating the synchronous reporting path, and we could > consider adding information that's only of use for debugging > asynchronous faults here. > > Since the callside is logged in the backtrace, we don't even need the > synthetic IP parameter. > Agree, especially because I tend to not like to rely on compiler builtins and what you proposed solves the problem ;) I will refactor my code once Andrey had a chance to take a look as well. > Thanks, > Mark. >
On Tue, Jan 19, 2021 at 3:46 PM Mark Rutland <mark.rutland@arm.com> wrote: > > Given there's no information available, I think it's simpler and > preferable to handle the logging separately, as is done for > kasan_report_invalid_free(). For example, we could do something roughly > like: > > void kasan_report_async(void) > { > unsigned long flags; > > start_report(&flags); > pr_err("BUG: KASAN: Tag mismatch detected asynchronously\n"); "BUG: KASAN: invalid-access" It also might make sense to pass the ip, even though it's not exactly related to the access: pr_err("BUG: KASAN: invalid-access in %pS\n", (void *)ip); Up to you. > pr_err("KASAN: no fault information available\n"); pr_err("Asynchronous mode enabled: no access details available\n"); > dump_stack(); > end_report(&flags); > } This approach with a dedicated function is better. Thanks, Mark! Please put it next to kasan_report_invalid_free().
On 1/19/21 6:12 PM, Andrey Konovalov wrote: > On Tue, Jan 19, 2021 at 3:46 PM Mark Rutland <mark.rutland@arm.com> wrote: >> >> Given there's no information available, I think it's simpler and >> preferable to handle the logging separately, as is done for >> kasan_report_invalid_free(). For example, we could do something roughly >> like: >> >> void kasan_report_async(void) >> { >> unsigned long flags; >> >> start_report(&flags); >> pr_err("BUG: KASAN: Tag mismatch detected asynchronously\n"); > > "BUG: KASAN: invalid-access" > Ok, I will do in v5. It looks more uniform with what we have for the sync exception. > It also might make sense to pass the ip, even though it's not exactly > related to the access: > I would like to avoid to add a builtin for something that has not a real meaning as you are correctly pointing out. > pr_err("BUG: KASAN: invalid-access in %pS\n", (void *)ip); > > Up to you. > >> pr_err("KASAN: no fault information available\n"); > > pr_err("Asynchronous mode enabled: no access details available\n"); > >> dump_stack(); >> end_report(&flags); >> } > > This approach with a dedicated function is better. Thanks, Mark! > > Please put it next to kasan_report_invalid_free(). > Will do in v5. Thanks!
diff --git a/include/linux/kasan.h b/include/linux/kasan.h index fe1ae73ff8b5..8f43836ccdac 100644 --- a/include/linux/kasan.h +++ b/include/linux/kasan.h @@ -336,6 +336,9 @@ static inline void *kasan_reset_tag(const void *addr) bool kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip); +bool kasan_report_async(unsigned long addr, size_t size, + bool is_write, unsigned long ip); + #else /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS */ static inline void *kasan_reset_tag(const void *addr) diff --git a/mm/kasan/report.c b/mm/kasan/report.c index c0fb21797550..946016ead6a9 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -388,11 +388,11 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write, start_report(&flags); print_error_description(&info); - if (addr_has_metadata(untagged_addr)) + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) print_tags(get_tag(tagged_addr), info.first_bad_addr); pr_err("\n"); - if (addr_has_metadata(untagged_addr)) { + if (addr_has_metadata(untagged_addr) && (untagged_addr != 0)) { print_address_description(untagged_addr, get_tag(tagged_addr)); pr_err("\n"); print_memory_metadata(info.first_bad_addr); @@ -419,6 +419,18 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write, return ret; } +bool kasan_report_async(unsigned long addr, size_t size, + bool is_write, unsigned long ip) +{ + pr_info("==================================================================\n"); + pr_info("KASAN: set in asynchronous mode\n"); + pr_info("KASAN: some information might not be accurate\n"); + pr_info("KASAN: fault address is ignored\n"); + pr_info("KASAN: write/read distinction is ignored\n"); + + return kasan_report(addr, size, is_write, ip); +} + #ifdef CONFIG_KASAN_INLINE /* * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high
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> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> --- include/linux/kasan.h | 3 +++ mm/kasan/report.c | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-)