Re: [PATCH v7 18/43] instrumented.h: add KMSAN support

From: Alexander Potapenko <glider@google.com>
To: youling 257 <youling257@gmail.com>
Cc: Marco Elver <elver@google.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	 Alexei Starovoitov <ast@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	 Andrey Konovalov <andreyknvl@google.com>,
	Andy Lutomirski <luto@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	 Borislav Petkov <bp@alien8.de>, Christoph Hellwig <hch@lst.de>,
	Christoph Lameter <cl@linux.com>,
	 David Rientjes <rientjes@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	 Eric Biggers <ebiggers@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	 Ilya Leoshkevich <iii@linux.ibm.com>,
	Ingo Molnar <mingo@redhat.com>, Jens Axboe <axboe@kernel.dk>,
	 Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	Kees Cook <keescook@chromium.org>,
	 Mark Rutland <mark.rutland@arm.com>,
	Matthew Wilcox <willy@infradead.org>,
	 "Michael S. Tsirkin" <mst@redhat.com>,
	Pekka Enberg <penberg@kernel.org>,
	 Peter Zijlstra <peterz@infradead.org>,
	Petr Mladek <pmladek@suse.com>,
	 Stephen Rothwell <sfr@canb.auug.org.au>,
	Steven Rostedt <rostedt@goodmis.org>,
	 Thomas Gleixner <tglx@linutronix.de>,
	Vasily Gorbik <gor@linux.ibm.com>,
	 Vegard Nossum <vegard.nossum@oracle.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	kasan-dev@googlegroups.com,  linux-mm@kvack.org,
	linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 18/43] instrumented.h: add KMSAN support
Date: Fri, 21 Oct 2022 13:37:31 -0700	[thread overview]
Message-ID: <CAG_fn=UVARRueXn4mU51TkzLTpZ=2fKNL7NAB3YH7mGP71ZhUQ@mail.gmail.com> (raw)
In-Reply-To: <CAOzgRdYr82TztbX4j7SDjJFiTd8b1B60QZ7jPkNOebB-jO9Ocg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3536 bytes --]

On Fri, Oct 21, 2022 at 8:19 AM youling 257 <youling257@gmail.com> wrote:

> CONFIG_DEBUG_INFO=y
> CONFIG_AS_HAS_NON_CONST_LEB128=y
> # CONFIG_DEBUG_INFO_NONE is not set
> CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y
> # CONFIG_DEBUG_INFO_DWARF4 is not set
> # CONFIG_DEBUG_INFO_DWARF5 is not set
> # CONFIG_DEBUG_INFO_REDUCED is not set
> # CONFIG_DEBUG_INFO_COMPRESSED is not set
> # CONFIG_DEBUG_INFO_SPLIT is not set
> # CONFIG_DEBUG_INFO_BTF is not set
> # CONFIG_GDB_SCRIPTS is not set
>
> perf top still no function name.
>
> 12.90%  [kernel]              [k] 0xffffffff833dfa64
>

I think I know what's going on. The two functions that differ with and
without the patch were passing an incremented pointer to unsafe_put_user(),
which is a macro, e.g.:

   unsafe_put_user((compat_ulong_t)m, umask++, Efault);

Because that macro didn't evaluate its second parameter, "umask++" was
passed to a call to kmsan_copy_to_user(), which resulted in an extra
increment of umask.
This probably violated some expectations of the userspace app, which in
turn led to repetitive kernel calls.

Could you please check if the patch below fixes the problem for you?

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 8bc614cfe21b9..1cc756eafa447 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -254,24 +254,25 @@ extern void __put_user_nocheck_8(void);
 #define __put_user_size(x, ptr, size, label)                           \
 do {                                                                   \
        __typeof__(*(ptr)) __x = (x); /* eval x once */                 \
-       __chk_user_ptr(ptr);                                            \
+       __typeof__(ptr) __ptr = (ptr); /* eval ptr once */              \
+       __chk_user_ptr(__ptr);                                          \
        switch (size) {                                                 \
        case 1:                                                         \
-               __put_user_goto(__x, ptr, "b", "iq", label);            \
+               __put_user_goto(__x, __ptr, "b", "iq", label);          \
                break;                                                  \
        case 2:                                                         \
-               __put_user_goto(__x, ptr, "w", "ir", label);            \
+               __put_user_goto(__x, __ptr, "w", "ir", label);          \
                break;                                                  \
        case 4:                                                         \
-               __put_user_goto(__x, ptr, "l", "ir", label);            \
+               __put_user_goto(__x, __ptr, "l", "ir", label);          \
                break;                                                  \
        case 8:                                                         \
-               __put_user_goto_u64(__x, ptr, label);                   \
+               __put_user_goto_u64(__x, __ptr, label);                 \
                break;                                                  \
        default:                                                        \
                __put_user_bad();                                       \
        }                                                               \
-       instrument_put_user(__x, ptr, size);                            \
+       instrument_put_user(__x, __ptr, size);                          \
 } while (0)

 #ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT

[-- Attachment #2: Type: text/html, Size: 4999 bytes --]
