Re: [PATCH RFC v1 23/26] kmsan: unpoisoning buffers from devices etc.

[Alexander Potapenko <glider@google.com>]

On Tue, Oct 29, 2019 at 3:45 PM Alexander Potapenko <glider@google.com> wrote:
>
> On Fri, Oct 18, 2019 at 6:22 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Fri, Oct 18, 2019 at 11:43:01AM +0200, glider@google.com wrote:
> > > When data is copied to memory from a device KMSAN should treat it as
> > > initialized. In most cases it's enough to just unpoison the buffer that
> > > is known to come from a device.
> > > In the case with __do_page_cache_readahead() and bio_copy_user_iov() we
> > > have to mark the whole pages as ignored by KMSAN, as it's not obvious
> > > where these pages are read again.
> >
> > ...
> >
> > > +++ b/mm/filemap.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/uaccess.h>
> > >  #include <linux/capability.h>
> > >  #include <linux/kernel_stat.h>
> > > +#include <linux/kmsan-checks.h>
> > >  #include <linux/gfp.h>
> > >  #include <linux/mm.h>
> > >  #include <linux/swap.h>
> > > @@ -2810,6 +2811,8 @@ static struct page *do_read_cache_page(struct address_space *mapping,
> > >               page = wait_on_page_read(page);
> > >               if (IS_ERR(page))
> > >                       return page;
> > > +             /* Assume all pages in page cache are initialized. */
> > > +             kmsan_unpoison_shadow(page_address(page), PAGE_SIZE);
> >
> > Why would you do that?  The page cache already keeps track of which
> > pages are initialised -- the PageUptodate flag is set on them.  Indeed,
> > just adding a kmsan call to SetPageUptodate and __SetPageUptodate would
> > probably be a very straightforward way of handling things, and probably
> > means you can get rid of a lot of these other calls.
> This seems like a very good thing to do, I'll definitely try that.
Hm, turns out there's no need for this particular call to
kmsan_unpoison_memory(),
because the errors that it was suppressing are better addressed by
making READ_ONCE_NOCHECK() return initialized memory.
On the other hand, unpoisoning memory in SetPageUptodate doesn't seem
to address the same errors.

 =====================================================
 BUG: KMSAN: uninit-value in[<      none      >] get_wchan+0x2c6/0x410
arch/x86/kernel/process.c:851
 CPU: 0 PID: 5159 Comm: ps Not tainted 5.4.0-rc5+ #3220
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
 Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:77
 [<      none      >] dump_stack+0x196/0x1f0 lib/dump_stack.c:113
 [<      none      >] kmsan_report+0x127/0x220 mm/kmsan/kmsan_report.c:108
 [<      none      >] __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
 [<      none      >] get_wchan+0x2c6/0x410 arch/x86/kernel/process.c:851
 [<      none      >] do_task_stat+0x16f3/0x30a0 fs/proc/array.c:522
 [<      none      >] proc_tgid_stat+0xbe/0xf0 fs/proc/array.c:632
 [<      none      >] proc_single_show+0x1a8/0x2b0 fs/proc/base.c:756
 [<      none      >] seq_read+0xad1/0x1da0 fs/seq_file.c:229
 [<      none      >] __vfs_read+0x1a9/0xc90 fs/read_write.c:425
 [<      none      >] vfs_read+0x333/0x6a0 fs/read_write.c:461
 [<      none      >] ksys_read+0x281/0x460 fs/read_write.c:587
 [<     inline     >] __do_sys_read fs/read_write.c:597
 [<      none      >] __se_sys_read+0x92/0xb0 fs/read_write.c:595
 [<      none      >] __x64_sys_read+0x4a/0x70 fs/read_write.c:595
 [<      none      >] do_syscall_64+0xb6/0x160 arch/x86/entry/common.c:291
 [<      none      >] entry_SYSCALL_64_after_hwframe+0x63/0xe7
arch/x86/entry/entry_64.S:177
 Local variable description: ----stat.i@__se_sys_newstat
 Variable was created at:
 [<     inline     >] __do_sys_newstat fs/stat.c:337
 [<      none      >] __se_sys_newstat+0x71/0xac0 fs/stat.c:337
 [<     inline     >] __do_sys_newstat fs/stat.c:337
 [<      none      >] __se_sys_newstat+0x71/0xac0 fs/stat.c:337
 =====================================================



> I however noticed that __SetPageUptodate is used when copying pages,
> not during disk I/O. Is that really so?
>
> We basically need the following behavior: if a device writes to a
> page, the contents of that page are considered initialized.
> However when the kernel copies one page to another, we must explicitly
> copy the source shadow page to the destination.
>
> On a related note, there seems to be a PG_dirty bit that indicates the
> page is to be flushed to disk.
> What's the best place to check such pages for being initialized, so
> that we can also report writes of uninitialized data to the disk?
>
>
> --
> 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



--
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