[RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Arnd Bergmann]

Hi Dmitry,

I've come a little further on the stack size analysis after initially
finding that gcc-7.0.1 is much better than the horrible stack frames
we were seeing on gcc-7.0.0 (I found over 80kb in one case), I found
more that remain with the newer compiler, but also analyzed the worst
remaining ones down to two common helpers: typecheck() caused the
worst remaining problem, but only in a few files like
"drivers/net/wireless/ralink/rt2x00/rt2800lib.c:5068:1: error: the frame
size of 23768 bytes is larger than 1024 bytes". I think my fix should
be able to make it in, but I'd give it some more testing. It also seems
here that gcc asan-stack isn't too smart, as it adds checks to local
variables we never use except for comparing their addresses. This may
also impact min() and max(), which have the same check.

READ_ONCE()/WRITE_ONCE() are used for atomic_t and caused the next worst
offender (12688 byte stacks in lib/atomic64_test.c) as well as a lot
of other instances that were over 2048 byte stacks. This one is a lot
trickier as my the version from my patch is most likely not safe
for all callers, and the helper has been extended to cover a lot of
corner cases that my version destroys (it doesn't force aggregates
to be accessed as scalars for example, probably also causes sparse
warnings, and maybe doesn't even guarantee the "ONCE" semantics).

I see that Andrey also provided a READ_ONCE_NOCHECK() helper that
would also take care of this if used consistently, but it seems
wrong to use that for all atomics. Any other ideas?

     Arnd

diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index fcc5cd387fd1..0c243dd569fe 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -30,7 +30,7 @@ static inline void prepare_switch_to(struct task_struct *prev,
 	 *
 	 * To minimize cache pollution, just follow the stack pointer.
 	 */
-	READ_ONCE(*(unsigned char *)next->thread.sp);
+	(void)READ_ONCE(*(unsigned char *)next->thread.sp);
 #endif
 }
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 01157d6e8cfe..dc677c7c22be 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -222,8 +222,8 @@ void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry)
 
 void ovl_inode_init(struct inode *inode, struct inode *realinode, bool is_upper)
 {
-	WRITE_ONCE(inode->i_private, (unsigned long) realinode |
-		   (is_upper ? OVL_ISUPPER_MASK : 0));
+	WRITE_ONCE(inode->i_private, (void *)((unsigned long) realinode |
+		   (is_upper ? OVL_ISUPPER_MASK : 0)));
 }
 
 void ovl_inode_update(struct inode *inode, struct inode *upperinode)
@@ -231,7 +231,7 @@ void ovl_inode_update(struct inode *inode, struct inode *upperinode)
 	WARN_ON(!upperinode);
 	WARN_ON(!inode_unhashed(inode));
 	WRITE_ONCE(inode->i_private,
-		   (unsigned long) upperinode | OVL_ISUPPER_MASK);
+		   (void *)((unsigned long) upperinode | OVL_ISUPPER_MASK));
 	if (!S_ISDIR(upperinode->i_mode))
 		__insert_inode_hash(inode, (unsigned long) upperinode);
 }
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 416b17e03016..b8018eddd757 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -310,14 +310,17 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 
 #define __READ_ONCE(x, check)						\
-({									\
-	union { typeof(x) __val; char __c[1]; } __u;			\
+	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x), \
+	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x), \
+	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x), \
+	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x), \
+	({union { typeof(x) __val; char __c[1]; } __u;			\
 	if (check)							\
 		__read_once_size(&(x), __u.__c, sizeof(x));		\
 	else								\
 		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
 	__u.__val;							\
-})
+	})))))
 #define READ_ONCE(x) __READ_ONCE(x, 1)
 
 /*
@@ -326,13 +329,19 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)
 
-#define WRITE_ONCE(x, val) \
-({							\
-	union { typeof(x) __val; char __c[1]; } __u =	\
-		{ .__val = (__force typeof(x)) (val) }; \
-	__write_once_size(&(x), __u.__c, sizeof(x));	\
-	__u.__val;					\
-})
+#define WRITE_ONCE(x, val)								\
+(											\
+	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x) = (val),	\
+	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x) = (val),	\
+	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x) = (val),	\
+	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x) = (val),	\
+	({										\
+		typeof(x) __val = (val);						\
+		barrier();								\
+		__builtin_memcpy((void *)&x, (void *)&__val, sizeof(__val));			\
+		barrier();								\
+	})))))			\
+)
 
 #endif /* __KERNEL__ */
 
diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h
index eb5b74a575be..adb1579fa5f0 100644
--- a/include/linux/typecheck.h
+++ b/include/linux/typecheck.h
@@ -5,12 +5,7 @@
  * Check at compile time that something is of a particular type.
  * Always evaluates to 1 so you may use it easily in comparisons.
  */
-#define typecheck(type,x) \
-({	type __dummy; \
-	typeof(x) __dummy2; \
-	(void)(&__dummy == &__dummy2); \
-	1; \
-})
+#define typecheck(type,x) ({(void)((typeof(type) *)NULL == (typeof(x) *)NULL); 1;})
 
 /*
  * Check at compile time that 'function' is a certain type, or is a pointer

Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Dmitry Vyukov]

On Wed, Feb 15, 2017 at 12:03 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> Hi Dmitry,
>
> I've come a little further on the stack size analysis after initially
> finding that gcc-7.0.1 is much better than the horrible stack frames
> we were seeing on gcc-7.0.0 (I found over 80kb in one case), I found
> more that remain with the newer compiler, but also analyzed the worst
> remaining ones down to two common helpers: typecheck() caused the
> worst remaining problem, but only in a few files like
> "drivers/net/wireless/ralink/rt2x00/rt2800lib.c:5068:1: error: the frame
> size of 23768 bytes is larger than 1024 bytes". I think my fix should
> be able to make it in, but I'd give it some more testing. It also seems
> here that gcc asan-stack isn't too smart, as it adds checks to local
> variables we never use except for comparing their addresses. This may
> also impact min() and max(), which have the same check.
>
> READ_ONCE()/WRITE_ONCE() are used for atomic_t and caused the next worst
> offender (12688 byte stacks in lib/atomic64_test.c) as well as a lot
> of other instances that were over 2048 byte stacks. This one is a lot
> trickier as my the version from my patch is most likely not safe
> for all callers, and the helper has been extended to cover a lot of
> corner cases that my version destroys (it doesn't force aggregates
> to be accessed as scalars for example, probably also causes sparse
> warnings, and maybe doesn't even guarantee the "ONCE" semantics).
>
> I see that Andrey also provided a READ_ONCE_NOCHECK() helper that
> would also take care of this if used consistently, but it seems
> wrong to use that for all atomics. Any other ideas?


Hi Arnd,

Frankly it looks like something that needs to be fixed in compilers.
However, that probably won't happen in near future.
Our bet is on clang where we would be interested in fixing such issues.

Alexander, can you please check how clang compares with gcc with asan
stack instrumentation on typecheck and READ_ONCE?
If clang introduces additional redzones due to code in typecheck and
READ_ONCE, then it looks like something that we need to fix. Not sure
why typecheck affects codegen at all...

Re committing this change: Arnd, do you have an estimation on number
of changes we will need to land to enable the warning? If it's
handful, then it may be worth pursuing (but we still need a proper
impl for READ_ONCE).


> diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
> index fcc5cd387fd1..0c243dd569fe 100644
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -30,7 +30,7 @@ static inline void prepare_switch_to(struct task_struct *prev,
>          *
>          * To minimize cache pollution, just follow the stack pointer.
>          */
> -       READ_ONCE(*(unsigned char *)next->thread.sp);
> +       (void)READ_ONCE(*(unsigned char *)next->thread.sp);
>  #endif
>  }
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 01157d6e8cfe..dc677c7c22be 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -222,8 +222,8 @@ void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry)
>
>  void ovl_inode_init(struct inode *inode, struct inode *realinode, bool is_upper)
>  {
> -       WRITE_ONCE(inode->i_private, (unsigned long) realinode |
> -                  (is_upper ? OVL_ISUPPER_MASK : 0));
> +       WRITE_ONCE(inode->i_private, (void *)((unsigned long) realinode |
> +                  (is_upper ? OVL_ISUPPER_MASK : 0)));
>  }
>
>  void ovl_inode_update(struct inode *inode, struct inode *upperinode)
> @@ -231,7 +231,7 @@ void ovl_inode_update(struct inode *inode, struct inode *upperinode)
>         WARN_ON(!upperinode);
>         WARN_ON(!inode_unhashed(inode));
>         WRITE_ONCE(inode->i_private,
> -                  (unsigned long) upperinode | OVL_ISUPPER_MASK);
> +                  (void *)((unsigned long) upperinode | OVL_ISUPPER_MASK));
>         if (!S_ISDIR(upperinode->i_mode))
>                 __insert_inode_hash(inode, (unsigned long) upperinode);
>  }
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 416b17e03016..b8018eddd757 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -310,14 +310,17 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>   */
>
>  #define __READ_ONCE(x, check)                                          \
> -({                                                                     \
> -       union { typeof(x) __val; char __c[1]; } __u;                    \
> +       __builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x), \
> +       __builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x), \
> +       __builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x), \
> +       __builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x), \
> +       ({union { typeof(x) __val; char __c[1]; } __u;                  \
>         if (check)                                                      \
>                 __read_once_size(&(x), __u.__c, sizeof(x));             \
>         else                                                            \
>                 __read_once_size_nocheck(&(x), __u.__c, sizeof(x));     \
>         __u.__val;                                                      \
> -})
> +       })))))
>  #define READ_ONCE(x) __READ_ONCE(x, 1)
>
>  /*
> @@ -326,13 +329,19 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>   */
>  #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)
>
> -#define WRITE_ONCE(x, val) \
> -({                                                     \
> -       union { typeof(x) __val; char __c[1]; } __u =   \
> -               { .__val = (__force typeof(x)) (val) }; \
> -       __write_once_size(&(x), __u.__c, sizeof(x));    \
> -       __u.__val;                                      \
> -})
> +#define WRITE_ONCE(x, val)                                                             \
> +(                                                                                      \
> +       __builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x) = (val),     \
> +       __builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x) = (val),     \
> +       __builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x) = (val),     \
> +       __builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x) = (val),  \
> +       ({                                                                              \
> +               typeof(x) __val = (val);                                                \
> +               barrier();                                                              \
> +               __builtin_memcpy((void *)&x, (void *)&__val, sizeof(__val));                    \
> +               barrier();                                                              \
> +       })))))                  \
> +)
>
>  #endif /* __KERNEL__ */
>
> diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h
> index eb5b74a575be..adb1579fa5f0 100644
> --- a/include/linux/typecheck.h
> +++ b/include/linux/typecheck.h
> @@ -5,12 +5,7 @@
>   * Check at compile time that something is of a particular type.
>   * Always evaluates to 1 so you may use it easily in comparisons.
>   */
> -#define typecheck(type,x) \
> -({     type __dummy; \
> -       typeof(x) __dummy2; \
> -       (void)(&__dummy == &__dummy2); \
> -       1; \
> -})
> +#define typecheck(type,x) ({(void)((typeof(type) *)NULL == (typeof(x) *)NULL); 1;})
>
>  /*
>   * Check at compile time that 'function' is a certain type, or is a pointer
>

Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Arnd Bergmann]

On Wed, Feb 15, 2017 at 10:18 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Feb 15, 2017 at 12:03 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> Hi Arnd,
>
> Frankly it looks like something that needs to be fixed in compilers.
> However, that probably won't happen in near future.
> Our bet is on clang where we would be interested in fixing such issues.
>
> Alexander, can you please check how clang compares with gcc with asan
> stack instrumentation on typecheck and READ_ONCE?
> If clang introduces additional redzones due to code in typecheck and
> READ_ONCE, then it looks like something that we need to fix. Not sure
> why typecheck affects codegen at all...
>
> Re committing this change: Arnd, do you have an estimation on number
> of changes we will need to land to enable the warning? If it's
> handful, then it may be worth pursuing (but we still need a proper
> impl for READ_ONCE).

My current estimate is only a handful of patches to enable
CONFIG_FRAME_WARN=3072 with a clean build, and maybe a few dozen
more to get down to 2048 with gcc-7.0.1 (no idea for older versions).

What I'm looking at now is this set of remaining warnings (>3072 bytes)
and 24 other files >2048:

drivers/gpu/drm/i915/gvt/handlers.c:2200:1: error: the frame size of
30448 bytes is larger than 3000 bytes
drivers/media/usb/em28xx/em28xx-dvb.c:2061:1: error: the frame size of
4264 bytes is larger than 3000 bytes
drivers/staging/unisys/visorbus/visorchipset.c:1466:1: error: the
frame size of 3008 bytes is larger than 3000 bytes
fs/ocfs2/super.c:1219:1: error: the frame size of 3232 bytes is larger
than 3000 bytes
lib/lz4/lz4_compress.c:883:1: error: the frame size of 6696 bytes is
larger than 3000 bytes
lib/lz4/lz4hc_compress.c:579:1: error: the frame size of 5576 bytes is
larger than 3000 bytes

and this set of patches I have applied locally, most of which need
some more work:

# shown here earlier, need work
rewrite READ_ONCE/WRITE_ONCE
typecheck.h: avoid local variables in typecheck() macro
# needs rewrite to use __uninline_for_kasan
[SUBMITTED 20170208] netlink: move nla_put_{u8,u16,u32} out of line
# pending for 4.11 or 4.12
[SUBMITTED 20170208] [media] tc358743: fix register i2c_rd/wr functions
[SUBMITTED 20170214] rt2500usb: don't mark register accesses as inline
# these just add more __uninline_for_kasan in driver specific code
staging: dgnc: reduce stack usage with KASAN
[media] i2c: cx25840: avoid stack overflow with KASAN
[media] tuners: reduce stack usage with kasan
[media] i2c: ks0127: reduce stack frame size
[media] dvb-frontends: reduce stack size in i2c access
[media] r820t: rewrite register access to reduce stack size
tty: kbd: reduce stack size with KASAN

     Arnd

Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Arnd Bergmann]

On Wed, Feb 15, 2017 at 11:34 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Feb 15, 2017 at 10:18 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Wed, Feb 15, 2017 at 12:03 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> My current estimate is only a handful of patches to enable
> CONFIG_FRAME_WARN=3072 with a clean build, and maybe a few dozen
> more to get down to 2048 with gcc-7.0.1 (no idea for older versions).
>
> What I'm looking at now is this set of remaining warnings (>3072 bytes)
> and 24 other files >2048:

I now found that most of the remaining ones go away when I make
CONFIG_KMEMCHECK depend on !KASAN_EXTRA (from my earlier patch).
Presumably this definition:

static __always_inline void *__inline_memcpy(void *to, const void
*from, size_t n)
{
        unsigned long d0, d1, d2;
        asm volatile("rep ; movsl\n\t"
                     "testb $2,%b4\n\t"
                     "je 1f\n\t"
                     "movsw\n"
                     "1:\ttestb $1,%b4\n\t"
                     "je 2f\n\t"
                     "movsb\n"
                     "2:"
                     : "=&c" (d0), "=&D" (d1), "=&S" (d2)
                     : "0" (n / 4), "q" (n), "1" ((long)to), "2" ((long)from)
                     : "memory");
        return to;
}

is the culprit when building with -fsanitize-address-use-after-scope

     Arnd

Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Andrey Ryabinin]

On 02/15/2017 02:03 AM, Arnd Bergmann wrote:
> Hi Dmitry,
> 
> I've come a little further on the stack size analysis after initially
> finding that gcc-7.0.1 is much better than the horrible stack frames
> we were seeing on gcc-7.0.0 (I found over 80kb in one case), I found
> more that remain with the newer compiler, but also analyzed the worst
> remaining ones down to two common helpers: typecheck() caused the
> worst remaining problem, but only in a few files like
> "drivers/net/wireless/ralink/rt2x00/rt2800lib.c:5068:1: error: the frame
> size of 23768 bytes is larger than 1024 bytes". I think my fix should
> be able to make it in, but I'd give it some more testing. It also seems
> here that gcc asan-stack isn't too smart, as it adds checks to local
> variables we never use except for comparing their addresses. This may
> also impact min() and max(), which have the same check.
> 
> READ_ONCE()/WRITE_ONCE() are used for atomic_t and caused the next worst
> offender (12688 byte stacks in lib/atomic64_test.c) as well as a lot
> of other instances that were over 2048 byte stacks. This one is a lot
> trickier as my the version from my patch is most likely not safe
> for all callers, and the helper has been extended to cover a lot of
> corner cases that my version destroys (it doesn't force aggregates
> to be accessed as scalars for example,

I think the following patch on top of yours should fix that for READ_ONCE().
WRITE_ONCE() probably can be fixed in a similar way, but I didn't try it.

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 10bca12..5d9dd63 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -301,10 +301,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 
 #define __READ_ONCE(x, check)						\
-	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x), \
-	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x), \
-	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x), \
-	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x), \
+	__builtin_choose_expr(sizeof(x) == 1, (typeof(x))(__u64)*(volatile __u8 *)&(x), \
+	__builtin_choose_expr(sizeof(x) == 2, (typeof(x))(__u64)*(volatile __u16 *)&(x), \
+	__builtin_choose_expr(sizeof(x) == 4, (typeof(x))(__u64)*(volatile __u32 *)&(x), \
+	__builtin_choose_expr(sizeof(x) == 8, (typeof(x))(__u64)*(volatile __u64 *)&(x), \
 	({union { typeof(x) __val; char __c[1]; } __u;			\
 	if (check)							\
 		__read_once_size(&(x), __u.__c, sizeof(x));		\


> probably also causes sparse
> warnings, and maybe doesn't even guarantee the "ONCE" semantics).
> 
> I see that Andrey also provided a READ_ONCE_NOCHECK() helper that
> would also take care of this if used consistently, but it seems
> wrong to use that for all atomics. Any other ideas?
> 
>      Arnd
> 

> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 416b17e03016..b8018eddd757 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -310,14 +310,17 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>   */
>  
>  #define __READ_ONCE(x, check)						\
> -({									\
> -	union { typeof(x) __val; char __c[1]; } __u;			\
> +	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x), \
> +	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x), \
> +	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x), \
> +	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x), \
> +	({union { typeof(x) __val; char __c[1]; } __u;			\

This should be under "if (check)" otherwise it breaks READ_ONCE_NOCHECK()

>  	if (check)							\
>  		__read_once_size(&(x), __u.__c, sizeof(x));		\
>  	else								\
>  		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
>  	__u.__val;							\
> -})
> +	})))))
>  #define READ_ONCE(x) __READ_ONCE(x, 1)
>  

Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Andrey Ryabinin]

