[PATCH v2 0/2] Silence KASAN warnings in get_wchan()

[Andrey Ryabinin]

Originally I suggested to implement READ_ONCE_NOCHECK(), like this:
#define READ_ONCE_NOCHECK(x) \
{( typeof(x) __val; \
kasan_disable_local(); \
__val = READ_ONCE(x); \
kasan_enable_local(); \
__val;\
)}

But then I realised that we can't put this into linux/compiler.h
since it requires to add some includes (we need linux/kasan.h which
also requires linux/sched.h). It's not even that simple to put this into kernel.h

So I've come to another approach, see the first patch for details.
Pros:
- It generates more efficient code rather than variant above
Cons:
- REAS_ONCE() becomes rather messy.


Andrey Ryabinin (2):
Provide READ_ONCE_NOCHECK()
x86/process: Silence KASAN warnings in get_wchan()

arch/x86/kernel/process.c | 6 +++---
include/linux/compiler-gcc.h | 13 ++++++++++++
include/linux/compiler.h | 49 ++++++++++++++++++++++++++++++++------------
3 files changed, 52 insertions(+), 16 deletions(-)

--
2.4.9

[Andrey Ryabinin]

Some code may perform racy by design memory reads. This could be harmless,
yet such code may produce KASAN warnings.

To hide such accesses from KASAN this patch introduces READ_ONCE_NOCHECK()
macro. KASAN will not check the memory accessed by READ_ONCE_NOCHECK().

This patch creates __read_once_size_nocheck() a clone of
__read_once_size_check() (renamed __read_once_size()).
The only difference between them is 'no_sanitized_address' attribute
appended to '*_nocheck' function. This attribute tells compiler to not
instrumentation of memory accesses should not be applied to that function.
We declare it as static '__maybe_unsed' because GCC is not capable to
inline such function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368

With KASAN=n READ_ONCE_NOCHECK() is just a clone of READ_ONCE().

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>

---
include/linux/compiler-gcc.h | 13 ++++++++++++
include/linux/compiler.h | 49 ++++++++++++++++++++++++++++++++------------

2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index dfaa7b3..7ab09de 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -237,12 +237,25 @@
#define KASAN_ABI_VERSION 3
#endif

+#if GCC_VERSION >= 40902
+/*
+ * Tell the compiler that address safety instrumentation (e.g. KASAN)
+ * should not be applied to that function.
+ * Confilcts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
+ */
+#define __no_sanitize_address __attribute__((no_sanitize_address))
+#endif
+
#endif /* gcc version >= 40000 specific checks */

#if !defined(__noclone)
#define __noclone /* not needed */
#endif

+#if !defined(__no_sanitize_address)
+#define __no_sanitize_address
+#endif
+
/*
* A trick to suppress uninitialized variable warning without generating any
* code
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index c836eb2..2153fd8 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -198,20 +198,37 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);

#include <uapi/linux/types.h>

-static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
+#define __READ_ONCE_SIZE \
+({ \
+ switch (size) { \
+ case 1: *(__u8 *)res = *(volatile __u8 *)p; break; \
+ case 2: *(__u16 *)res = *(volatile __u16 *)p; break; \
+ case 4: *(__u32 *)res = *(volatile __u32 *)p; break; \
+ case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
+ default: \
+ barrier(); \
+ __builtin_memcpy((void *)res, (const void *)p, size); \
+ barrier(); \
+ } \
+})
+
+static __always_inline
+void __read_once_size_check(const volatile void *p, void *res, int size)
{
- switch (size) {
- case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
- case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
- case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
- case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
- default:
- barrier();
- __builtin_memcpy((void *)res, (const void *)p, size);
- barrier();
- }
+ __READ_ONCE_SIZE;
}

+#ifdef CONFIG_KASAN
+static __no_sanitize_address __maybe_unused
+void __read_once_size_nocheck(const volatile void *p, void *res, int size)
+{
+ __READ_ONCE_SIZE;
+}
+#else
+static __always_inline __alias(__read_once_size_check)
+void __read_once_size_nocheck(const volatile void *p, void *res, int size);
+#endif
+
static __always_inline void __write_once_size(volatile void *p, void *res, int size)
{
switch (size) {
@@ -248,8 +265,14 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
* required ordering.
*/

-#define READ_ONCE(x) \
- ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })
+#define __READ_ONCE(x, check) \
+({ \
+ union { typeof(x) __val; char __c[1]; } __u; \
+ __read_once_size##check(&(x), __u.__c, sizeof(x)); \
+ __u.__val; \
+})
+#define READ_ONCE(x) __READ_ONCE(x, _check)
+#define READ_ONCE_NOCHECK(x) __READ_ONCE(x, _nocheck)

#define WRITE_ONCE(x, val) \
({ \
--
2.4.9

[Andrey Ryabinin]

get_wchan() is racy by design, it may access volatile stack
of running task, thus it may access redzone in a stack frame
and cause KASAN to warn about this.

Use READ_ONCE_NOCHECK() to silence these warnings.

Reported-by: Sasha Levin <sasha...@oracle.com>

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
---

arch/x86/kernel/process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 39e585a..e28db18 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
if (sp < bottom || sp > top)
return 0;

- fp = READ_ONCE(*(unsigned long *)sp);
+ fp = READ_ONCE_NOCHECK(*(unsigned long *)sp);
do {
if (fp < bottom || fp > top)
return 0;
- ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
+ ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
if (!in_sched_functions(ip))
return ip;
- fp = READ_ONCE(*(unsigned long *)fp);
+ fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
} while (count++ < 16 && p->state != TASK_RUNNING);
return 0;
}
--
2.4.9

[Ingo Molnar]

Hm, exactly how is the 'red zone' defined? Is this about the current task mostly,
or when doing get_wchan() on other tasks?

Thanks,

Ingo

[Dmitry Vyukov]

When code is compiled with AddressSanitizer, most variables on stack
have redzones around them, on entry function "poisons" these redzones
(any accesses to them will be flagged), on exit function "unpoisons"
these redzones.

[Andrey Ryabinin]

We doing get_whcan() *only* on other tasks:

520: if (!p || p == current || p->state == TASK_RUNNING)
521: return 0;


Current wouldn't be a problem for KASAN.




> Thanks,
>
> Ingo
>

[Ingo Molnar]

* Andrey Ryabinin <arya...@virtuozzo.com> wrote:

> +#define __READ_ONCE_SIZE \
> +({ \
> + switch (size) { \
> + case 1: *(__u8 *)res = *(volatile __u8 *)p; break; \
> + case 2: *(__u16 *)res = *(volatile __u16 *)p; break; \
> + case 4: *(__u32 *)res = *(volatile __u32 *)p; break; \
> + case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
> + default: \
> + barrier(); \
> + __builtin_memcpy((void *)res, (const void *)p, size); \
> + barrier(); \
> + } \
> +})
> +
> +static __always_inline
> +void __read_once_size_check(const volatile void *p, void *res, int size)
> {

> + __READ_ONCE_SIZE;
> }

> +#ifdef CONFIG_KASAN
> +static __no_sanitize_address __maybe_unused
> +void __read_once_size_nocheck(const volatile void *p, void *res, int size)
> +{
> + __READ_ONCE_SIZE;
> +}
> +#else
> +static __always_inline __alias(__read_once_size_check)
> +void __read_once_size_nocheck(const volatile void *p, void *res, int size);
> +#endif

> +#define __READ_ONCE(x, check) \
> +({ \
> + union { typeof(x) __val; char __c[1]; } __u; \
> + __read_once_size##check(&(x), __u.__c, sizeof(x)); \
> + __u.__val; \
> +})
> +#define READ_ONCE(x) __READ_ONCE(x, _check)
> +#define READ_ONCE_NOCHECK(x) __READ_ONCE(x, _nocheck)

So all this code is quite convoluted, and as the changelog explains it's done to
work around a GCC inlining + no-kasan bug.

But please explain this in the code as well, in a comment, so future generations
are not kept wondering why these relatively simple wrappers are coded in such an
ugly and roundabout fashion.

Thanks,

Ingo

[Andrey Ryabinin]

An example bellow (stolen from slides - http://events.linuxfoundation.org/sites/events/files/slides/LinuxCon%20North%20America%202015%20KernelAddressSanitizer.pdf)

The following function:
void foo(void) {
char a[328];
...
a[i] = 0;
}

will be transform by GCC to something like this:

void foo(void) {
char redzone1[32];
char a[328];
char redzone2[24];
char redzone3[32];

int *shadow = (&redzone1 >> 3) + shadow_offset;
shadow[0] = 0xf1f1f1f1; // poison redzone1
shadow[11] = 0xf4f4f400; // poison redzone2
shadow[12] = 0xf3f3f3f3; // poison redzone3

...
__asan_store1(&a[i]); //check access to a[i]
a[i] = 0;

shadow[0] = shadow[11] = shadow[12] = 0; //unpoison redzones.
}

[Ingo Molnar]

* Dmitry Vyukov <dvy...@google.com> wrote:

> > Hm, exactly how is the 'red zone' defined? Is this about the current task
> > mostly, or when doing get_wchan() on other tasks?
>
> When code is compiled with AddressSanitizer, most variables on stack have
> redzones around them, on entry function "poisons" these redzones (any accesses
> to them will be flagged), on exit function "unpoisons" these redzones.

I see, fair enough!

This series looks good to me, modulo the small documentation nit I had about the
first patch.

Thanks,

Ingo

[Andrey Ryabinin]

Changes since v2:
- Added some code comments in the first patch.

Andrey Ryabinin (2):
Provide READ_ONCE_NOCHECK()
x86/process: Silence KASAN warnings in get_wchan()

arch/x86/kernel/process.c | 6 ++---
include/linux/compiler-gcc.h | 13 ++++++++++
include/linux/compiler.h | 60 ++++++++++++++++++++++++++++++++++----------
3 files changed, 63 insertions(+), 16 deletions(-)

--
2.4.9

[Andrey Ryabinin]

Some code may perform racy by design memory reads. This could be harmless,
yet such code may produce KASAN warnings.

To hide such accesses from KASAN this patch introduces READ_ONCE_NOCHECK()
macro. KASAN will not check the memory accessed by READ_ONCE_NOCHECK().

This patch creates __read_once_size_nocheck() a clone of
__read_once_size_check() (renamed __read_once_size()).
The only difference between them is 'no_sanitized_address' attribute

appended to '*_nocheck' function. This attribute tells the compiler that

instrumentation of memory accesses should not be applied to that function.
We declare it as static '__maybe_unsed' because GCC is not capable to
inline such function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368

With KASAN=n READ_ONCE_NOCHECK() is just a clone of READ_ONCE().

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
---

include/linux/compiler-gcc.h | 13 ++++++++++
include/linux/compiler.h | 60 ++++++++++++++++++++++++++++++++++----------

2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index dfaa7b3..f2a9aec 100644

--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -237,12 +237,25 @@
#define KASAN_ABI_VERSION 3
#endif

+#if GCC_VERSION >= 40902
+/*

+ * Tell the compiler that address safety instrumentation (KASAN)

+ * should not be applied to that function.
+ * Confilcts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
+ */
+#define __no_sanitize_address __attribute__((no_sanitize_address))
+#endif
+
#endif /* gcc version >= 40000 specific checks */

#if !defined(__noclone)
#define __noclone /* not needed */
#endif

+#if !defined(__no_sanitize_address)
+#define __no_sanitize_address
+#endif
+
/*
* A trick to suppress uninitialized variable warning without generating any
* code
diff --git a/include/linux/compiler.h b/include/linux/compiler.h

index c836eb2..aa2ae4c 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -198,19 +198,42 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);

#include <uapi/linux/types.h>

-static __always_inline void __read_once_size(const volatile void *p, void *res, int size)

+#define __READ_ONCE_SIZE \
+({ \
+ switch (size) { \
+ case 1: *(__u8 *)res = *(volatile __u8 *)p; break; \
+ case 2: *(__u16 *)res = *(volatile __u16 *)p; break; \
+ case 4: *(__u32 *)res = *(volatile __u32 *)p; break; \
+ case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
+ default: \
+ barrier(); \
+ __builtin_memcpy((void *)res, (const void *)p, size); \
+ barrier(); \
+ } \
+})
+
+static __always_inline
+void __read_once_size_check(const volatile void *p, void *res, int size)
{

- switch (size) {
- case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
- case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
- case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
- case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
- default:
- barrier();
- __builtin_memcpy((void *)res, (const void *)p, size);
- barrier();
- }
+ __READ_ONCE_SIZE;
+}

+
+#ifdef CONFIG_KASAN
+/*
+ * This function is not 'inline' because __no_sanitize_address confilcts
+ * with inlining. Attempt to inline it may cause a build failure.
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
+ * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
+ */

+static __no_sanitize_address __maybe_unused
+void __read_once_size_nocheck(const volatile void *p, void *res, int size)
+{
+ __READ_ONCE_SIZE;
}

+#else
+static __always_inline __alias(__read_once_size_check)
+void __read_once_size_nocheck(const volatile void *p, void *res, int size);
+#endif

static __always_inline void __write_once_size(volatile void *p, void *res, int size)
{
@@ -248,8 +271,19 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s

* required ordering.
*/

-#define READ_ONCE(x) \
- ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })

+#define __READ_ONCE(x, check) \
+({ \
+ union { typeof(x) __val; char __c[1]; } __u; \
+ __read_once_size##check(&(x), __u.__c, sizeof(x)); \
+ __u.__val; \
+})
+#define READ_ONCE(x) __READ_ONCE(x, _check)
+

+/*
+ * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need
+ * to hide memory access from KASAN.
+ */

[Andrey Ryabinin]

get_wchan() is racy by design, it may access volatile stack
of running task, thus it may access redzone in a stack frame
and cause KASAN to warn about this.

Use READ_ONCE_NOCHECK() to silence these warnings.

Reported-by: Sasha Levin <sasha...@oracle.com>

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
---

arch/x86/kernel/process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 39e585a..e28db18 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
if (sp < bottom || sp > top)
return 0;

- fp = READ_ONCE(*(unsigned long *)sp);
+ fp = READ_ONCE_NOCHECK(*(unsigned long *)sp);
do {
if (fp < bottom || fp > top)
return 0;
- ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
+ ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
if (!in_sched_functions(ip))
return ip;
- fp = READ_ONCE(*(unsigned long *)fp);
+ fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
} while (count++ < 16 && p->state != TASK_RUNNING);
return 0;
}

--
2.4.9

[kbuild test robot]

Hi Andrey,

[auto build test WARNING on tip/auto-latest -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url: https://github.com/0day-ci/linux/commits/Andrey-Ryabinin/Provide-READ_ONCE_NOCHECK/20151013-204127
config: mn10300-allyesconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mn10300

All warnings (new ones prefixed by >>):

fs/nfs/filelayout/filelayout.c: In function 'filelayout_read_pagelist':
fs/nfs/filelayout/filelayout.c:476:2: warning: format '%Zu' expects argument of type 'size_t', but argument 5 has type '__u32' [-Wformat=]
dprintk("--> %s ino %lu pgbase %u req %Zu@%llu\n",
^
In file included from include/linux/nfs_fs.h:28:0,
from fs/nfs/filelayout/filelayout.c:32:
fs/nfs/filelayout/filelayout.c: In function 'filelayout_write_pagelist':
include/linux/compiler.h:270:8: warning: format '%Zu' expects argument of type 'size_t', but argument 5 has type '__u32' [-Wformat=]
union { typeof(x) __val; char __c[1]; } __u; \
^
include/linux/sunrpc/debug.h:33:24: note: in definition of macro 'dfprintk'
printk(KERN_DEFAULT args); \
^
>> include/linux/compiler.h:274:22: note: in expansion of macro '__READ_ONCE'
#define READ_ONCE(x) __READ_ONCE(x, _check)
^
include/asm-generic/atomic.h:130:24: note: in expansion of macro 'READ_ONCE'
#define atomic_read(v) READ_ONCE((v)->counter)
^
include/linux/sunrpc/debug.h:23:48: note: in expansion of macro 'atomic_read'
#define dprintk(args...) dfprintk(FACILITY, ## args)
^
fs/nfs/filelayout/filelayout.c:534:2: note: in expansion of macro 'dprintk'
dprintk("%s ino %lu sync %d req %Zu@%llu DS: %s cl_count %d\n",
^

vim +/__READ_ONCE +274 include/linux/compiler.h

264 * with an explicit memory barrier or atomic instruction that provides the
265 * required ordering.
266 */
267
268 #define __READ_ONCE(x, check) \
269 ({ \
> 270 union { typeof(x) __val; char __c[1]; } __u; \
271 __read_once_size##check(&(x), __u.__c, sizeof(x)); \
272 __u.__val; \
273 })
> 274 #define READ_ONCE(x) __READ_ONCE(x, _check)
275 #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, _nocheck)
276
277 #define WRITE_ONCE(x, val) \

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

[Andrey Ryabinin]

On 10/13/2015 07:02 PM, kbuild test robot wrote:
> Hi Andrey,
>
> [auto build test WARNING on tip/auto-latest -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
>
> url: https://github.com/0day-ci/linux/commits/Andrey-Ryabinin/Provide-READ_ONCE_NOCHECK/20151013-204127
> config: mn10300-allyesconfig (attached as .config)
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=mn10300
>
> All warnings (new ones prefixed by >>):
>
> fs/nfs/filelayout/filelayout.c: In function 'filelayout_read_pagelist':
> fs/nfs/filelayout/filelayout.c:476:2: warning: format '%Zu' expects argument of type 'size_t', but argument 5 has type '__u32' [-Wformat=]
> dprintk("--> %s ino %lu pgbase %u req %Zu@%llu\n",
> ^

So the actual warning is here. And it's not new.

> In file included from include/linux/nfs_fs.h:28:0,
> from fs/nfs/filelayout/filelayout.c:32:
> fs/nfs/filelayout/filelayout.c: In function 'filelayout_write_pagelist':
> include/linux/compiler.h:270:8: warning: format '%Zu' expects argument of type 'size_t', but argument 5 has type '__u32' [-Wformat=]
> union { typeof(x) __val; char __c[1]; } __u; \
> ^
> include/linux/sunrpc/debug.h:33:24: note: in definition of macro 'dfprintk'
> printk(KERN_DEFAULT args); \
> ^
>>> include/linux/compiler.h:274:22: note: in expansion of macro '__READ_ONCE'
> #define READ_ONCE(x) __READ_ONCE(x, _check)
> ^

And this 'note: in expansion of macro' belongs to the warning above.
So it's a false-positive.

[Ingo Molnar]

* Andrey Ryabinin <arya...@virtuozzo.com> wrote:

>
>

So if this is a new warning introduced by these patches then the warning needs to
be addresses - regardless of whether it's a false positive or not.

Thanks,

Ingo

[Andrey Ryabinin]

It's not a new warning. It's a new 'note: in expansion of macro' message which is
belongs to the old warning from above.
I changed the macros, so obviously it caused change in macro expansion backtrace.
The actual warning existed before this patch.

> - regardless of whether it's a false positive or not.

I wasn't very clear. By false positive I meant that it is kbuild robot's false positive.
The warning itself is not a false positive, but it's an old warning, it's not introduced by this patch.

It seems that kbuild robot treats 'note: in expansion of macro' message as a new warning,
but such messages are not warnings by itself. I think, that kbuild robot should just ignore
new 'note: in expansion of macro' messages unless these messages preceded by a new 'warning:' message.


> Thanks,
>
> Ingo
>

[tip-bot for Andrey Ryabinin]

Commit-ID: 4115ffdf4d6f8986a7abe1dd522c163f599bc0e6
Gitweb: http://git.kernel.org/tip/4115ffdf4d6f8986a7abe1dd522c163f599bc0e6
Author: Andrey Ryabinin <arya...@virtuozzo.com>
AuthorDate: Tue, 13 Oct 2015 18:28:07 +0300
Committer: Ingo Molnar <mi...@kernel.org>
CommitDate: Wed, 14 Oct 2015 16:44:06 +0200

compiler, atomics: Provide READ_ONCE_NOCHECK()

Some code may perform racy by design memory reads. This could be
harmless, yet such code may produce KASAN warnings.

To hide such accesses from KASAN this patch introduces
READ_ONCE_NOCHECK() macro. KASAN will not check the memory
accessed by READ_ONCE_NOCHECK().

This patch creates __read_once_size_nocheck() a clone of
__read_once_size_check() (renamed __read_once_size()).
The only difference between them is 'no_sanitized_address'
attribute appended to '*_nocheck' function. This attribute tells
the compiler that instrumentation of memory accesses should not
be applied to that function. We declare it as static
'__maybe_unsed' because GCC is not capable to inline such
function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368

With KASAN=n READ_ONCE_NOCHECK() is just a clone of READ_ONCE().

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>

Cc: Alexander Potapenko <gli...@google.com>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Andrey Konovalov <andre...@google.com>
Cc: Andy Lutomirski <lu...@amacapital.net>
Cc: Borislav Petkov <b...@alien8.de>
Cc: Denys Vlasenko <dvla...@redhat.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Kostya Serebryany <k...@google.com>
Cc: Linus Torvalds <torv...@linux-foundation.org>
Cc: Paul E. McKenney <pau...@linux.vnet.ibm.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Sasha Levin <sasha...@oracle.com>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: Wolfram Gloger <wm...@dent.med.uni-muenchen.de>
Cc: kasan-dev <kasa...@googlegroups.com>
Link: http://lkml.kernel.org/r/1444750088-24444-2-gi...@virtuozzo.com
Signed-off-by: Ingo Molnar <mi...@kernel.org>

[tip-bot for Andrey Ryabinin]

Commit-ID: 1e6f85cb6771d54c9346270d7b0667500d9f3eab
Gitweb: http://git.kernel.org/tip/1e6f85cb6771d54c9346270d7b0667500d9f3eab
Author: Andrey Ryabinin <arya...@virtuozzo.com>
AuthorDate: Tue, 13 Oct 2015 18:28:08 +0300

Committer: Ingo Molnar <mi...@kernel.org>
CommitDate: Wed, 14 Oct 2015 16:44:06 +0200

x86/mm: Silence KASAN warnings in get_wchan()

get_wchan() is racy by design, it may access volatile stack
of running task, thus it may access redzone in a stack frame
and cause KASAN to warn about this.

Use READ_ONCE_NOCHECK() to silence these warnings.

Reported-by: Sasha Levin <sasha...@oracle.com>
Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>

Cc: Alexander Potapenko <gli...@google.com>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Andrey Konovalov <andre...@google.com>
Cc: Andy Lutomirski <lu...@amacapital.net>
Cc: Borislav Petkov <b...@alien8.de>
Cc: Denys Vlasenko <dvla...@redhat.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Kostya Serebryany <k...@google.com>
Cc: Linus Torvalds <torv...@linux-foundation.org>
Cc: Paul E. McKenney <pau...@linux.vnet.ibm.com>
Cc: Peter Zijlstra <pet...@infradead.org>

Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: Wolfram Gloger <wm...@dent.med.uni-muenchen.de>
Cc: kasan-dev <kasa...@googlegroups.com>

Link: http://lkml.kernel.org/r/1444750088-24444-3-gi...@virtuozzo.com
Signed-off-by: Ingo Molnar <mi...@kernel.org>

[Paul E. McKenney]

On Wed, Oct 14, 2015 at 08:28:43AM -0700, tip-bot for Andrey Ryabinin wrote:
> Commit-ID: 4115ffdf4d6f8986a7abe1dd522c163f599bc0e6
> Gitweb: http://git.kernel.org/tip/4115ffdf4d6f8986a7abe1dd522c163f599bc0e6
> Author: Andrey Ryabinin <arya...@virtuozzo.com>
> AuthorDate: Tue, 13 Oct 2015 18:28:07 +0300
> Committer: Ingo Molnar <mi...@kernel.org>
> CommitDate: Wed, 14 Oct 2015 16:44:06 +0200
>
> compiler, atomics: Provide READ_ONCE_NOCHECK()
>
> Some code may perform racy by design memory reads. This could be
> harmless, yet such code may produce KASAN warnings.
>
> To hide such accesses from KASAN this patch introduces
> READ_ONCE_NOCHECK() macro. KASAN will not check the memory
> accessed by READ_ONCE_NOCHECK().
>
> This patch creates __read_once_size_nocheck() a clone of
> __read_once_size_check() (renamed __read_once_size()).
> The only difference between them is 'no_sanitized_address'
> attribute appended to '*_nocheck' function. This attribute tells
> the compiler that instrumentation of memory accesses should not
> be applied to that function. We declare it as static
> '__maybe_unsed' because GCC is not capable to inline such
> function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
>
> With KASAN=n READ_ONCE_NOCHECK() is just a clone of READ_ONCE().

So I add READ_ONCE_NOCHECK() for accesses for which the compiler cannot
prove safe address for KASAN's benefit, but READ_ONCE() suffices for
the data-race-detection logic in KTSAN, correct?

Thanx, Paul

[Dmitry Vyukov]

KTSAN also needs READ_ONCE_NOCHECK() here. KTSAN will flag races
between get_wchan() and the thread accesses to own stack even more
aggressively than KASAN, because KTSAN won't like get_wchan() accesses
even to non-poisoned areas of other thread stack.

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To post to this group, send email to kasa...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20151014154532.GV3910%40linux.vnet.ibm.com.
> For more options, visit https://groups.google.com/d/optout.

[Paul E. McKenney]

So to keep KTSAN happy, any read from some other thread's stack requires
READ_ONCE_NOCHECK()? What if the access is via a locking primitive or
read-modify-write atomic operation?

This is of some interest in RCU, which implements synchronous grace
periods using completions that are allocated on the calling task's stack
and manipulated by RCU callbacks that are likely executing elsewhere.

[Dmitry Vyukov]

On Wed, Oct 14, 2015 at 6:01 PM, Paul E. McKenney

KTSAN does not have any special logic for stacks. It just generally
flags pairs of accesses when (1) at least one access is not atomic,
(2) at least one access is a write and (3) these accesses are not
synchronized by means of other synchronization.
There is a bunch of cases when kernel code allocates objects on stack
and then passes them to other threads, but as far as there is proper
synchronization it is OK.

For the record, KTSAN is this:
https://github.com/google/ktsan
https://github.com/google/ktsan/wiki/Found-Bugs

[Peter Zijlstra]

On Wed, Oct 14, 2015 at 06:08:16PM +0200, Dmitry Vyukov wrote:
> >> > So I add READ_ONCE_NOCHECK() for accesses for which the compiler cannot
> >> > prove safe address for KASAN's benefit, but READ_ONCE() suffices for
> >> > the data-race-detection logic in KTSAN, correct?
> >>
> >> KTSAN also needs READ_ONCE_NOCHECK() here. KTSAN will flag races
> >> between get_wchan() and the thread accesses to own stack even more
> >> aggressively than KASAN, because KTSAN won't like get_wchan() accesses
> >> even to non-poisoned areas of other thread stack.
> >
> > So to keep KTSAN happy, any read from some other thread's stack requires
> > READ_ONCE_NOCHECK()? What if the access is via a locking primitive or
> > read-modify-write atomic operation?
> >
> > This is of some interest in RCU, which implements synchronous grace
> > periods using completions that are allocated on the calling task's stack
> > and manipulated by RCU callbacks that are likely executing elsewhere.
>
>
> KTSAN does not have any special logic for stacks. It just generally
> flags pairs of accesses when (1) at least one access is not atomic,
> (2) at least one access is a write and (3) these accesses are not
> synchronized by means of other synchronization.

But but but.. WRITE_ONCE/READ_ONCE _are_ atomic when on naturally
aligned machine word sized thingies. We very much rely on that.

And the wchan thing is very much that, its not some weird large object,
its a single word, read with an explicit 'volatile' cast.

This is good, and should not require more magic annotations.

[Dmitry Vyukov]

Well, if another thread writes it byte-by-byte, it pretty much does
not matter how you read it.
Note that I said "at least one access is not atomic". If both are
atomic, then this is, of course, legal. And KTSAN considers
READ/WRITE_ONCE as atomic operations.

[Andrey Ryabinin]

Does it? What's the difference between READ_ONCE_NOCHECK() and READ_ONCE() with KTSAN=y?
AFAIK READ_ONCE() is sufficient to hide race from KTSAN. It doesn't *require* READ_ONCE_NOCHECK(), right?

[Paul E. McKenney]

OK, so let me see if I understand this. ;-)

KASAN requires READ_ONCE_NOCHECK() for get_wchan(). KTSAN would be
just as happy with READ_ONCE(), but READ_ONCE_NOCHECK() works for
both.

Did I get it right?

Thanx, Paul

[Peter Zijlstra]

On Wed, Oct 14, 2015 at 06:18:58PM +0200, Dmitry Vyukov wrote:
>
> Well, if another thread writes it byte-by-byte, it pretty much does
> not matter how you read it.
> Note that I said "at least one access is not atomic". If both are
> atomic, then this is, of course, legal. And KTSAN considers
> READ/WRITE_ONCE as atomic operations.

OK, then I'm confused on what exactly the annotation does, but less
worried.

[Andy Lutomirski]

The annotation says "hey, KASAN (etc), don't worry if you think that
the memory being accessed is out of bounds". Presumably KTSAN is okay
with the operation because it's atomic, but KASAN dislikes it because
it's accessing memory that is out of bounds from the perspective of a
C program.

I'd still rather find a way to just delete get_wchan, but whatever.

--Andy

[Dmitry Vyukov]

For not there is no difference, because you just added
READ_ONCE_NOCHECK and we have not yet supported it.
But my plan is to completely ignore accessed from READ_ONCE_NOCHECK in
KTSAN so that they never lead to race reports.

READ_ONCE in get_wchan still can lead to a data race report, because
it is READ_ONCE in get_wchan versus a normal write to stack in the
other thread. That is not atomic and not generally safe.

[Dmitry Vyukov]

On Wed, Oct 14, 2015 at 6:20 PM, Paul E. McKenney

No, KTSAN also needs READ_ONCE_NOCHECK.
READ_ONCE in get_wchan can lead to a data race report.
Consider:

// the other thead
some_stack_var = ...;

// get_wchan
bp = READ_ONCE(p); // where p happens to point to some_stack_var in
the other thread

This is generally not atomic and not safe. And this is a data race by
all possible definitions.
Only READ_ONCE on reading side is not enough to ensure atomicity, also
all concurrent writes must be done with atomic operations.

[Dmitry Vyukov]

The plan is to make READ_ONCE_NOCHECK ignored by KTSAN, just it is
ignored by KASAN. So that it never leads to a report ("not checked").

[Peter Zijlstra]

There's going to be more of that..

> I'd still rather find a way to just delete get_wchan, but whatever.

:-)

[Peter Zijlstra]

Would a _NOKSAN suffix not be more appropriate? NOCHECK seems somewhat
generic.

[Paul E. McKenney]

OK. However, this is specific to get_wchan()'s out-of-bounds stack, right?
If I have multiple tasks accessing some other task's on-stack variable,
then as long as all other potentially concurrent accesses use atomic
operations, READ_ONCE() suffices. Or am I still missing something here?

Thanx, Paul

[Paul E. McKenney]

Where possible, it would be better to make the normal write instead
be WRITE_ONCE(). That might well not be possible here, but let's not
be too aggressive about silencing KTSAN.

Thanx, Paul

[Dmitry Vyukov]

On Wed, Oct 14, 2015 at 7:04 PM, Paul E. McKenney

Yes... and no.
This is specific to racy accesses, not necessary to out-of-bounds
stack. If you have a data race in-bounds, it is also not OK :)

> If I have multiple tasks accessing some other task's on-stack variable,
> then as long as all other potentially concurrent accesses use atomic
> operations, READ_ONCE() suffices. Or am I still missing something here?

This is correct.
Generally, the idea is that KTSAN flags what you think is a bug, and
does not flag what you think is not a bug. If you have all proper
synchronization in place, then KTSAN should not flag that; whether the
object is on stack or not is irrelevant.

[Paul E. McKenney]

But a non-data-race out of bounds would presumably trigger KASAN but not
KTSAN.

> > If I have multiple tasks accessing some other task's on-stack variable,
> > then as long as all other potentially concurrent accesses use atomic
> > operations, READ_ONCE() suffices. Or am I still missing something here?
>
> This is correct.
> Generally, the idea is that KTSAN flags what you think is a bug, and
> does not flag what you think is not a bug. If you have all proper
> synchronization in place, then KTSAN should not flag that; whether the
> object is on stack or not is irrelevant.

Good!

Thanx, Paul

[Ingo Molnar]

* Peter Zijlstra <pet...@infradead.org> wrote:

> > I'd still rather find a way to just delete get_wchan, but whatever.
>
> :-)

AFAICS can only do that at the price of slowing down various scheduler functions
by saving the caller address.

Thanks,

Ingo

[Andy Lutomirski]

A quick check on my machine:

# for i in /proc/*/wchan; do cat $i && echo ''; done |sort |uniq

doesn't have very much of interest to say:

0
devtmpfsd
dmcrypt_write
do_sigtimedwait
do_wait
ep_poll
fsnotify_mark_destroy
futex_wait_queue_me
hrtimer_nanosleep
irq_thread
kauditd_thread
khugepaged
kjournald2
ksm_scan_thread
kswapd
kthreadd
pipe_wait
poll_schedule_timeout
rcu_gp_kthread
rcu_nocb_kthread
rescuer_thread
scsi_error_handler
sk_wait_data
smpboot_thread_fn
unix_stream_recvmsg
wait_woken
worker_thread
xfsaild

A bunch of those look like they're specific to kernel threads, for
which this whole mechanism is probably pointless -- just reading
/proc/PID/stack (with suitable privilege) is probably better.

For the rest, a few are useful, but I find myself wondering whether
this mechanism is really useful enough to be worth keeping. We could
just return "asleep" in /proc/PID/wchan.

A more interesting thing to do might be to try to decode regs->orig_ax
to give a guess as to which syscall is asleep. But this seems like a
lot of fiddling and a lot of worry about security issues for a
mechanism of dubious value. Also:

$ cat /proc/1/wchan
ep_poll

Why do we allow unprivileged queries like that at all?

--Andy

[Stephen Rothwell]

Hi Andrey,

On Tue, 13 Oct 2015 18:28:07 +0300 Andrey Ryabinin <arya...@virtuozzo.com> wrote:
>
> Some code may perform racy by design memory reads. This could be harmless,
> yet such code may produce KASAN warnings.
>
> To hide such accesses from KASAN this patch introduces READ_ONCE_NOCHECK()
> macro. KASAN will not check the memory accessed by READ_ONCE_NOCHECK().
>
> This patch creates __read_once_size_nocheck() a clone of
> __read_once_size_check() (renamed __read_once_size()).
> The only difference between them is 'no_sanitized_address' attribute
> appended to '*_nocheck' function. This attribute tells the compiler that
> instrumentation of memory accesses should not be applied to that function.
> We declare it as static '__maybe_unsed' because GCC is not capable to
> inline such function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
>
> With KASAN=n READ_ONCE_NOCHECK() is just a clone of READ_ONCE().
>

> Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>

> ---
> include/linux/compiler-gcc.h | 13 ++++++++++
> include/linux/compiler.h | 60 ++++++++++++++++++++++++++++++++++----------
> 2 files changed, 60 insertions(+), 13 deletions(-)

I am pretty sure that this patch is causing quite a bit of compile
breakage in linux-next today. During the day I compile with gcc 4.9.0
and did not see any problems with c86_64 allmodconfig, or i386
defconfig etc, but overnight we compile with older compilers (gcc 4.6.3
in particular) and are getting quite a few errors:

From an i386 allnoconfig build:

arch/x86/entry/vdso/vdso32.so.dbg: undefined symbols found
/home/kisskb/slave/src/arch/x86/entry/vdso/Makefile:154: recipe for target 'arch/x86/entry/vdso/vdso32.so.dbg' failed

From an x86_64 allnoconfig build:

arch/x86/entry/vdso/vclock_gettime.o: In function `__read_once_size_check':
vclock_gettime.c:(.text+0x5f): undefined reference to `memcpy'
arch/x86/entry/vdso/vgetcpu.o: In function `__read_once_size_check':
vgetcpu.c:(.text+0x2f): undefined reference to `memcpy'

and several others ...
--
Cheers,
Stephen Rothwell s...@canb.auug.org.au

[Andrey Ryabinin]

Looks like that older GCC doesn't like __alias (or combination of static __always_inline __alias).
It creates outline and unused copy of __read_once_size_check() function in the object file.
Should be easy to work around this.

[Andrey Ryabinin]

Some old versions of GCC doesn't handle __alias (or maybe a combination
of 'static __always_inline __alias') right.

