Re: [PATCH v2] ubsan: don't handle misaligned address when support unaligned access

[Andrey Ryabinin]

On 12/08/2017 02:24 AM, Andrew Morton wrote:
> On Thu, 7 Dec 2017 16:31:23 +0300 Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> wrote:
> 
>> On 12/07/2017 03:49 AM, Andrew Morton wrote:
>>> (correcting Andrey's email address)
>>>
>>>
>>> From: Ding Tianhong <dingtianhong@xxxxxxxxxx>
>>> Subject: lib/ubsan.c: don't handle misaligned address when kernel supports unaligned access
>>>
>>> ubsan reports a warning like:
>>>
>>> UBSAN: Undefined behaviour in ../include/linux/etherdevice.h:386:9
>>> load of misaligned address ffffffc069ba0482 for type 'long unsigned int'
>>> which requires 8 byte alignment
>>> CPU: 0 PID: 901 Comm: sshd Not tainted 4.xx+ #1
>>> Hardware name: linux,dummy-virt (DT)
>>> Call trace:
>>> [<ffffffc000093600>] dump_backtrace+0x0/0x348
>>> [<ffffffc000093968>] show_stack+0x20/0x30
>>> [<ffffffc001651664>] dump_stack+0x144/0x1b4
>>> [<ffffffc0016519b0>] ubsan_epilogue+0x18/0x74
>>> [<ffffffc001651bac>] __ubsan_handle_type_mismatch+0x1a0/0x25c
>>> [<ffffffc00125d8a0>] dev_gro_receive+0x17d8/0x1830
>>> [<ffffffc00125d928>] napi_gro_receive+0x30/0x158
>>> [<ffffffc000f4f93c>] virtnet_receive+0xad4/0x1fa8
>>>
>>> The reason is that when enabling the CONFIG_UBSAN_ALIGNMENT, ubsan will
>>> report the unaligned access even if the system supports it
>>> (CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y).  This produces a lot of noise
>>> in the log and causes confusion.
>>>
>>
>> NACK. This doesn't make sense. If you don't want to see misaligned access reports
>> you simply shouldn't enable CONFIG_UBSAN_ALIGNMENT.
> 
> So should UBSAN Kconfig disable CONFIG_UBSAN_ALIGNMENT when
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y?
> 

CONFIG_UBSAN_ALIGNMENT is already disabled by default for HAVE_EFFICIENT_UNALIGNED_ACCESS=y because it's noisy,
but we still allow users to enable it if they want to.

I don't think we should completely forbid enabling it for HAVE_EFFICIENT_UNALIGNED_ACCESS=y.
Unaligned access is still a bug in non-arch code and outside of sections like #ifdef HAVE_EFFICIENT_UNALIGNED_ACCESS .. #endif .

As for UBSAN noise inside #ifdef HAVE_EFFICIENT_UNALIGNED_ACCESS .. #endif sections, it should be possible to suppress it
with something like this:

	typedef __attribute__((aligned(1))) int unaligned_int;                                                                                                                                                              
	....

	int x = *(unaligned_int*)unalinged_addr;

This shouldn't affect generated code (on arches that support unaligned loads) and suppresses UBSAN warnings.
It's might be a right thing todo. Even if arch supports unaligned access, it's still undefined behaviour according to the C standard.
And one day, GCC might start doing optimizations based on this, e.g.:

	u64 *ptr;
	...
	x = *ptr;
	...
	if (ptr & 7)  // Compiler can assume that this statement is always false, because 'ptr' was deferenced, so it must be aligned
		do_something();