Re: [RFC PATCH v2 14/18] mm/filemap: read large folios without folio references

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/filemap: lockless large folio read can bypass hwpoison check and copy poisoned subpage
--

commit 3f27d3e89354814e80ba93e8767ff8878253a593
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

mm/filemap: read large folios without folio references

This patch extends lockless RCU pagecache reads to support large folios
without taking folio references. It checks whether the last page of the
read range still points to the same folio in the XArray and copies data
page by page via _copy_to_iter().

> diff --git a/mm/filemap.c b/mm/filemap.c
> index e99705a66c1d4..182ae7d5db56a 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2834,15 +2834,16 @@ static bool filemap_read_rcu_can_copy(struct folio *folio, pgoff_t index,
>  		return false;
>  	if (folio_test_readahead(folio) || folio_test_dropbehind(folio))
>  		return false;
> -	/* One reference covers all of a large folio's pages */
> -	if (folio_test_large(folio))
> -		return false;
>  	/* A split clears PG_head before it puts the new folios in their slots */
> -	if (folio->index != index)
> +	if (!folio_test_large(folio) && folio->index != index)
>  		return false;
>  	/* memory_failure() waits only for readers that got past this */
>  	if (folio_test_hwpoison(folio))
>  		return false;
> +	/* Not folio_test_has_hwpoisoned(): it asserts PG_head, a split clears it */
> +	if (folio_test_large(folio) &&
> +	    test_bit(PG_has_hwpoisoned, &folio->_flags_1))
> +		return false;
>  
>  	/*
>  	 * The slow path does not call folio_mark_accessed() on the folio that
> @@ -2896,6 +2897,62 @@ static struct folio *filemap_read_rcu_lookup(struct xa_state *xas,
>  	return folio;
>  }
>  
> +/*
> + * How much of @bytes, starting @offset into page @index, can be copied from
> + * @folio.  A folio is in all the slots of its range or in none: when the slot
> + * of the last page holds it too, so do the slots in between, and the pages
> + * are this folio's.
> + */
> +static size_t filemap_read_rcu_size(struct xa_state *xas, struct folio *folio,
> +		pgoff_t index, size_t offset, size_t bytes)
> +{
> +	pgoff_t end = index + DIV_ROUND_UP(offset + bytes, PAGE_SIZE);
> +	unsigned int order;
> +	pgoff_t next;
> +
> +	if (end - index > 1 && folio_test_large(folio)) {
> +		/* A split can change it under us: it only picks the page to check */
> +		order = min(folio_large_order(folio), MAX_PAGECACHE_ORDER);
> +		next = folio->index + (1UL << order);
> +		if (next > index && next < end)
> +			end = next;
> +		xas_set(xas, end - 1);
> +		/* A split after this copies PG_rcu_free to the new folios */
> +		if (xas_load(xas) == folio)
> +			return min(bytes, (end - index) * PAGE_SIZE - offset);
> +	}
> +	return min(bytes, PAGE_SIZE - offset);
> +}
> +
> +/*
> + * Copy @bytes from @folio, starting @offset into its page at @index.  Page
> + * by page, as copy_page_to_iter() does, without its checks for each call.
> + * Not copy_to_iter(): its usercopy check reads the folio's order, which a
> + * split can change.
> + */
> +static size_t filemap_read_rcu_copy(struct folio *folio, pgoff_t index,
> +		size_t offset, size_t bytes, struct iov_iter *iter)
> +{
> +	struct page *page = folio_page(folio, index - folio->index);
> +	size_t n = 0;
> +
> +	while (n < bytes) {
> +		size_t len = min(bytes - n, PAGE_SIZE - offset);
> +		size_t done;
> +		void *kaddr;
> +
> +		kaddr = kmap_local_page(page);
> +		done = _copy_to_iter(kaddr + offset, len, iter);

[Severity: High]
As reported by Sashiko on RFC [1], can a lockless reader in
filemap_read_rcu_batch() bypass the hwpoison checks in
filemap_read_rcu_can_copy() and copy from a hardware-poisoned tail subpage in
filemap_read_rcu_copy()?

When a hardware memory error hits a tail page p of a large folio,
memory_failure() sets PG_hwpoison on the tail struct page p rather than on
&folio->page, so folio_test_hwpoison(folio) in filemap_read_rcu_can_copy()
only checks subpage 0. Meanwhile, filemap_read_rcu_copy() copies each subpage
via _copy_to_iter() without checking PageHWPoison(page), relying on
filemap_read_rcu_can_copy() to reject the folio beforehand.

However, there appear to be three ways a poisoned tail subpage can get past
filemap_read_rcu_can_copy():

1. During a split, __split_folio_to_order() in mm/huge_memory.c clears
PG_has_hwpoisoned on the original folio at the start of the split, before the
new sub-folios are initialized, before ClearPageCompound() clears PG_head,
and before __folio_freeze_and_split_unmapped() updates the XArray slots in
mapping->i_pages:

mm/huge_memory.c:__split_folio_to_order() {
	...
	folio_clear_has_hwpoisoned(folio);

	/* Check first new_nr_pages since the loop below skips them */
	if (handle_hwpoison &&
	    page_range_has_hwpoisoned(folio_page(folio, 0), new_nr_pages))
		folio_set_has_hwpoisoned(folio);
	...
}

While the XArray slots still point to folio, folio_test_large(folio) remains
true (including across splits to new_order > 0 when the poisoned subpage is
at index >= folio->index + (1UL << new_order)) while PG_has_hwpoisoned is 0.
A reader looking up the poisoned tail index or sizing a multi-page read
across the folio in filemap_read_rcu_size() will see the folio still in its
slots with PG_has_hwpoisoned cleared and proceed to copy the poisoned subpage.

2. filemap_read_rcu_can_copy() evaluates folio_test_large(folio) twice
without caching the result:

	if (!folio_test_large(folio) && folio->index != index)
		return false;
	if (folio_test_hwpoison(folio))
		return false;
	if (folio_test_large(folio) &&
	    test_bit(PG_has_hwpoisoned, &folio->_flags_1))
		return false;

If a concurrent split to order 0 clears PG_has_hwpoisoned and PG_head between
the two folio_test_large(folio) calls while reading at index != folio->index,
the first check sees folio_test_large(folio) as true (skipping the
folio->index != index check) and the second check sees it as false (skipping
the PG_has_hwpoisoned check). filemap_read_rcu_size() then returns a single
page and filemap_read_rcu_copy() copies from folio_page(folio, index -
folio->index).

3. In mm/memory-failure.c, memory_failure() sets PG_hwpoison on the tail page
p via TestSetPageHWPoison(p) before calling get_hwpoison_page() and before
setting PG_has_hwpoisoned on the folio. If get_hwpoison_page() fails (for
example, while the folio refcount is transiently frozen), PG_hwpoison remains
set on tail page p while PG_has_hwpoisoned is never set on the folio. If a
subsequent memory_failure() call hits TestSetPageHWPoison(p), it calls
synchronize_rcu() and returns 0 so the caller can unmap p from the kernel
direct map, after which a lockless reader copying from p in
filemap_read_rcu_copy() can trigger an unhandled kernel page fault.

Would checking PageHWPoison(page) on each subpage in filemap_read_rcu_copy()
(or before copying) avoid reading from a poisoned tail page across these
races?

[1] https://lore.kernel.org/r/20261019154913.1765974-16-kas@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104130118.2507513-1-kas@kernel.org?part=14