Subject: Re: [PATCH v2] ubsan: don't handle misaligned address when support
 unaligned access
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Ding Tianhong <dingtianhong@huawei.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        LinuxArm <linuxarm@huawei.com>, David Laight <David.Laight@ACULAB.COM>
References: <5b905d56-609e-3822-096a-3b93b3eb7675@huawei.com>
 <20171206164916.687d5006b7a49ef1149e7154@linux-foundation.org>
 <022fdf6b-6e00-fe70-3124-80b8ff50a859@virtuozzo.com>
 <20171207152453.7aaf8f420ebf69d13c29a761@linux-foundation.org>
From: Andrey Ryabinin <aryabinin@virtuozzo.com>
Message-ID: <8c7cc89e-f976-077b-7b64-1046a1f69569@virtuozzo.com>
Date: Fri, 8 Dec 2017 13:48:53 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <20171207152453.7aaf8f420ebf69d13c29a761@linux-foundation.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2017 10:45:19.3969 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: d5888285-61d2-4ac8-4893-08d53e28c6e2
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR08MB2823
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2947
Lines: 66


On 12/08/2017 02:24 AM, Andrew Morton wrote:
> On Thu, 7 Dec 2017 16:31:23 +0300 Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> 
>> On 12/07/2017 03:49 AM, Andrew Morton wrote:
>>> (correcting Andrey's email address)
>>>
>>>
>>> From: Ding Tianhong <dingtianhong@huawei.com>
>>> 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();