On Tue, Oct 29, 2019 at 3:45 PM Alexander Potapenko <glider@xxxxxxxxxx> wrote: > > On Fri, Oct 18, 2019 at 6:22 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > > > On Fri, Oct 18, 2019 at 11:43:01AM +0200, glider@xxxxxxxxxx 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
