Re: [PATCH RFC v3 35/36] kmsan: ext4: skip block merging logic in ext4_mpage_readpages for KMSAN

[Alexander Potapenko <glider@google.com>]

On Mon, Nov 25, 2019 at 5:05 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 22/11/2019 11:26 am, glider@google.com wrote:
> > KMSAN doesn't allow treating adjacent memory pages as such, if they were
> > allocated by different alloc_pages() calls.
> > ext4_mpage_readpages() however does so: adjacent pages end up being passed
> > together to dma_direct_map_sg().
>
> Urgh, there are definitely more places where physically-contiguous pages
> are coalesced into single scatterlist entries - see
> sg_alloc_table_from_pages() for instance. I wouldn't be surprised if
> there are further open-coded versions hiding out in various other
> drivers/subsystems too. Unless I've misunderstood, this seems like quite
> an invasive limitation :(
You're right.
Other places haven't fired off so far, so I was just unaware of them,
but I'm anticipating more.
There are two possible solutions to that problem:
1. Allocate shadow and origin pages at fixed offset from the kernel page.
This is what we already do for vmalloc, but not for page_alloc(), as
it turned out to be quite hard.
Ideas on how to implement this approach are still welcome, because
it'll simplify the rest of the KMSAN runtime a lot.
2. Make all accesses touching non-contiguous pages access dummy shadow
pages instead, so that such accesses don't produce any uninitialized
values.
This is quite controversial, as it may prevent true positives from
being reported.

> Robin.
>
> > To prevent this, jump directly to the buffer_head-based read function in
> > KMSAN builds.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: linux-mm@kvack.org
> > ---
> >
> > Change-Id: I54ae8af536626a988e6398ff18a06c179b0ce034
> > ---
> >   fs/ext4/readpage.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> > index a30b203fa461..a3bb9e3ce5de 100644
> > --- a/fs/ext4/readpage.c
> > +++ b/fs/ext4/readpage.c
> > @@ -252,6 +252,17 @@ int ext4_mpage_readpages(struct address_space *mapping,
> >               if (page_has_buffers(page))
> >                       goto confused;
> >
> > +#if defined(CONFIG_KMSAN)
> > +             /*
> > +              * The following code may treat adjacent pages allocated
> > +              * separately as a bigger contiguous allocation.
> > +              * KMSAN doesn't allow this, as the corresponding metadata
> > +              * pages may be separated.
> > +              * Skip this complex logic for KMSAN builds.
> > +              */
> > +             goto confused;
> > +#endif
> > +
> >               block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits);
> >               last_block = block_in_file + nr_pages * blocks_per_page;
> >               last_block_in_file = (ext4_readpage_limit(inode) +
> >



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