Re: [PATCH v4 3/5] kasan: Add report for async mode

[Vincenzo Frascino]

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.
> 

-- 
Regards,
Vincenzo