KASAN provides an asynchronous mode of execution.
Add reporting functionality for this mode.
Cc: Dmitry Vyukov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Alexander Potapenko <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Reviewed-by: Andrey Konovalov <[email protected]>
Signed-off-by: Vincenzo Frascino <[email protected]>
---
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);
+}
+#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 Tue, Jan 26, 2021 at 2:46 PM Vincenzo Frascino
<[email protected]> wrote:
>
> KASAN provides an asynchronous mode of execution.
>
> Add reporting functionality for this mode.
>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Cc: Andrey Konovalov <[email protected]>
> Reviewed-by: Andrey Konovalov <[email protected]>
> Signed-off-by: Vincenzo Frascino <[email protected]>
> ---
> 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
> <[email protected]> wrote:
>>
>> KASAN provides an asynchronous mode of execution.
>>
>> Add reporting functionality for this mode.
>>
>> Cc: Dmitry Vyukov <[email protected]>
>> Cc: Andrey Ryabinin <[email protected]>
>> Cc: Alexander Potapenko <[email protected]>
>> Cc: Andrey Konovalov <[email protected]>
>> Reviewed-by: Andrey Konovalov <[email protected]>
>> Signed-off-by: Vincenzo Frascino <[email protected]>
>> ---
>> 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
>>
--
Regards,
Vincenzo
On Fri, Jan 29, 2021 at 6:44 PM Vincenzo Frascino
<[email protected]> wrote:
>
>
>
> On 1/29/21 5:40 PM, Andrey Konovalov wrote:
> > On Tue, Jan 26, 2021 at 2:46 PM Vincenzo Frascino
> > <[email protected]> wrote:
> >>
> >> KASAN provides an asynchronous mode of execution.
> >>
> >> Add reporting functionality for this mode.
> >>
> >> Cc: Dmitry Vyukov <[email protected]>
> >> Cc: Andrey Ryabinin <[email protected]>
> >> Cc: Alexander Potapenko <[email protected]>
> >> Cc: Andrey Konovalov <[email protected]>
> >> Reviewed-by: Andrey Konovalov <[email protected]>
> >> Signed-off-by: Vincenzo Frascino <[email protected]>
> >> ---
> >> 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?
--
Regards,
Vincenzo
On 1/29/21 5:56 PM, Andrey Konovalov wrote:
> On Fri, Jan 29, 2021 at 6:44 PM Vincenzo Frascino
> <[email protected]> wrote:
>>
>>
>>
>> On 1/29/21 5:40 PM, Andrey Konovalov wrote:
>>> On Tue, Jan 26, 2021 at 2:46 PM Vincenzo Frascino
>>> <[email protected]> wrote:
>>>>
>>>> KASAN provides an asynchronous mode of execution.
>>>>
>>>> Add reporting functionality for this mode.
>>>>
>>>> Cc: Dmitry Vyukov <[email protected]>
>>>> Cc: Andrey Ryabinin <[email protected]>
>>>> Cc: Alexander Potapenko <[email protected]>
>>>> Cc: Andrey Konovalov <[email protected]>
>>>> Reviewed-by: Andrey Konovalov <[email protected]>
>>>> Signed-off-by: Vincenzo Frascino <[email protected]>
>>>> ---
>>>> 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
--
Regards,
Vincenzo
On Fri, Jan 29, 2021 at 6:56 PM Vincenzo Frascino
<[email protected]> 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
<[email protected]> 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
> <[email protected]> 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.
--
Regards,
Vincenzo
On 1/29/21 6:10 PM, Andrey Konovalov wrote:
> On Fri, Jan 29, 2021 at 6:57 PM Vincenzo Frascino
> <[email protected]> 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 ;)
--
Regards,
Vincenzo
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.
--
Regards,
Vincenzo
On Fri, Jan 29, 2021 at 7:42 PM Vincenzo Frascino
<[email protected]> 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!