| Message ID | 20191122112621.204798-36-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add KernelMemorySanitizer infrastructure | expand |
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 :( 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) + >
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) + > >
On Fri, 22 Nov 2019 at 12:28, <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(). > 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) Prefer 'if (IS_ENABLED(CONFIG_KMSAN))'. Thanks, -- Marco
On Tue, Dec 3, 2019 at 3:22 PM Marco Elver <elver@google.com> wrote: > > On Fri, 22 Nov 2019 at 12:28, <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(). > > 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) > > Prefer 'if (IS_ENABLED(CONFIG_KMSAN))'. Done in v4. > Thanks, > -- Marco
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) +
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(). 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(+)