Re: [RFC PATCH v2 17/18] mm/shmem: read without folio references

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: race in filemap_read_rcu_can_copy() copies hwpoisoned tail pages on split
--

commit 6b69ad4da5a3278d80ff53e3999062ede8b8a468
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

mm/shmem: read without folio references

This commit updates shmem_file_read_iter() to copy from the page cache
without taking folio references or locks via filemap_read_rcu() before
falling back to the slow path. It also updates filemap_read_rcu_can_copy()
to ignore PG_readahead on swapbacked folios where that bit is PG_reclaim.

> diff --git a/mm/shmem.c b/mm/shmem.c
> index 1c700fa9666d0..8700b03a8a5ee 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3313,7 +3313,14 @@ static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  	pgoff_t index;
>  	unsigned long offset;
>  	int error = 0;
> -	ssize_t retval = 0;
> +	ssize_t retval;
> +
> +	retval = filemap_read_rcu(iocb, to, last_pos);
> +	if (retval) {
> +		last_pos = iocb->ki_pos;
> +		if (!iov_iter_count(to))
> +			goto out;
> +	}

[Severity: High]
Can calling filemap_read_rcu() from shmem_file_read_iter() copy from a
hwpoisoned tail page instead of returning -EIO when a large shmem folio is
concurrently split?

Unlike regular filesystems, shmem_error_remove_folio() keeps hwpoisoned
folios in the page cache. When a large folio has a hwpoisoned tail page
(index > folio->index), PG_hwpoison is set on that tail subpage and
PG_has_hwpoisoned is set on the folio. If the folio is later split (for
example via truncate_inode_partial_folio(), shmem_unused_huge_shrink(), or
shmem_writeout(), or during memory_failure()), a concurrent reader in
shmem_file_read_iter() -> filemap_read_rcu() -> filemap_read_rcu_batch() can
bypass the hwpoison checks in three ways:

1. In mm/huge_memory.c:__split_folio_to_order(), PG_has_hwpoisoned is cleared
on the folio at the beginning of the split, before the loop over the subpages,
before ClearPageCompound() clears PG_head, and before
__folio_freeze_and_split_unmapped() updates the XArray slots:

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);
	...
	if (new_order)
		folio_set_order(folio, new_order);
	else
		ClearPageCompound(&folio->page);
}

During this window (or after a non-uniform split where new_order > 0 leaves
PG_head set on the first half while the poisoned subpage is in the second
half, before the XArray slots are updated), xas_load() still returns the
original folio with PG_head set and PG_has_hwpoisoned clear. In
filemap_read_rcu_can_copy(), folio_test_hwpoison(folio) only checks the head
page, so it returns true for the poisoned tail page, and
filemap_read_rcu_size() also sees xas_load(xas) == folio at end - 1, allowing
filemap_read_rcu_copy() to copy across the poisoned subpage.

2. In mm/filemap.c:filemap_read_rcu_can_copy(), folio_test_large(folio) is
tested twice without synchronization:

mm/filemap.c:filemap_read_rcu_can_copy() {
	...
	/* A split clears PG_head before it puts the new folios in their slots */
	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;
	...
}

If ClearPageCompound(&folio->page) runs between those two checks, the first
folio_test_large(folio) sees true (allowing folio->index != index) and the
second sees false (skipping the PG_has_hwpoisoned check).

3. Neither filemap_read_rcu_can_copy() nor filemap_read_rcu_copy() checks
PageHWPoison(page) on the individual subpages being copied:

mm/filemap.c:filemap_read_rcu_copy() {
	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);
		...
}

Could this copy corrupted data to userspace or trigger an unhandled kernel
machine check or page fault if the poisoned page was unmapped from the direct
map?

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