| Message ID | 20191122112621.204798-37-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add KernelMemorySanitizer infrastructure | expand |
On Fri, 22 Nov 2019 at 12:28, <glider@google.com> wrote: > > This is needed to allow memory tools like KASAN and KMSAN see the > memory accesses from the checksum code. Without CONFIG_GENERIC_CSUM the > tools can't see memory accesses originating from handwritten assembly > code. > For KASAN it's a question of detecting more bugs, for KMSAN using the C > implementation also helps avoid false positives originating from > seemingly uninitialized checksum values. The files touched are only in x86. The title mentions 'net' -- is this still accurate? > Signed-off-by: Alexander Potapenko <glider@google.com> > To: Alexander Potapenko <glider@google.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Michal Simek <monstr@monstr.eu> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: Vegard Nossum <vegard.nossum@oracle.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: linux-mm@kvack.org > --- > > v2: > - dropped the "default n" (as requested by Randy Dunlap) > > Change-Id: I645e2c097253a8d5717ad87e2e2df6f6f67251f3 > --- > arch/x86/Kconfig | 4 ++++ > arch/x86/include/asm/checksum.h | 10 +++++++--- > arch/x86/lib/Makefile | 2 ++ > 3 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 3f83a5c53808..f497aae3dbf4 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -272,6 +272,10 @@ config GENERIC_ISA_DMA > def_bool y > depends on ISA_DMA_API > > +config GENERIC_CSUM > + bool > + default y if KMSAN || KASAN When would it be desirable to use GENERIC_CSUM without KMSAN or KASAN? If it is generally a bad idea to disable GENERIC_CSUM with KMSAN and KASAN, does it make more sense to just remove this config option and unconditionally (if CONFIG_K{A,M}SAN) use the asm-generic version in checksum.h? This would simplify this patch and avoid introducing a config option that will likely never be set explicitly. > config GENERIC_BUG > def_bool y > depends on BUG > diff --git a/arch/x86/include/asm/checksum.h b/arch/x86/include/asm/checksum.h > index d79d1e622dcf..ab3464cbce26 100644 > --- a/arch/x86/include/asm/checksum.h > +++ b/arch/x86/include/asm/checksum.h > @@ -1,6 +1,10 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > -#ifdef CONFIG_X86_32 > -# include <asm/checksum_32.h> > +#ifdef CONFIG_GENERIC_CSUM > +# include <asm-generic/checksum.h> > #else > -# include <asm/checksum_64.h> > +# ifdef CONFIG_X86_32 > +# include <asm/checksum_32.h> > +# else > +# include <asm/checksum_64.h> > +# endif > #endif > diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile > index 5246db42de45..bca9031de9ff 100644 > --- a/arch/x86/lib/Makefile > +++ b/arch/x86/lib/Makefile > @@ -55,7 +55,9 @@ endif > lib-$(CONFIG_X86_USE_3DNOW) += mmx_32.o > else > obj-y += iomap_copy_64.o > +ifneq ($(CONFIG_GENERIC_CSUM),y) > lib-y += csum-partial_64.o csum-copy_64.o csum-wrappers_64.o > +endif > lib-y += clear_page_64.o copy_page_64.o > lib-y += memmove_64.o memset_64.o > lib-y += copy_user_64.o Thanks, -- Marco
On Tue, Dec 3, 2019 at 3:17 PM Marco Elver <elver@google.com> wrote: > > On Fri, 22 Nov 2019 at 12:28, <glider@google.com> wrote: > > > > This is needed to allow memory tools like KASAN and KMSAN see the > > memory accesses from the checksum code. Without CONFIG_GENERIC_CSUM the > > tools can't see memory accesses originating from handwritten assembly > > code. > > For KASAN it's a question of detecting more bugs, for KMSAN using the C > > implementation also helps avoid false positives originating from > > seemingly uninitialized checksum values. > > The files touched are only in x86. The title mentions 'net' -- is this > still accurate? Not sure :/ I'll probably replace it with "x86:", as other patches to this file don't mention "net". > > Signed-off-by: Alexander Potapenko <glider@google.com> > > To: Alexander Potapenko <glider@google.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Michal Simek <monstr@monstr.eu> > > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > > Cc: Vegard Nossum <vegard.nossum@oracle.com> > > Cc: Dmitry Vyukov <dvyukov@google.com> > > Cc: Randy Dunlap <rdunlap@infradead.org> > > Cc: linux-mm@kvack.org > > --- > > > > v2: > > - dropped the "default n" (as requested by Randy Dunlap) > > > > Change-Id: I645e2c097253a8d5717ad87e2e2df6f6f67251f3 > > --- > > arch/x86/Kconfig | 4 ++++ > > arch/x86/include/asm/checksum.h | 10 +++++++--- > > arch/x86/lib/Makefile | 2 ++ > > 3 files changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > index 3f83a5c53808..f497aae3dbf4 100644 > > --- a/arch/x86/Kconfig > > +++ b/arch/x86/Kconfig > > @@ -272,6 +272,10 @@ config GENERIC_ISA_DMA > > def_bool y > > depends on ISA_DMA_API > > > > +config GENERIC_CSUM > > + bool > > + default y if KMSAN || KASAN > > When would it be desirable to use GENERIC_CSUM without KMSAN or KASAN? > > If it is generally a bad idea to disable GENERIC_CSUM with KMSAN and > KASAN, does it make more sense to just remove this config option and > unconditionally (if CONFIG_K{A,M}SAN) use the asm-generic version in > checksum.h? > > This would simplify this patch and avoid introducing a config option > that will likely never be set explicitly. There is already a similar option for MIPS and m68k, so I think it's better to be consistent with them. > > config GENERIC_BUG > > def_bool y > > depends on BUG > > diff --git a/arch/x86/include/asm/checksum.h b/arch/x86/include/asm/checksum.h > > index d79d1e622dcf..ab3464cbce26 100644 > > --- a/arch/x86/include/asm/checksum.h > > +++ b/arch/x86/include/asm/checksum.h > > @@ -1,6 +1,10 @@ > > /* SPDX-License-Identifier: GPL-2.0 */ > > -#ifdef CONFIG_X86_32 > > -# include <asm/checksum_32.h> > > +#ifdef CONFIG_GENERIC_CSUM > > +# include <asm-generic/checksum.h> > > #else > > -# include <asm/checksum_64.h> > > +# ifdef CONFIG_X86_32 > > +# include <asm/checksum_32.h> > > +# else > > +# include <asm/checksum_64.h> > > +# endif > > #endif > > diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile > > index 5246db42de45..bca9031de9ff 100644 > > --- a/arch/x86/lib/Makefile > > +++ b/arch/x86/lib/Makefile > > @@ -55,7 +55,9 @@ endif > > lib-$(CONFIG_X86_USE_3DNOW) += mmx_32.o > > else > > obj-y += iomap_copy_64.o > > +ifneq ($(CONFIG_GENERIC_CSUM),y) > > lib-y += csum-partial_64.o csum-copy_64.o csum-wrappers_64.o > > +endif > > lib-y += clear_page_64.o copy_page_64.o > > lib-y += memmove_64.o memset_64.o > > lib-y += copy_user_64.o > > Thanks, > -- Marco
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 3f83a5c53808..f497aae3dbf4 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -272,6 +272,10 @@ config GENERIC_ISA_DMA def_bool y depends on ISA_DMA_API +config GENERIC_CSUM + bool + default y if KMSAN || KASAN + config GENERIC_BUG def_bool y depends on BUG diff --git a/arch/x86/include/asm/checksum.h b/arch/x86/include/asm/checksum.h index d79d1e622dcf..ab3464cbce26 100644 --- a/arch/x86/include/asm/checksum.h +++ b/arch/x86/include/asm/checksum.h @@ -1,6 +1,10 @@ /* SPDX-License-Identifier: GPL-2.0 */ -#ifdef CONFIG_X86_32 -# include <asm/checksum_32.h> +#ifdef CONFIG_GENERIC_CSUM +# include <asm-generic/checksum.h> #else -# include <asm/checksum_64.h> +# ifdef CONFIG_X86_32 +# include <asm/checksum_32.h> +# else +# include <asm/checksum_64.h> +# endif #endif diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile index 5246db42de45..bca9031de9ff 100644 --- a/arch/x86/lib/Makefile +++ b/arch/x86/lib/Makefile @@ -55,7 +55,9 @@ endif lib-$(CONFIG_X86_USE_3DNOW) += mmx_32.o else obj-y += iomap_copy_64.o +ifneq ($(CONFIG_GENERIC_CSUM),y) lib-y += csum-partial_64.o csum-copy_64.o csum-wrappers_64.o +endif lib-y += clear_page_64.o copy_page_64.o lib-y += memmove_64.o memset_64.o lib-y += copy_user_64.o
This is needed to allow memory tools like KASAN and KMSAN see the memory accesses from the checksum code. Without CONFIG_GENERIC_CSUM the tools can't see memory accesses originating from handwritten assembly code. For KASAN it's a question of detecting more bugs, for KMSAN using the C implementation also helps avoid false positives originating from seemingly uninitialized checksum values. Signed-off-by: Alexander Potapenko <glider@google.com> To: Alexander Potapenko <glider@google.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Michal Simek <monstr@monstr.eu> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> Cc: Vegard Nossum <vegard.nossum@oracle.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: linux-mm@kvack.org --- v2: - dropped the "default n" (as requested by Randy Dunlap) Change-Id: I645e2c097253a8d5717ad87e2e2df6f6f67251f3 --- arch/x86/Kconfig | 4 ++++ arch/x86/include/asm/checksum.h | 10 +++++++--- arch/x86/lib/Makefile | 2 ++ 3 files changed, 13 insertions(+), 3 deletions(-)