diff mbox series

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

Message ID 5b905d56-609e-3822-096a-3b93b3eb7675@huawei.com
State New
Headers show
Series [v2] ubsan: don't handle misaligned address when support unaligned access | expand

Commit Message

Ding Tianhong Dec. 2, 2017, 5:46 a.m. UTC 
The ubsan always report Warning just 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 enable the CONFIG_UBSAN_ALIGNMENT, the ubsan
will report the unaligned access even if the system support it
(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y), it will produce a lot
of noise in the log and cause confusion.

This patch will close the detection of unaligned access when
the system support unaligned access.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

---
 lib/ubsan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
1.8.3.1

Comments

Andrew Morton Dec. 7, 2017, 12:49 a.m. UTC | #1 
(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.

Prevent the detection of unaligned access when the system support
unaligned access.

Link: http://lkml.kernel.org/r/5b905d56-609e-3822-096a-3b93b3eb7675@huawei.com
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

---

 lib/ubsan.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -puN lib/ubsan.c~ubsan-dont-handle-misaligned-address-when-support-unaligned-access lib/ubsan.c
--- a/lib/ubsan.c~ubsan-dont-handle-misaligned-address-when-support-unaligned-access
+++ a/lib/ubsan.c
@@ -322,7 +322,8 @@ void __ubsan_handle_type_mismatch(struct
 	if (!ptr)
 		handle_null_ptr_deref(data);
 	else if (data->alignment && !IS_ALIGNED(ptr, data->alignment))
-		handle_missaligned_access(data, ptr);
+		if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
+			handle_missaligned_access(data, ptr);
 	else
 		handle_object_size_mismatch(data, ptr);
 }
Ding Tianhong Dec. 7, 2017, 1:11 a.m. UTC | #2 
Hi Andrew:

Sorry for the mistaken of the Andrey's email.

After the test I found this version still exist the problem that will transfer the align problem to size
mismatch, I will send a new version to fix it.

The correct way is like this:

diff --git a/lib/ubsan.c b/lib/ubsan.c
index fb0409d..0799678 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -321,9 +321,10 @@ void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,

        if (!ptr)
                handle_null_ptr_deref(data);
-       else if (data->alignment && !IS_ALIGNED(ptr, data->alignment))
-               handle_missaligned_access(data, ptr);
-       else
+       else if (data->alignment && !IS_ALIGNED(ptr, data->alignment)) {
+               if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
+                       handle_missaligned_access(data, ptr);
+       } else
                handle_object_size_mismatch(data, ptr);
 }
 EXPORT_SYMBOL(__ubsan_handle_type_mismatch);
--


Thanks
Ding

On 2017/12/7 8:49, 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.

> 

> Prevent the detection of unaligned access when the system support

> unaligned access.

> 

> Link: http://lkml.kernel.org/r/5b905d56-609e-3822-096a-3b93b3eb7675@huawei.com

> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

> Cc: David Laight <David.Laight@ACULAB.COM>

> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>

> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

> ---

> 

>  lib/ubsan.c |    3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)

> 

> diff -puN lib/ubsan.c~ubsan-dont-handle-misaligned-address-when-support-unaligned-access lib/ubsan.c

> --- a/lib/ubsan.c~ubsan-dont-handle-misaligned-address-when-support-unaligned-access

> +++ a/lib/ubsan.c

> @@ -322,7 +322,8 @@ void __ubsan_handle_type_mismatch(struct

>  	if (!ptr)

>  		handle_null_ptr_deref(data);

>  	else if (data->alignment && !IS_ALIGNED(ptr, data->alignment))

> -		handle_missaligned_access(data, ptr);

> +		if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))

> +			handle_missaligned_access(data, ptr);

>  	else

>  		handle_object_size_mismatch(data, ptr);

>  }

> _

> 

> 

> .

>
Andrey Ryabinin Dec. 7, 2017, 1:31 p.m. UTC | #3 
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.
Andrew Morton Dec. 7, 2017, 11:24 p.m. UTC | #4 
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?
David Laight Dec. 8, 2017, 10 a.m. UTC | #5 
From: Andrew Morton

> Sent: 07 December 2017 23:25

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


From the look of the code fragment it would appear that the function
is called when the original request wasn't actioned.
But you either want to action it or print the error.
Jus suppressing the error message cannot be right.

	David
Andrey Ryabinin Dec. 8, 2017, 10:48 a.m. UTC | #6 
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();
David Laight Dec. 8, 2017, 11:14 a.m. UTC | #7 
From: Andrey Ryabinin

> Sent: 08 December 2017 10:49

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


Don't think so.
Code that knows that unaligned accesses don't fault can set up pointers
that non-arch code dereferences.
Happens all the time in the networking stack.

...
> 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();


Ugg - shoot the gcc developers :-)

	David
Andrey Ryabinin Dec. 8, 2017, 11:45 a.m. UTC | #8 
On 12/08/2017 02:14 PM, David Laight wrote:
> From: Andrey Ryabinin

>> Sent: 08 December 2017 10:49

> ...

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

> 

> Don't think so.

> Code that knows that unaligned accesses don't fault can set up pointers

> that non-arch code dereferences.

> Happens all the time in the networking stack.

> 


Ok, *could* be a bug.


> ...

>> 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();

> 

> Ugg - shoot the gcc developers :-)

 
The gcc developers must have good self-preservation instinct, that's why they usually provide switch off for optimizations like this.
diff mbox series

Patch

diff --git a/lib/ubsan.c b/lib/ubsan.c
index fb0409d..9207e65 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -322,7 +322,8 @@  void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,
 	if (!ptr)
 		handle_null_ptr_deref(data);
 	else if (data->alignment && !IS_ALIGNED(ptr, data->alignment))
-		handle_missaligned_access(data, ptr);
+		if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
+			handle_missaligned_access(data, ptr);
 	else
 		handle_object_size_mismatch(data, ptr);
 }