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

[Dmitry Vyukov]

On Wed, Feb 15, 2017 at 12:03 AM, Arnd Bergmann <arnd@xxxxxxxx> 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
>