GCC creates outline and unused copy of __read_once_size_check()
function in the object file which references memcpy and causes
the build failure:

arch/x86/entry/vdso/vclock_gettime.o: In function `__read_once_size_check':
vclock_gettime.c:(.text+0x5f): undefined reference to `memcpy'
arch/x86/entry/vdso/vgetcpu.o: In function `__read_once_size_check':
vgetcpu.c:(.text+0x2f): undefined reference to `memcpy'

We could avoid using alias to work around this problem.

Reported-by: Stephen Rothwell <s...@canb.auug.org.au>

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
---

include/linux/compiler.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index aa2ae4c..3436a4c 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -231,8 +231,11 @@ void __read_once_size_nocheck(const volatile void *p, void *res, int size)
__READ_ONCE_SIZE;
}
#else
-static __always_inline __alias(__read_once_size_check)
-void __read_once_size_nocheck(const volatile void *p, void *res, int size);
+static __always_inline

+void __read_once_size_nocheck(const volatile void *p, void *res, int size)
+{
+ __READ_ONCE_SIZE;
+}

#endif

static __always_inline void __write_once_size(volatile void *p, void *res, int size)

--
2.4.9

[Ingo Molnar]

Yeah, so could you please re-send the original two patches, with all the changes
(typo fixes, renaming, this build fix, etc.) propagated that were found in testing
and that were suggested in the review thread?

I'll remove the broken commits from linux-next meanwhile.

Thanks,

Ingo

[Andrey Ryabinin]

get_wchan() is racy by design, it may access volatile stack
of running task, thus it may access redzone in a stack frame
and cause KASAN to warn about this.

Use READ_ONCE_NOKSAN() to silence these warnings.

Reported-by: Sasha Levin <sasha...@oracle.com>

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
---

arch/x86/kernel/process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 39e585a..971cf5f 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
if (sp < bottom || sp > top)
return 0;

- fp = READ_ONCE(*(unsigned long *)sp);
+ fp = READ_ONCE_NOKSAN(*(unsigned long *)sp);
do {
if (fp < bottom || fp > top)
return 0;
- ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
+ ip = READ_ONCE_NOKSAN(*(unsigned long *)(fp + sizeof(unsigned long)));
if (!in_sched_functions(ip))
return ip;
- fp = READ_ONCE(*(unsigned long *)fp);
+ fp = READ_ONCE_NOKSAN(*(unsigned long *)fp);
} while (count++ < 16 && p->state != TASK_RUNNING);
return 0;
}
--
2.4.9

[Andrey Ryabinin]

Changes since v3:
- Fixed build failure.
- Rename: s/READ_ONCE_NOCHECK/READ_ONCE_NOKSAN
- Spelling fix.

