All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/swap: Avoid undefined behavior in __swapoffset
@ 2019-03-07  9:46 Pi-Hsun Shih
  2019-03-07 12:23 ` Michal Hocko
  2019-03-12  8:18 ` [PATCH v2] mm/swap: Use offsetof instead of custom __swapoffset macro Pi-Hsun Shih
  0 siblings, 2 replies; 9+ messages in thread
From: Pi-Hsun Shih @ 2019-03-07  9:46 UTC (permalink / raw)
  Cc: Pi-Hsun Shih, Andrew Morton, Michal Hocko, Matthew Wilcox,
	Johannes Weiner, Vlastimil Babka, Minchan Kim, Omar Sandoval,
	Huang Ying, Aaron Lu, Tejun Heo, Wei Yang, open list

Use offsetof to calculate offset of a field to avoid UBSAN warning like:

===================================================================
UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
member access within null pointer of type 'union swap_header'
CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
Call trace:
 dump_backtrace+0x0/0x194
 show_stack+0x20/0x2c
 __dump_stack+0x20/0x28
 dump_stack+0x70/0x94
 ubsan_epilogue+0x14/0x44
 ubsan_type_mismatch_common+0xf4/0xfc
 __ubsan_handle_type_mismatch_v1+0x34/0x54
 __se_sys_swapon+0x654/0x1084
 __arm64_sys_swapon+0x1c/0x24
 el0_svc_common+0xa8/0x150
 el0_svc_compat_handler+0x2c/0x38
 el0_svc_compat+0x8/0x18
