Re: [PATCH v4 10/14] filemap: Add hwpoison handling to filemap_read()

[Matthew Wilcox <willy@infradead.org>]

Sashiko review:
> --- Patch [10]: [PATCH v4 10/14] filemap: Add hwpoison handling to filemap_read() ---
> commit c46fb3fcf6cb7055457888cfb186af5ae3a493c2
> Author: Jane Chu <jane.chu@oracle.com>
> 
> filemap: Add hwpoison handling to filemap_read()
> 
> This commit adds hardware poison handling to filemap_read(). It allows the
> read iterator to make a best-effort copy of data from clean pages,
> preventing Machine Check Exceptions when the page cache contains HWpoison.
> 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 9cde74320e48a..b146c5228741c 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -1111,6 +1112,16 @@ static inline bool is_page_hwpoison(const struct page *page)
> >  	return PageHWPoison(page);
> >  }
> >
> > +static inline bool is_ref_page_hwpoison(const struct folio *folio,
> > +		const struct page *page)
> > +{
> > +	if (PageHWPoison(page))
> > +		return true;
> > +	if (folio_test_hugetlb(folio))
> > +		return hugetlb_page_hwpoison(folio, page);
> 
> Does this lockless traversal risk a Use-After-Free?
> 
> When this function calls into hugetlb_page_hwpoison(), it eventually reaches
> precise_page_poisoned() which performs a lockless iteration of the
> hugetlb_hwpoison llist:
> 
> precise_page_poisoned()
>   llist_for_each_entry(p, READ_ONCE(list->first), node) {
>     ...
>   }
> 
> However, a concurrent memory_failure() can free the list nodes while the
> lockless traversal is in progress, since __folio_free_raw_hwp() calls
> kfree(p) on the nodes without an RCU grace period.
> 
> While filemap_read() does not process hugetlb folios, hugetlbfs_read_iter()
> (added earlier in this series) does, making the Use-After-Free directly
> reachable.

Will be addressed by the spinlock I mentioned earlier.

> [ ... ]
> 
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 58eb9d2406434..26a5f18121f90 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -2483,6 +2483,8 @@ static void filemap_get_read_batch(struct address_space *mapping,
> >
> >  		if (!folio_batch_add(fbatch, folio))
> >  			break;
> > +		if (folio_has_hwpoisoned_page(folio))
> > +			break;
> 
> This isn't a bug introduced by this patch, but does returning a poisoned
> folio here risk a Machine Check Exception if a disk read populates the folio?

Sure, but we remove hwpoisoned pages from the pagecache for
storage-backed files.  So this doesn't come up.

> If a poisoned folio in the page cache is not uptodate,
> filemap_get_read_batch() will return it as the last folio in the batch.
> 
> Then filemap_get_pages() will see !folio_test_uptodate(folio) and call
> filemap_update_page(), which locks the folio and passes it to the
> filesystem's read_folio callback:
> 
> filemap_update_page()
>   error = filemap_read_folio(iocb->ki_filp, mapping->a_ops->read_folio,
>                              folio);
> 
> If the filesystem populates the folio using CPU operations (which is common
> for inline data, fscrypt, squashfs, or block device emulation), the CPU will
> access the poisoned memory and trigger an unrecoverable Machine Check
> Exception. The patch guards the userspace copy path, but misses guarding the
> cache-miss path where the kernel itself reads into the poisoned folio.
> 
> >  		if (!folio_test_uptodate(folio))
> >  			break;
> 
> [ ... ]
> 
> > @@ -2862,14 +2887,18 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
> >  			if (writably_mapped)
> >  				flush_dcache_folio(folio);
> >
> > -			copied = copy_folio_to_iter(folio, offset, bytes, iter);
> > +			copied = adjust_range_hwpoison(folio, offset, bytes);
> > +			if (copied < bytes)
> > +				error = -EIO;
> > +			copied = copy_folio_to_iter(folio, offset, copied, iter);
> 
> This isn't a bug introduced by this patch, but will filemap_splice_read()
> still trigger a Machine Check Exception on poisoned pages since it lacks
> similar protection?
> 
> While this patch successfully updates filemap_read() to use
> adjust_range_hwpoison() to prevent reading poisoned memory,
> filemap_splice_read() was left unmodified.

Hugetlb doesn't implement splice_read(), so this is irrelevant.

> filemap_splice_read() uses the same filemap_get_pages() logic, loops over
> the returned batch, and unconditionally splices each folio into the pipe:
> 
> filemap_splice_read()
>   ...
>   n = splice_folio_into_pipe(pipe, folio, *ppos, n);
> 
> When a userspace process subsequently reads from the pipe, the kernel will
> access the poisoned physical page (e.g., via copy_page_to_iter()), triggering
> an unrecoverable Machine Check Exception.