Changes since v2:
- Added some code comments in the first patch.

Andrey Ryabinin (2):
compiler, atomics: Provide READ_ONCE_NOKSAN()
x86/mm: Silence KASAN warnings in get_wchan()

arch/x86/kernel/process.c | 6 ++--
include/linux/compiler-gcc.h | 13 +++++++++
include/linux/compiler.h | 66 +++++++++++++++++++++++++++++++++++---------
3 files changed, 69 insertions(+), 16 deletions(-)

--
2.4.9

[Andrey Ryabinin]

Some code may perform racy by design memory reads. This could be
harmless, yet such code may produce KASAN warnings.

To hide such accesses from KASAN this patch introduces

READ_ONCE_NOKSAN() macro. KASAN will not check the memory
accessed by READ_ONCE_NOKSAN(). The KernelThreadSanitizer (KTSAN)
is going to ignore it as well.

This patch creates __read_once_size_noksan() a clone of
__read_once_size(). The only difference between them is
'no_sanitized_address' attribute appended to '*_nokasan' function.

This attribute tells the compiler that instrumentation of memory
accesses should not be applied to that function. We declare it as
static '__maybe_unsed' because GCC is not capable to inline such
function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368

With KASAN=n READ_ONCE_NOKSAN() is just a clone of READ_ONCE().

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
---

include/linux/compiler-gcc.h | 13 +++++++++
include/linux/compiler.h | 66 +++++++++++++++++++++++++++++++++++---------

2 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index dfaa7b3..8efb40e 100644

--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -237,12 +237,25 @@
#define KASAN_ABI_VERSION 3
#endif

+#if GCC_VERSION >= 40902
+/*
+ * Tell the compiler that address safety instrumentation (KASAN)

+ * should not be applied to that function.
+ * Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368

+ */
+#define __no_sanitize_address __attribute__((no_sanitize_address))
+#endif
+
#endif /* gcc version >= 40000 specific checks */

#if !defined(__noclone)
#define __noclone /* not needed */
#endif

+#if !defined(__no_sanitize_address)
+#define __no_sanitize_address
+#endif
+
/*
* A trick to suppress uninitialized variable warning without generating any
* code

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index c836eb2..dfa172f7 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -198,19 +198,45 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);

#include <uapi/linux/types.h>

-static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
+#define __READ_ONCE_SIZE \
+({ \
+ switch (size) { \
+ case 1: *(__u8 *)res = *(volatile __u8 *)p; break; \
+ case 2: *(__u16 *)res = *(volatile __u16 *)p; break; \
+ case 4: *(__u32 *)res = *(volatile __u32 *)p; break; \
+ case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \
+ default: \
+ barrier(); \
+ __builtin_memcpy((void *)res, (const void *)p, size); \
+ barrier(); \
+ } \
+})
+
+static __always_inline

+void __read_once_size(const volatile void *p, void *res, int size)

{
- switch (size) {
- case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
- case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
- case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
- case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
- default:
- barrier();
- __builtin_memcpy((void *)res, (const void *)p, size);
- barrier();
- }
+ __READ_ONCE_SIZE;
+}
+
+#ifdef CONFIG_KASAN
+/*
+ * This function is not 'inline' because __no_sanitize_address confilcts
+ * with inlining. Attempt to inline it may cause a build failure.
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
+ * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
+ */
+static __no_sanitize_address __maybe_unused