On 02/15/2017 05:06 PM, Andrey Ryabinin wrote:

> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 10bca12..5d9dd63 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -301,10 +301,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>   */
>  
>  #define __READ_ONCE(x, check)						\
> -	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x), \
> -	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x), \
> -	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x), \
> -	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x), \
> +	__builtin_choose_expr(sizeof(x) == 1, (typeof(x))(__u64)*(volatile __u8 *)&(x), \
> +	__builtin_choose_expr(sizeof(x) == 2, (typeof(x))(__u64)*(volatile __u16 *)&(x), \
> +	__builtin_choose_expr(sizeof(x) == 4, (typeof(x))(__u64)*(volatile __u32 *)&(x), \
> +	__builtin_choose_expr(sizeof(x) == 8, (typeof(x))(__u64)*(volatile __u64 *)&(x), \
>  	({union { typeof(x) __val; char __c[1]; } __u;			\
>  	if (check)							\
>  		__read_once_size(&(x), __u.__c, sizeof(x));		\

Scratch this, it doesn't work:
../arch/x86/mm/gup.c:20:2: error: conversion to non-scalar type requested
  return READ_ONCE(*ptep);
  ^

Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Christian Borntraeger]

On 02/15/2017 12:03 AM, Arnd Bergmann wrote:
> 
> -#define WRITE_ONCE(x, val) \
> -({							\
> -	union { typeof(x) __val; char __c[1]; } __u =	\
> -		{ .__val = (__force typeof(x)) (val) }; \
> -	__write_once_size(&(x), __u.__c, sizeof(x));	\
> -	__u.__val;					\
> -})
> +#define WRITE_ONCE(x, val)								\
> +(											\
> +	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x) = (val),	\
> +	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x) = (val),	\
> +	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x) = (val),	\
> +	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x) = (val),	\

Have you run sparse on those changes?
IIRC we had to add the __force to get rid of address space annotations
in that macro above. Cannot tell if we need something like that here.

Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Christian Borntraeger]

On 02/15/2017 05:18 PM, Christian Borntraeger wrote:
> On 02/15/2017 12:03 AM, Arnd Bergmann wrote:
>>
>> -#define WRITE_ONCE(x, val) \
>> -({							\
>> -	union { typeof(x) __val; char __c[1]; } __u =	\
>> -		{ .__val = (__force typeof(x)) (val) }; \
>> -	__write_once_size(&(x), __u.__c, sizeof(x));	\
>> -	__u.__val;					\
>> -})
>> +#define WRITE_ONCE(x, val)								\
>> +(											\
>> +	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x) = (val),	\
>> +	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x) = (val),	\
>> +	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x) = (val),	\
>> +	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x) = (val),	\
> 
> Have you run sparse on those changes?
> IIRC we had to add the __force to get rid of address space annotations
> in that macro above. Cannot tell if we need something like that here.
> 