==================================================================

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 include/linux/swap.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index fc50e21b3b88..4bfb5c4ac108 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -157,9 +157,9 @@ struct swap_extent {
 /*
  * Max bad pages in the new format..
  */
-#define __swapoffset(x) ((unsigned long)&((union swap_header *)0)->x)
 #define MAX_SWAP_BADPAGES \
-	((__swapoffset(magic.magic) - __swapoffset(info.badpages)) / sizeof(int))
+	((offsetof(union swap_header, magic.magic) - \
+	  offsetof(union swap_header, info.badpages)) / sizeof(int))
 
 enum {
 	SWP_USED	= (1 << 0),	/* is slot in swap_info[] used? */
-- 
2.21.0.352.gf09ad66450-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/swap: Avoid undefined behavior in __swapoffset
  2019-03-07  9:46 [PATCH] mm/swap: Avoid undefined behavior in __swapoffset Pi-Hsun Shih
@ 2019-03-07 12:23 ` Michal Hocko
  2019-03-07 12:47   ` Pi-Hsun Shih
  2019-03-12  8:18 ` [PATCH v2] mm/swap: Use offsetof instead of custom __swapoffset macro Pi-Hsun Shih
  1 sibling, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2019-03-07 12:23 UTC (permalink / raw)
  To: Pi-Hsun Shih
  Cc: Andrew Morton, Matthew Wilcox, Johannes Weiner, Vlastimil Babka,
	Minchan Kim, Omar Sandoval, Huang Ying, Aaron Lu, Tejun Heo,
	Wei Yang, open list

On Thu 07-03-19 17:46:50, Pi-Hsun Shih wrote:
> Use offsetof to calculate offset of a field to avoid UBSAN warning like:
> 
> ===================================================================
> UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
> member access within null pointer of type 'union swap_header'
> CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
> Call trace:
>  dump_backtrace+0x0/0x194
>  show_stack+0x20/0x2c
>  __dump_stack+0x20/0x28
>  dump_stack+0x70/0x94
>  ubsan_epilogue+0x14/0x44
>  ubsan_type_mismatch_common+0xf4/0xfc
>  __ubsan_handle_type_mismatch_v1+0x34/0x54
>  __se_sys_swapon+0x654/0x1084
>  __arm64_sys_swapon+0x1c/0x24
>  el0_svc_common+0xa8/0x150
>  el0_svc_compat_handler+0x2c/0x38
>  el0_svc_compat+0x8/0x18
> ==================================================================

Could you be more specific about what exactly is undefined here and
why offsetof is any better. AFAIR it uses the same construct unless a
compiler defines a built in.

I do not object the change itself because it is cleaner to use the
existing helper but I am wondering why this is fixing ubsan. Is ubsan
defining the compiler variant and consider it safe?

> 
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> ---
>  include/linux/swap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index fc50e21b3b88..4bfb5c4ac108 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -157,9 +157,9 @@ struct swap_extent {
>  /*
>   * Max bad pages in the new format..
>   */
> -#define __swapoffset(x) ((unsigned long)&((union swap_header *)0)->x)
>  #define MAX_SWAP_BADPAGES \
> -	((__swapoffset(magic.magic) - __swapoffset(info.badpages)) / sizeof(int))
> +	((offsetof(union swap_header, magic.magic) - \
> +	  offsetof(union swap_header, info.badpages)) / sizeof(int))
>  
>  enum {
>  	SWP_USED	= (1 << 0),	/* is slot in swap_info[] used? */
> -- 
> 2.21.0.352.gf09ad66450-goog
> 

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/swap: Avoid undefined behavior in __swapoffset
  2019-03-07 12:23 ` Michal Hocko
@ 2019-03-07 12:47   ` Pi-Hsun Shih
  2019-03-07 13:23     ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: Pi-Hsun Shih @ 2019-03-07 12:47 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Matthew Wilcox, Johannes Weiner, Vlastimil Babka,
	Minchan Kim, Omar Sandoval, Huang Ying, Aaron Lu, Tejun Heo,
	Wei Yang, open list

On Thu, Mar 7, 2019 at 8:23 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Thu 07-03-19 17:46:50, Pi-Hsun Shih wrote:
> > Use offsetof to calculate offset of a field to avoid UBSAN warning like:
> >
> > ===================================================================
> > UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
> > member access within null pointer of type 'union swap_header'
> > CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
> > Call trace:
> >  dump_backtrace+0x0/0x194
> >  show_stack+0x20/0x2c
> >  __dump_stack+0x20/0x28
> >  dump_stack+0x70/0x94
> >  ubsan_epilogue+0x14/0x44
> >  ubsan_type_mismatch_common+0xf4/0xfc
> >  __ubsan_handle_type_mismatch_v1+0x34/0x54
> >  __se_sys_swapon+0x654/0x1084
> >  __arm64_sys_swapon+0x1c/0x24
> >  el0_svc_common+0xa8/0x150
> >  el0_svc_compat_handler+0x2c/0x38
> >  el0_svc_compat+0x8/0x18
> > ==================================================================
>
> Could you be more specific about what exactly is undefined here and
> why offsetof is any better. AFAIR it uses the same construct unless a
> compiler defines a built in.
>
> I do not object the change itself because it is cleaner to use the
> existing helper but I am wondering why this is fixing ubsan. Is ubsan
> defining the compiler variant and consider it safe?
>

The undefined behavior is from trying to accessing a member of NULL,
even not using it value but only use the address.

Since the compiler variant for offsetof is used for recent compiler
(GCC >= 4 has support for it), ubsan would not warn if the compiler
variant is used. For old compiler, I guess ubsan would complain on all
offsetof uses.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/swap: Avoid undefined behavior in __swapoffset
  2019-03-07 12:47   ` Pi-Hsun Shih
@ 2019-03-07 13:23     ` Michal Hocko
  2019-03-12  7:02       ` Pi-Hsun Shih
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2019-03-07 13:23 UTC (permalink / raw)
  To: Pi-Hsun Shih
  Cc: Andrew Morton, Matthew Wilcox, Johannes Weiner, Vlastimil Babka,
	Minchan Kim, Omar Sandoval, Huang Ying, Aaron Lu, Tejun Heo,
	Wei Yang, open list

On Thu 07-03-19 20:47:52, Pi-Hsun Shih wrote:
> On Thu, Mar 7, 2019 at 8:23 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Thu 07-03-19 17:46:50, Pi-Hsun Shih wrote:
> > > Use offsetof to calculate offset of a field to avoid UBSAN warning like:
> > >
> > > ===================================================================
> > > UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
> > > member access within null pointer of type 'union swap_header'
> > > CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
> > > Call trace:
> > >  dump_backtrace+0x0/0x194
> > >  show_stack+0x20/0x2c
> > >  __dump_stack+0x20/0x28
> > >  dump_stack+0x70/0x94
> > >  ubsan_epilogue+0x14/0x44
> > >  ubsan_type_mismatch_common+0xf4/0xfc
> > >  __ubsan_handle_type_mismatch_v1+0x34/0x54
> > >  __se_sys_swapon+0x654/0x1084
> > >  __arm64_sys_swapon+0x1c/0x24
> > >  el0_svc_common+0xa8/0x150
> > >  el0_svc_compat_handler+0x2c/0x38
> > >  el0_svc_compat+0x8/0x18
> > > ==================================================================
> >
> > Could you be more specific about what exactly is undefined here and
> > why offsetof is any better. AFAIR it uses the same construct unless a
> > compiler defines a built in.
> >
> > I do not object the change itself because it is cleaner to use the
> > existing helper but I am wondering why this is fixing ubsan. Is ubsan
> > defining the compiler variant and consider it safe?
> >
> 
> The undefined behavior is from trying to accessing a member of NULL,
> even not using it value but only use the address.

Hmm, we've been using this trick for ages and I do not remember any
compiler to complain as there is no real access. I am not sure what the
C standard has to tell about that but I presume reasonable compilers
will not abuse the UB here.

> Since the compiler variant for offsetof is used for recent compiler
> (GCC >= 4 has support for it), ubsan would not warn if the compiler
> variant is used. For old compiler, I guess ubsan would complain on all
> offsetof uses.

Is this the case for all compilers? If yes then we might want to drop
the non-compiler part. Btw. 
$ git grep "#define offsetof"
drivers/gpu/drm/radeon/mkregtable.c:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
drivers/infiniband/core/uverbs_cmd.c:#define offsetof_after(_struct, _member)                                       \
include/linux/stddef.h:#define offsetof(TYPE, MEMBER)   __compiler_offsetof(TYPE, MEMBER)
include/linux/stddef.h:#define offsetof(TYPE, MEMBER)   ((size_t)&((TYPE *)0)->MEMBER)
include/linux/stddef.h:#define offsetofend(TYPE, MEMBER) \
samples/bpf/cookie_uid_helper_example.c:#define offsetof(type, member)  __builtin_offsetof(type, member)
scripts/kconfig/list.h:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
tools/include/linux/kernel.h:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
tools/testing/selftests/bpf/test_select_reuseport_kern.c:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
tools/usb/usbip/libsrc/list.h:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)

so there is much more to take care of. Not to mention any open coded
variants.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/swap: Avoid undefined behavior in __swapoffset
  2019-03-07 13:23     ` Michal Hocko
@ 2019-03-12  7:02       ` Pi-Hsun Shih
  2019-03-12  8:07         ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: Pi-Hsun Shih @ 2019-03-12  7:02 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Matthew Wilcox, Johannes Weiner, Vlastimil Babka,
	Minchan Kim, Omar Sandoval, Huang Ying, Tejun Heo, Wei Yang,
	open list

On Thu, Mar 7, 2019 at 9:23 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Thu 07-03-19 20:47:52, Pi-Hsun Shih wrote:
> > On Thu, Mar 7, 2019 at 8:23 PM Michal Hocko <mhocko@kernel.org> wrote:
> > >
> > > On Thu 07-03-19 17:46:50, Pi-Hsun Shih wrote:
> > > > Use offsetof to calculate offset of a field to avoid UBSAN warning like:
> > > >
> > > > ===================================================================
> > > > UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
> > > > member access within null pointer of type 'union swap_header'
> > > > CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
> > > > Call trace:
> > > >  dump_backtrace+0x0/0x194
> > > >  show_stack+0x20/0x2c
> > > >  __dump_stack+0x20/0x28
> > > >  dump_stack+0x70/0x94
> > > >  ubsan_epilogue+0x14/0x44
> > > >  ubsan_type_mismatch_common+0xf4/0xfc
> > > >  __ubsan_handle_type_mismatch_v1+0x34/0x54
> > > >  __se_sys_swapon+0x654/0x1084
> > > >  __arm64_sys_swapon+0x1c/0x24
> > > >  el0_svc_common+0xa8/0x150
> > > >  el0_svc_compat_handler+0x2c/0x38
> > > >  el0_svc_compat+0x8/0x18
> > > > ==================================================================
> > >
> > > Could you be more specific about what exactly is undefined here and
> > > why offsetof is any better. AFAIR it uses the same construct unless a
> > > compiler defines a built in.
> > >
> > > I do not object the change itself because it is cleaner to use the
> > > existing helper but I am wondering why this is fixing ubsan. Is ubsan
> > > defining the compiler variant and consider it safe?
> > >
> >
> > The undefined behavior is from trying to accessing a member of NULL,
> > even not using it value but only use the address.
>
> Hmm, we've been using this trick for ages and I do not remember any
> compiler to complain as there is no real access. I am not sure what the
> C standard has to tell about that but I presume reasonable compilers
> will not abuse the UB here.
>

Some more testing shows that GCC optimize the
((size_t)&((type*)0)->member) to a constant in the result binary, and
never emit any UBSAN checks on the statement.
Clang doesn't optimize it to a constant in -O0, optimize it to a
constant in -O1 or above, and always emit the
__ubsan_handle_type_mismatch check when "-fsanitize=undefined" is
given.
So this UBSAN warning only happens when kernel is compiled by clang, not GCC.

From what I've found, it's a UB from C standard view point
(https://software.intel.com/en-us/blogs/2015/04/20/null-pointer-dereferencing-causes-undefined-behavior),
but I agree that probably no reasonable compilers would abuse the UB
here.

> > Since the compiler variant for offsetof is used for recent compiler
> > (GCC >= 4 has support for it), ubsan would not warn if the compiler
> > variant is used. For old compiler, I guess ubsan would complain on all
> > offsetof uses.
>
> Is this the case for all compilers? If yes then we might want to drop
> the non-compiler part. Btw.
> $ git grep "#define offsetof"
> drivers/gpu/drm/radeon/mkregtable.c:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
> drivers/infiniband/core/uverbs_cmd.c:#define offsetof_after(_struct, _member)                                       \
> include/linux/stddef.h:#define offsetof(TYPE, MEMBER)   __compiler_offsetof(TYPE, MEMBER)
> include/linux/stddef.h:#define offsetof(TYPE, MEMBER)   ((size_t)&((TYPE *)0)->MEMBER)
> include/linux/stddef.h:#define offsetofend(TYPE, MEMBER) \
> samples/bpf/cookie_uid_helper_example.c:#define offsetof(type, member)  __builtin_offsetof(type, member)
> scripts/kconfig/list.h:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
> tools/include/linux/kernel.h:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
> tools/testing/selftests/bpf/test_select_reuseport_kern.c:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
> tools/usb/usbip/libsrc/list.h:#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
>
> so there is much more to take care of. Not to mention any open coded
> variants.

Did a "git grep '(0|NULL)\)->'" and there are about 25 of them that
can be changed to use offsetof.

> --
> Michal Hocko
> SUSE Labs

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/swap: Avoid undefined behavior in __swapoffset
  2019-03-12  7:02       ` Pi-Hsun Shih
@ 2019-03-12  8:07         ` Michal Hocko
  2019-03-12  8:12           ` Pi-Hsun Shih
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2019-03-12  8:07 UTC (permalink / raw)
  To: Pi-Hsun Shih
  Cc: Andrew Morton, Matthew Wilcox, Johannes Weiner, Vlastimil Babka,
	Minchan Kim, Omar Sandoval, Huang Ying, Tejun Heo, Wei Yang,
	open list

On Tue 12-03-19 15:02:38, Pi-Hsun Shih wrote:
> On Thu, Mar 7, 2019 at 9:23 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Thu 07-03-19 20:47:52, Pi-Hsun Shih wrote:
> > > On Thu, Mar 7, 2019 at 8:23 PM Michal Hocko <mhocko@kernel.org> wrote:
> > > >
> > > > On Thu 07-03-19 17:46:50, Pi-Hsun Shih wrote:
> > > > > Use offsetof to calculate offset of a field to avoid UBSAN warning like:
> > > > >
> > > > > ===================================================================
> > > > > UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
> > > > > member access within null pointer of type 'union swap_header'
> > > > > CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
> > > > > Call trace:
> > > > >  dump_backtrace+0x0/0x194
> > > > >  show_stack+0x20/0x2c
> > > > >  __dump_stack+0x20/0x28
> > > > >  dump_stack+0x70/0x94
> > > > >  ubsan_epilogue+0x14/0x44
> > > > >  ubsan_type_mismatch_common+0xf4/0xfc
> > > > >  __ubsan_handle_type_mismatch_v1+0x34/0x54
> > > > >  __se_sys_swapon+0x654/0x1084
> > > > >  __arm64_sys_swapon+0x1c/0x24
> > > > >  el0_svc_common+0xa8/0x150
> > > > >  el0_svc_compat_handler+0x2c/0x38
> > > > >  el0_svc_compat+0x8/0x18
> > > > > ==================================================================
> > > >
> > > > Could you be more specific about what exactly is undefined here and
> > > > why offsetof is any better. AFAIR it uses the same construct unless a
> > > > compiler defines a built in.
> > > >
> > > > I do not object the change itself because it is cleaner to use the
> > > > existing helper but I am wondering why this is fixing ubsan. Is ubsan
> > > > defining the compiler variant and consider it safe?
> > > >
> > >
> > > The undefined behavior is from trying to accessing a member of NULL,
> > > even not using it value but only use the address.
> >
> > Hmm, we've been using this trick for ages and I do not remember any
> > compiler to complain as there is no real access. I am not sure what the
> > C standard has to tell about that but I presume reasonable compilers
> > will not abuse the UB here.
> >
> 
> Some more testing shows that GCC optimize the
> ((size_t)&((type*)0)->member) to a constant in the result binary, and
> never emit any UBSAN checks on the statement.
> Clang doesn't optimize it to a constant in -O0, optimize it to a
> constant in -O1 or above, and always emit the
> __ubsan_handle_type_mismatch check when "-fsanitize=undefined" is
> given.
> So this UBSAN warning only happens when kernel is compiled by clang, not GCC.
> 
> From what I've found, it's a UB from C standard view point
> (https://software.intel.com/en-us/blogs/2015/04/20/null-pointer-dereferencing-causes-undefined-behavior),
> but I agree that probably no reasonable compilers would abuse the UB
> here.

I really do not want to go and lawyering about the standard here but
getting an address of an offset based on NULL ptr is not really
dereferencing of a NULL ptr. At least this was not the case for ages
and no compiler can afford to change it because there is quite a lot of
userspace to rely on this construct.

But as I've said using offseoff is nicer so I completely support a patch
that get's read of a custom redefinition of it or open code directly.
But calling it an UB is a bit of stretch.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/swap: Avoid undefined behavior in __swapoffset
  2019-03-12  8:07         ` Michal Hocko
@ 2019-03-12  8:12           ` Pi-Hsun Shih
  0 siblings, 0 replies; 9+ messages in thread
From: Pi-Hsun Shih @ 2019-03-12  8:12 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Matthew Wilcox, Johannes Weiner, Vlastimil Babka,
	Minchan Kim, Omar Sandoval, Huang Ying, Tejun Heo, Wei Yang,
	open list

On Tue, Mar 12, 2019 at 4:07 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Tue 12-03-19 15:02:38, Pi-Hsun Shih wrote:
> > On Thu, Mar 7, 2019 at 9:23 PM Michal Hocko <mhocko@kernel.org> wrote:
> > >
> > > On Thu 07-03-19 20:47:52, Pi-Hsun Shih wrote:
> > > > On Thu, Mar 7, 2019 at 8:23 PM Michal Hocko <mhocko@kernel.org> wrote:
> > > > >
> > > > > On Thu 07-03-19 17:46:50, Pi-Hsun Shih wrote:
> > > > > > Use offsetof to calculate offset of a field to avoid UBSAN warning like:
> > > > > >
> > > > > > ===================================================================
> > > > > > UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
> > > > > > member access within null pointer of type 'union swap_header'
> > > > > > CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
> > > > > > Call trace:
> > > > > >  dump_backtrace+0x0/0x194
> > > > > >  show_stack+0x20/0x2c
> > > > > >  __dump_stack+0x20/0x28
> > > > > >  dump_stack+0x70/0x94
> > > > > >  ubsan_epilogue+0x14/0x44
> > > > > >  ubsan_type_mismatch_common+0xf4/0xfc
> > > > > >  __ubsan_handle_type_mismatch_v1+0x34/0x54
> > > > > >  __se_sys_swapon+0x654/0x1084
> > > > > >  __arm64_sys_swapon+0x1c/0x24
> > > > > >  el0_svc_common+0xa8/0x150
> > > > > >  el0_svc_compat_handler+0x2c/0x38
> > > > > >  el0_svc_compat+0x8/0x18
> > > > > > ==================================================================
> > > > >
> > > > > Could you be more specific about what exactly is undefined here and
> > > > > why offsetof is any better. AFAIR it uses the same construct unless a
> > > > > compiler defines a built in.
> > > > >
> > > > > I do not object the change itself because it is cleaner to use the
> > > > > existing helper but I am wondering why this is fixing ubsan. Is ubsan
> > > > > defining the compiler variant and consider it safe?
> > > > >
> > > >
> > > > The undefined behavior is from trying to accessing a member of NULL,
> > > > even not using it value but only use the address.
> > >
> > > Hmm, we've been using this trick for ages and I do not remember any
> > > compiler to complain as there is no real access. I am not sure what the
> > > C standard has to tell about that but I presume reasonable compilers
> > > will not abuse the UB here.
> > >
> >
> > Some more testing shows that GCC optimize the
> > ((size_t)&((type*)0)->member) to a constant in the result binary, and
> > never emit any UBSAN checks on the statement.
> > Clang doesn't optimize it to a constant in -O0, optimize it to a
> > constant in -O1 or above, and always emit the
> > __ubsan_handle_type_mismatch check when "-fsanitize=undefined" is
> > given.
> > So this UBSAN warning only happens when kernel is compiled by clang, not GCC.
> >
> > From what I've found, it's a UB from C standard view point
> > (https://software.intel.com/en-us/blogs/2015/04/20/null-pointer-dereferencing-causes-undefined-behavior),
> > but I agree that probably no reasonable compilers would abuse the UB
> > here.
>
> I really do not want to go and lawyering about the standard here but
> getting an address of an offset based on NULL ptr is not really
> dereferencing of a NULL ptr. At least this was not the case for ages
> and no compiler can afford to change it because there is quite a lot of
> userspace to rely on this construct.
>
> But as I've said using offseoff is nicer so I completely support a patch
> that get's read of a custom redefinition of it or open code directly.
> But calling it an UB is a bit of stretch.

Ok I'll send a v2 with a better commit title / message.

> --
> Michal Hocko
> SUSE Labs

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] mm/swap: Use offsetof instead of custom __swapoffset macro.
  2019-03-07  9:46 [PATCH] mm/swap: Avoid undefined behavior in __swapoffset Pi-Hsun Shih
  2019-03-07 12:23 ` Michal Hocko
@ 2019-03-12  8:18 ` Pi-Hsun Shih
  2019-03-12  8:26   ` Michal Hocko
  1 sibling, 1 reply; 9+ messages in thread
From: Pi-Hsun Shih @ 2019-03-12  8:18 UTC (permalink / raw)
  Cc: Pi-Hsun Shih, Andrew Morton, Michal Hocko, Matthew Wilcox,
	Johannes Weiner, Stephen Rothwell, Minchan Kim, Huang Ying,
	Wei Yang, Yang Shi, Aaron Lu, Tejun Heo, Omar Sandoval,
	open list

Use offsetof to calculate offset of a field to take advantage of
compiler built-in version when possible, and avoid UBSAN warning when
compiling with Clang:

UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
member access within null pointer of type 'union swap_header'
CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
Call trace:
 dump_backtrace+0x0/0x194
 show_stack+0x20/0x2c
 __dump_stack+0x20/0x28
 dump_stack+0x70/0x94
 ubsan_epilogue+0x14/0x44
 ubsan_type_mismatch_common+0xf4/0xfc
 __ubsan_handle_type_mismatch_v1+0x34/0x54
 __se_sys_swapon+0x654/0x1084
 __arm64_sys_swapon+0x1c/0x24
 el0_svc_common+0xa8/0x150
 el0_svc_compat_handler+0x2c/0x38
 el0_svc_compat+0x8/0x18

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 include/linux/swap.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index fc50e21b3b88..4bfb5c4ac108 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -157,9 +157,9 @@ struct swap_extent {
 /*
  * Max bad pages in the new format..
  */
-#define __swapoffset(x) ((unsigned long)&((union swap_header *)0)->x)
 #define MAX_SWAP_BADPAGES \
-	((__swapoffset(magic.magic) - __swapoffset(info.badpages)) / sizeof(int))
+	((offsetof(union swap_header, magic.magic) - \
+	  offsetof(union swap_header, info.badpages)) / sizeof(int))
 
 enum {
 	SWP_USED	= (1 << 0),	/* is slot in swap_info[] used? */
-- 
2.21.0.360.g471c308f928-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] mm/swap: Use offsetof instead of custom __swapoffset macro.
  2019-03-12  8:18 ` [PATCH v2] mm/swap: Use offsetof instead of custom __swapoffset macro Pi-Hsun Shih
@ 2019-03-12  8:26   ` Michal Hocko
  0 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2019-03-12  8:26 UTC (permalink / raw)
  To: Pi-Hsun Shih
  Cc: Andrew Morton, Matthew Wilcox, Johannes Weiner, Stephen Rothwell,
	Minchan Kim, Huang Ying, Wei Yang, Yang Shi, Aaron Lu, Tejun Heo,
	Omar Sandoval, open list

On Tue 12-03-19 16:18:59, Pi-Hsun Shih wrote:
> Use offsetof to calculate offset of a field to take advantage of
> compiler built-in version when possible, and avoid UBSAN warning when
> compiling with Clang:
> 
> UBSAN: Undefined behaviour in mm/swapfile.c:3010:38
> member access within null pointer of type 'union swap_header'
> CPU: 6 PID: 1833 Comm: swapon Tainted: G S                4.19.23 #43
> Call trace:
>  dump_backtrace+0x0/0x194
>  show_stack+0x20/0x2c
>  __dump_stack+0x20/0x28
>  dump_stack+0x70/0x94
>  ubsan_epilogue+0x14/0x44
>  ubsan_type_mismatch_common+0xf4/0xfc
>  __ubsan_handle_type_mismatch_v1+0x34/0x54
>  __se_sys_swapon+0x654/0x1084
>  __arm64_sys_swapon+0x1c/0x24
>  el0_svc_common+0xa8/0x150
>  el0_svc_compat_handler+0x2c/0x38
>  el0_svc_compat+0x8/0x18
> 
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>

Yes this makes more sense. I still think the warning is bogus but
whatever. A cleanup is worth it.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  include/linux/swap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index fc50e21b3b88..4bfb5c4ac108 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -157,9 +157,9 @@ struct swap_extent {
>  /*
>   * Max bad pages in the new format..
>   */
> -#define __swapoffset(x) ((unsigned long)&((union swap_header *)0)->x)
>  #define MAX_SWAP_BADPAGES \
> -	((__swapoffset(magic.magic) - __swapoffset(info.badpages)) / sizeof(int))
> +	((offsetof(union swap_header, magic.magic) - \
> +	  offsetof(union swap_header, info.badpages)) / sizeof(int))
>  
>  enum {
>  	SWP_USED	= (1 << 0),	/* is slot in swap_info[] used? */
> -- 
> 2.21.0.360.g471c308f928-goog
> 

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-03-12  8:26 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-07  9:46 [PATCH] mm/swap: Avoid undefined behavior in __swapoffset Pi-Hsun Shih
2019-03-07 12:23 ` Michal Hocko
2019-03-07 12:47   ` Pi-Hsun Shih
2019-03-07 13:23     ` Michal Hocko
2019-03-12  7:02       ` Pi-Hsun Shih
2019-03-12  8:07         ` Michal Hocko
2019-03-12  8:12           ` Pi-Hsun Shih
2019-03-12  8:18 ` [PATCH v2] mm/swap: Use offsetof instead of custom __swapoffset macro Pi-Hsun Shih
2019-03-12  8:26   ` Michal Hocko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.