+void __read_once_size_noksan(const volatile void *p, void *res, int size)
+{
+ __READ_ONCE_SIZE;
+}
+#else
+static __always_inline
+void __read_once_size_noksan(const volatile void *p, void *res, int size)
+{
+ __READ_ONCE_SIZE;
}
+#endif

static __always_inline void __write_once_size(volatile void *p, void *res, int size)
{
@@ -248,8 +274,22 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s

* required ordering.
*/

-#define READ_ONCE(x) \
- ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })

+#define __READ_ONCE(x, noksan) \

+({ \
+ union { typeof(x) __val; char __c[1]; } __u; \

+ if (noksan) \
+ __read_once_size_noksan(&(x), __u.__c, sizeof(x)); \
+ else \
+ __read_once_size(&(x), __u.__c, sizeof(x)); \
+ __u.__val; \
+})
+#define READ_ONCE(x) __READ_ONCE(x, 0)
+
+/*
+ * Use READ_ONCE_NOKSAN() instead of READ_ONCE() if you need

+ * to hide memory access from KASAN.
+ */

+#define READ_ONCE_NOKSAN(x) __READ_ONCE(x, 1)

#define WRITE_ONCE(x, val) \
({ \
--

2.4.9

[Andrey Ryabinin]

[Peter Zijlstra]

On Fri, Oct 16, 2015 at 12:44:53PM +0300, Andrey Ryabinin wrote:
> Some code may perform racy by design memory reads. This could be
> harmless, yet such code may produce KASAN warnings.
>
> To hide such accesses from KASAN this patch introduces
> READ_ONCE_NOKSAN() macro. KASAN will not check the memory
> accessed by READ_ONCE_NOKSAN(). The KernelThreadSanitizer (KTSAN)
> is going to ignore it as well.
>
> This patch creates __read_once_size_noksan() a clone of
> __read_once_size(). The only difference between them is
> 'no_sanitized_address' attribute appended to '*_nokasan' function.
> This attribute tells the compiler that instrumentation of memory
> accesses should not be applied to that function. We declare it as
> static '__maybe_unsed' because GCC is not capable to inline such
> function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
>
> With KASAN=n READ_ONCE_NOKSAN() is just a clone of READ_ONCE().

Would we need a similar annotation for things like
mutex_spin_on_owner()'s dereference of owner, or is that considered safe
by KASAN?

(its not actually safe; as I remember we have a problem with using
rcu_read_lock for tasks like that)

[Borislav Petkov]

On Fri, Oct 16, 2015 at 12:44:53PM +0300, Andrey Ryabinin wrote:

> Some code may perform racy by design memory reads. This could be
> harmless, yet such code may produce KASAN warnings.
>
> To hide such accesses from KASAN this patch introduces
> READ_ONCE_NOKSAN() macro. KASAN will not check the memory
> accessed by READ_ONCE_NOKSAN(). The KernelThreadSanitizer (KTSAN)
> is going to ignore it as well.

Frankly, the "NOKSAN" suffix is too specific. I know, I know, I'm
bikeshedding but what happens if yet another tool wants to be disabled
from checking there and that tool is not *SAN? We rename again?

So the "NOCHECK" suffix made much more sense, even if it was generic.
IMNSVHO.

Thanks.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

[Andrey Ryabinin]

How exactly it's not safe? If we could dereference freed owner, I'd say we need to fix this,
but not hide.
I've seen use-after-free in mutex_spin_on_owner() once, but it was caused
by GPF in kernel which killed some task while it was holding mutex. So the next
time we tried to grab that mutex, lock->owner was already dead.
But normally we should release all locks before we able to kill task, so this won't happen.

[Peter Zijlstra]

On Fri, Oct 16, 2015 at 01:54:44PM +0300, Andrey Ryabinin wrote:
> On 10/16/2015 01:00 PM, Peter Zijlstra wrote:
> > On Fri, Oct 16, 2015 at 12:44:53PM +0300, Andrey Ryabinin wrote:
> >> Some code may perform racy by design memory reads. This could be
> >> harmless, yet such code may produce KASAN warnings.
> >>
> >> To hide such accesses from KASAN this patch introduces
> >> READ_ONCE_NOKSAN() macro. KASAN will not check the memory
> >> accessed by READ_ONCE_NOKSAN(). The KernelThreadSanitizer (KTSAN)
> >> is going to ignore it as well.
> >>
> >> This patch creates __read_once_size_noksan() a clone of
> >> __read_once_size(). The only difference between them is
> >> 'no_sanitized_address' attribute appended to '*_nokasan' function.
> >> This attribute tells the compiler that instrumentation of memory
> >> accesses should not be applied to that function. We declare it as
> >> static '__maybe_unsed' because GCC is not capable to inline such
> >> function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> >>
> >> With KASAN=n READ_ONCE_NOKSAN() is just a clone of READ_ONCE().
> >
> > Would we need a similar annotation for things like
> > mutex_spin_on_owner()'s dereference of owner, or is that considered safe
> > by KASAN?
> >
> > (its not actually safe; as I remember we have a problem with using
> > rcu_read_lock for tasks like that)
> >
>
> How exactly it's not safe?

I was worried perhaps KASAN would trip over the speculative nature of
the owner pointer, but we do verify it, so I suppose its allright.

> If we could dereference freed owner, I'd say we need to fix this,
> but not hide.

We should, just not 'trivial' and we seem to get distracted while
thinking of possible fixes :/

[Andrey Ryabinin]

On 10/16/2015 01:33 PM, Borislav Petkov wrote:
> On Fri, Oct 16, 2015 at 12:44:53PM +0300, Andrey Ryabinin wrote:
>> Some code may perform racy by design memory reads. This could be
>> harmless, yet such code may produce KASAN warnings.
>>
>> To hide such accesses from KASAN this patch introduces
>> READ_ONCE_NOKSAN() macro. KASAN will not check the memory
>> accessed by READ_ONCE_NOKSAN(). The KernelThreadSanitizer (KTSAN)
>> is going to ignore it as well.
>
> Frankly, the "NOKSAN" suffix is too specific. I know, I know, I'm
> bikeshedding but what happens if yet another tool wants to be disabled
> from checking there and that tool is not *SAN? We rename again?
>
> So the "NOCHECK" suffix made much more sense, even if it was generic.
> IMNSVHO.
>

Sounds reasonable.
Ingo, what do you think?

> Thanks.
>

[Paul E. McKenney]

On Fri, Oct 16, 2015 at 12:33:38PM +0200, Borislav Petkov wrote:
> On Fri, Oct 16, 2015 at 12:44:53PM +0300, Andrey Ryabinin wrote:
> > Some code may perform racy by design memory reads. This could be
> > harmless, yet such code may produce KASAN warnings.
> >
> > To hide such accesses from KASAN this patch introduces
> > READ_ONCE_NOKSAN() macro. KASAN will not check the memory
> > accessed by READ_ONCE_NOKSAN(). The KernelThreadSanitizer (KTSAN)
> > is going to ignore it as well.
>
> Frankly, the "NOKSAN" suffix is too specific. I know, I know, I'm
> bikeshedding but what happens if yet another tool wants to be disabled
> from checking there and that tool is not *SAN? We rename again?

Plaid, I say!!! The color of the bikeshed must be plaid!!! ;-)

Thanx, Paul

[Ingo Molnar]

* Andrey Ryabinin <arya...@virtuozzo.com> wrote:

Fine with me too.

Thanks,

Ingo

[Andrey Ryabinin]

get_wchan() is racy by design, it may access volatile stack
of running task, thus it may access redzone in a stack frame
and cause KASAN to warn about this.

Use READ_ONCE_NOCHECK() to silence these warnings.

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
Reported-by: Sasha Levin <sasha...@oracle.com>

---
arch/x86/kernel/process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c

index 39e585a..e28db18 100644

--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
if (sp < bottom || sp > top)
return 0;

- fp = READ_ONCE(*(unsigned long *)sp);

+ fp = READ_ONCE_NOCHECK(*(unsigned long *)sp);

do {
if (fp < bottom || fp > top)
return 0;
- ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));

+ ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));

if (!in_sched_functions(ip))
return ip;
- fp = READ_ONCE(*(unsigned long *)fp);

+ fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);

[Andrey Ryabinin]

Some code may perform racy by design memory reads. This could be
harmless, yet such code may produce KASAN warnings.

To hide such accesses from KASAN this patch introduces

READ_ONCE_NOCHECK() macro. KASAN will not check the memory
accessed by READ_ONCE_NOCHECK(). The KernelThreadSanitizer (KTSAN)

is going to ignore it as well.

This patch creates __read_once_size_nocheck() a clone of

__read_once_size(). The only difference between them is

'no_sanitized_address' attribute appended to '*_nocheck' function.

This attribute tells the compiler that instrumentation of memory
accesses should not be applied to that function. We declare it as
static '__maybe_unsed' because GCC is not capable to inline such
function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368

With KASAN=n READ_ONCE_NOCHECK() is just a clone of READ_ONCE().

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
---

include/linux/compiler-gcc.h | 13 +++++++++
include/linux/compiler.h | 66 +++++++++++++++++++++++++++++++++++---------

index c836eb2..3d78103 100644

+void __read_once_size_nocheck(const volatile void *p, void *res, int size)

+{
+ __READ_ONCE_SIZE;
+}
+#else
+static __always_inline

+void __read_once_size_nocheck(const volatile void *p, void *res, int size)

+{
+ __READ_ONCE_SIZE;
}
+#endif

static __always_inline void __write_once_size(volatile void *p, void *res, int size)
{
@@ -248,8 +274,22 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
* required ordering.
*/

-#define READ_ONCE(x) \
- ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })

+#define __READ_ONCE(x, check) \

+({ \
+ 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)
+
+/*
+ * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need

+ * to hide memory access from KASAN.
+ */

+#define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)

[Andrey Ryabinin]

Changes since v4:
- Rename back to READ_ONCE_NOCHECK().

Changes since v3:
- Fixed build failure.
- Rename: s/READ_ONCE_NOCHECK/READ_ONCE_NOKSAN
- Spelling fix.

Changes since v2:
- Added some code comments in the first patch.

Andrey Ryabinin (2):

compiler, atomics: Provide READ_ONCE_NOCHECK()

x86/mm: Silence KASAN warnings in get_wchan()

arch/x86/kernel/process.c | 6 ++--

include/linux/compiler-gcc.h | 13 +++++++++
include/linux/compiler.h | 66 +++++++++++++++++++++++++++++++++++---------

3 files changed, 69 insertions(+), 16 deletions(-)

--
2.4.9

[tip-bot for Andrey Ryabinin]

Commit-ID: f7d27c35ddff7c100d7a98db499ac0040149ac05
Gitweb: http://git.kernel.org/tip/f7d27c35ddff7c100d7a98db499ac0040149ac05
Author: Andrey Ryabinin <arya...@virtuozzo.com>
AuthorDate: Mon, 19 Oct 2015 11:37:18 +0300
Committer: Ingo Molnar <mi...@kernel.org>
CommitDate: Tue, 20 Oct 2015 11:04:19 +0200

x86/mm, kasan: Silence KASAN warnings in get_wchan()

get_wchan() is racy by design, it may access volatile stack
of running task, thus it may access redzone in a stack frame
and cause KASAN to warn about this.

Use READ_ONCE_NOCHECK() to silence these warnings.

Reported-by: Sasha Levin <sasha...@oracle.com>
Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>

Cc: Alexander Potapenko <gli...@google.com>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Andrey Konovalov <andre...@google.com>
Cc: Andy Lutomirski <lu...@amacapital.net>
Cc: Borislav Petkov <b...@alien8.de>
Cc: Denys Vlasenko <dvla...@redhat.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Kostya Serebryany <k...@google.com>
Cc: Linus Torvalds <torv...@linux-foundation.org>
Cc: Paul E. McKenney <pau...@linux.vnet.ibm.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: Wolfram Gloger <wm...@dent.med.uni-muenchen.de>
Cc: kasan-dev <kasa...@googlegroups.com>
Link: http://lkml.kernel.org/r/1445243838-17763-3-gi...@virtuozzo.com
Signed-off-by: Ingo Molnar <mi...@kernel.org>

[tip-bot for Andrey Ryabinin]

Commit-ID: d976441f44bc5d48635d081d277aa76556ffbf8b
Gitweb: http://git.kernel.org/tip/d976441f44bc5d48635d081d277aa76556ffbf8b
Author: Andrey Ryabinin <arya...@virtuozzo.com>
AuthorDate: Mon, 19 Oct 2015 11:37:17 +0300

Committer: Ingo Molnar <mi...@kernel.org>
CommitDate: Tue, 20 Oct 2015 11:04:19 +0200

compiler, atomics, kasan: Provide READ_ONCE_NOCHECK()

Some code may perform racy by design memory reads. This could be
harmless, yet such code may produce KASAN warnings.

To hide such accesses from KASAN this patch introduces
READ_ONCE_NOCHECK() macro. KASAN will not check the memory
accessed by READ_ONCE_NOCHECK(). The KernelThreadSanitizer
(KTSAN) is going to ignore it as well.

This patch creates __read_once_size_nocheck() a clone of
__read_once_size(). The only difference between them is
'no_sanitized_address' attribute appended to '*_nocheck'
function. This attribute tells the compiler that instrumentation
of memory accesses should not be applied to that function. We
declare it as static '__maybe_unsed' because GCC is not capable
to inline such function:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368

With KASAN=n READ_ONCE_NOCHECK() is just a clone of READ_ONCE().

Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>

Cc: Alexander Potapenko <gli...@google.com>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Andrey Konovalov <andre...@google.com>
Cc: Andy Lutomirski <lu...@amacapital.net>
Cc: Borislav Petkov <b...@alien8.de>
Cc: Denys Vlasenko <dvla...@redhat.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Kostya Serebryany <k...@google.com>
Cc: Linus Torvalds <torv...@linux-foundation.org>
Cc: Paul E. McKenney <pau...@linux.vnet.ibm.com>
Cc: Peter Zijlstra <pet...@infradead.org>

Cc: Sasha Levin <sasha...@oracle.com>

Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: Wolfram Gloger <wm...@dent.med.uni-muenchen.de>
Cc: kasan-dev <kasa...@googlegroups.com>

Link: http://lkml.kernel.org/r/1445243838-17763-2-gi...@virtuozzo.com
Signed-off-by: Ingo Molnar <mi...@kernel.org>