The original warning was     
  fs/afs/inode.c:448:9: sparse: incorrect type in initializer (different address spaces)
  fs/afs/inode.c:448:9:    expected struct afs_permits *__val
  fs/afs/inode.c:448:9:    got void [noderef] <asn:4>*<noident>
    

Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

[Arnd Bergmann]

On Wed, Feb 15, 2017 at 5:20 PM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> On 02/15/2017 05:18 PM, Christian Borntraeger wrote:
>> On 02/15/2017 12:03 AM, Arnd Bergmann wrote:
>>>
>>> -#define WRITE_ONCE(x, val) \
>>> -({                                                  \
>>> -    union { typeof(x) __val; char __c[1]; } __u =   \
>>> -            { .__val = (__force typeof(x)) (val) }; \
>>> -    __write_once_size(&(x), __u.__c, sizeof(x));    \
>>> -    __u.__val;                                      \
>>> -})
>>> +#define WRITE_ONCE(x, val)                                                          \
>>> +(                                                                                   \
>>> +    __builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x) = (val),     \
>>> +    __builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x) = (val),     \
>>> +    __builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x) = (val),     \
>>> +    __builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x) = (val),  \
>>
>> Have you run sparse on those changes?
>> IIRC we had to add the __force to get rid of address space annotations
>> in that macro above. Cannot tell if we need something like that here.

I haven't tried that yet, and I'm not overly worried about sparse, we can always
work around sparse using a different implementation of the macro
behind an #ifdef
if we can't easily fix that.

The hard part is coming up with a version that GCC won't turn into horrible
object code when building with ASAN, and that probably requires leaving out
the local variable.

I'm still struggling to understand what commit ab3f02fc237211 ("locking/arch:
Add WRITE_ONCE() to set_mb()") was for, and whether we should worry about
unaligned destinations trapping when we do a word-sized store to a location
that possibly has a smaller alignment.

> The original warning was
>   fs/afs/inode.c:448:9: sparse: incorrect type in initializer (different address spaces)
>   fs/afs/inode.c:448:9:    expected struct afs_permits *__val
>   fs/afs/inode.c:448:9:    got void [noderef] <asn:4>*<noident>

Is this just for the __rcu annotation in rcu_assign_pointer? WRITE_ONCE
should not care about that at all, but the caller should do it here, as this is
the place where a normal pointer becomes an __rcu pointer.

     Arnd