Re: [PATCH RFC v2 09/25] kmsan: add KMSAN runtime

[Alexander Potapenko <glider@google.com>]

On Fri, Nov 8, 2019 at 1:17 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Fri, Nov 8, 2019 at 1:08 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Wed, Oct 30, 2019 at 3:23 PM <glider@google.com> wrote:
> > >
> > > This patch adds the KernelMemorySanitizer runtime and associated files:
> > >
> > >   - arch/x86/include/asm/kmsan.h: assembly definitions for hooking
> > >     interrupt handlers;
> > >   - include/linux/kmsan-checks.h: user API to enable/disable KMSAN,
> > >     poison/unpoison memory etc.
> > >   - include/linux/kmsan.h: declarations of KMSAN memory hooks to be
> > >     referenced outside KMSAN runtime
> > >   - lib/Kconfig.kmsan: declarations for CONFIG_KMSAN and
> > >     CONFIG_TEST_KMSAN
> > >   - mm/kmsan/Makefile: boilerplate Makefile
> > >   - mm/kmsan/kmsan.h: internal KMSAN declarations
> > >   - mm/kmsan/kmsan.c: core functions that operate with shadow and
> > >     origin memory and perform checks, utility functions
> > >   - mm/kmsan/kmsan_entry.c: KMSAN hooks for entry_64.S
> > >   - mm/kmsan/kmsan_hooks.c: KMSAN hooks for kernel subsystems
> > >   - mm/kmsan/kmsan_init.c: KMSAN initialization routines
> > >   - mm/kmsan/kmsan_instr.c: functions called by KMSAN instrumentation
> > >   - scripts/Makefile.kmsan: CFLAGS_KMSAN
> > >
> > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > To: Alexander Potapenko <glider@google.com>
> > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > Cc: linux-mm@kvack.org
> >
> > >+                       kmsan_pr_err("Local variable description: %s\n", descr);
> > >+                       kmsan_pr_err("Variable was created at:\n");
> >
> > It would be nice to tidy up the description, it contains lots of
> > duplicate/unneeded/confusing info:
> >
> > Local variable description: ----br.i@vp7045_read_mac_addr
> > Variable was created at:
> >  vp7045_read_eeprom drivers/media/usb/dvb-usb/vp7045.c:119 [inline]
> >  vp7045_read_mac_addr+0x7b/0xbe0 drivers/media/usb/dvb-usb/vp7045.c:132
> >  vp7045_read_eeprom drivers/media/usb/dvb-usb/vp7045.c:119 [inline]
> >  vp7045_read_mac_addr+0x7b/0xbe0 drivers/media/usb/dvb-usb/vp7045.c:132
> >
> > It could be just:
> >
> > Local variable br was created at:
> >  vp7045_read_eeprom drivers/media/usb/dvb-usb/vp7045.c:119 [inline]
> >  vp7045_read_mac_addr+0x7b/0xbe0 drivers/media/usb/dvb-usb/vp7045.c:132
> >  vp7045_read_eeprom drivers/media/usb/dvb-usb/vp7045.c:119 [inline]
> >  vp7045_read_mac_addr+0x7b/0xbe0 drivers/media/usb/dvb-usb/vp7045.c:132
>
>
> >+                       kmsan_pr_err("Uninit was stored to memory at:\n");
> >+                       chained_nr_entries =
> >+                               stack_depot_fetch(head, &chained_entries);
> >+                       stack_trace_print(chained_entries, chained_nr_entries,
> >+                                         0);
>
> I like how KCSAN trims all stacks of internal runtime frames. It would
> be nice to do the same for KMSAN, currently we have 3-4 uninteresting
> frames in the beginning of each stack. Besides taking space, it also
> requires to manually visually search for the actual interesting frame
> somewhere in the middle of the stack. E.g. all these frames are
> pointing into kmsan runtime guts:
>
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x1c9/0x220 lib/dump_stack.c:113
>  kmsan_report+0x12d/0x290 mm/kmsan/kmsan.c:682
>  __msan_warning+0x76/0xc0 mm/kmsan/kmsan_instr.c:316
>
> Uninit was stored to memory at:
>  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:247 [inline]
>  kmsan_save_stack mm/kmsan/kmsan.c:262 [inline]
>  kmsan_internal_chain_origin+0x162/0x260 mm/kmsan/kmsan.c:470
>  __msan_chain_origin+0x6d/0xb0 mm/kmsan/kmsan_instr.c:197
>
> Uninit was stored to memory at:
>  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:247 [inline]
>  kmsan_save_stack mm/kmsan/kmsan.c:262 [inline]
>  kmsan_internal_chain_origin+0x162/0x260 mm/kmsan/kmsan.c:470
>  __msan_chain_origin+0x6d/0xb0 mm/kmsan/kmsan_instr.c:197
>
> Uninit was stored to memory at:
>  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:247 [inline]
>  kmsan_save_stack mm/kmsan/kmsan.c:262 [inline]
>  kmsan_internal_chain_origin+0x162/0x260 mm/kmsan/kmsan.c:470
>  kmsan_memcpy_memmove_metadata+0x1a9/0xf30 mm/kmsan/kmsan.c:345
>  kmsan_memcpy_metadata+0xb/0x10 mm/kmsan/kmsan.c:363
>  __msan_memcpy+0x61/0x70 mm/kmsan/kmsan_instr.c:148
>
> Uninit was created at:
>  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:247 [inline]
>  kmsan_internal_poison_shadow+0x92/0x150 mm/kmsan/kmsan.c:170
>  kmsan_kmalloc+0xa1/0x100 mm/kmsan/kmsan_hooks.c:179
>  kmsan_slab_alloc+0xe/0x10 mm/kmsan/kmsan_hooks.c:188
>  slab_post_alloc_hook mm/slab.h:446 [inline]
>  slab_alloc_node mm/slub.c:2759 [inline]
>  __kmalloc_node_track_caller+0xf06/0x1120 mm/slub.c:4383

Not sure this is strictly required for the initial version, but I've
added the feature to the issue tracker:
https://github.com/google/kmsan/issues/66

-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg