Re: [PATCH 2/3] mm: allow freezing i_pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] mm: shmem: race between shmem_writeout() and mapping_set_frozen()
- [High] mm: filemap_fault() returns VM_FAULT_OOM on -EBUSY from __filemap_get_folio()
- [High] mm: truncate: unlocked mapping_frozen() check and infinite loop on -EBUSY
- [High] mm: shmem: unhandled -EBUSY in shmem_unuse_swap_entries() on frozen mapping
- [Medium] mm: race between mapping_set_frozen() and filemap_remove_folio() rollback
--

commit 22b54347e6ef0325973e1e89c921cfa0dc0a8784
Author: Pratyush Yadav (Google) <pratyush@kernel.org>

mm: allow freezing i_pages

This commit introduces the AS_FROZEN address_space flag to prevent
additions, removals, replacements, splits, and collapses in mapping->i_pages
while a file's page cache is serialized for live update. Core page cache,
truncation, reclaim, migration, shmem, and THP paths check mapping_frozen()
and reject changes with -EBUSY or warnings.

[ ... ]

> diff --git a/mm/filemap.c b/mm/filemap.c
> index b74bc1e5015c6..d040c69177f79 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -138,6 +138,7 @@ static void page_cache_delete(struct address_space *mapping,
>  	nr = folio_nr_pages(folio);
>  
>  	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> +	VM_WARN_ON_ONCE_FOLIO(mapping_frozen(mapping), folio);

[Severity: Medium]
Can an in-progress page cache insertion race with mapping_set_frozen() and
trigger this warning during error rollback?

Several paths insert a folio into mapping->i_pages under
xa_lock_irq(&mapping->i_pages), drop the lock, and then call
filemap_remove_folio() -> __filemap_remove_folio() -> page_cache_delete() if
a subsequent step fails, without holding inode_lock:

1. In mm/shmem.c, shmem_get_folio_gfp() -> shmem_alloc_and_add_folio() calls
shmem_add_to_page_cache() before shmem_inode_acct_blocks() and before the
post-insertion i_size check:

    error = shmem_add_to_page_cache(folio, mapping, index, NULL, gfp);
    if (error)
        goto unlock;

    error = shmem_inode_acct_blocks(inode, pages);
    if (error) {
        ...
        error = shmem_inode_acct_blocks(inode, pages);
        if (error) {
            filemap_remove_folio(folio);
            goto unlock;
        }
    }

and in shmem_get_folio_gfp():

    /* Perhaps the file has been truncated since we checked */
    if (sgp <= SGP_CACHE &&
        ((loff_t)index << PAGE_SHIFT) >= i_size_read(inode)) {
        error = -EINVAL;
        goto unlock;
    }
    ...
unlock:
    if (alloced)
        filemap_remove_folio(folio);

If a fault checks index < i_size before a concurrent ftruncate() shrinks the
file, and mapping_set_frozen() runs after shmem_add_to_page_cache() succeeds
(where memfd_luo_preserve_folios() only scans up to the new i_size), the
rollback calls filemap_remove_folio() on the frozen mapping.

2. In mm/userfaultfd.c, __mfill_atomic_pte() adds the folio to i_pages via
ops->filemap_add() (shmem_mfill_filemap_add()) and then calls
mfill_atomic_install_pte():

    if (ops->filemap_add) {
        ret = ops->filemap_add(folio, state->vma, state->dst_addr);
        if (ret)
            goto err_folio_put;
    }

    ret = mfill_atomic_install_pte(state->pmd, state->vma, dst_addr,
                       &folio->page, flags);
    if (ret)
        goto err_filemap_remove;

    return 0;

err_filemap_remove:
    if (ops->filemap_remove)
        ops->filemap_remove(folio, state->vma);

If mapping_set_frozen() runs before mfill_atomic_install_pte() fails (for
example with -EEXIST or via mfill_file_over_size()),
shmem_mfill_filemap_remove() calls filemap_remove_folio() on the frozen
mapping.

3. In mm/readahead.c, read_pages() inserts folios into mapping->i_pages
before calling aops->readahead(), and removes any unprocessed folios with
filemap_remove_folio().

How should these post-insertion rollback paths synchronize with
mapping_set_frozen()?

[ ... ]

> @@ -873,6 +876,10 @@ noinline int __filemap_add_folio(struct address_space *mapping,
>  		void *entry, *old = NULL;
>  
>  		xas_lock_irq(&xas);
> +		if (unlikely(mapping_frozen(mapping))) {
> +			xas_set_err(&xas, -EBUSY);
> +			goto unlock;
> +		}
>  		xas_for_each_conflict(&xas, entry) {
>  			old = entry;
>  			if (!xa_is_value(entry)) {

[Severity: High]
How does filemap_fault() in mm/filemap.c handle -EBUSY when
__filemap_add_folio() rejects adding a folio to a frozen mapping?

When __filemap_add_folio() returns -EBUSY, it propagates through
filemap_add_folio() and __filemap_get_folio_mpol() as ERR_PTR(-EBUSY).
However, in mm/filemap.c, filemap_fault() treats any error from
__filemap_get_folio() as -ENOMEM and returns VM_FAULT_OOM:

    folio = __filemap_get_folio(mapping, index,
                      FGP_CREAT|FGP_FOR_MMAP,
                      vmf->gfp_mask);
    if (IS_ERR(folio)) {
        if (fpin)
            goto out_retry;
        filemap_invalidate_unlock_shared(mapping);
        return VM_FAULT_OOM;
    }

When a page fault hits an unpopulated index within i_size on a frozen
mapping using filemap_fault(), returning VM_FAULT_OOM to the architecture
page fault handler triggers pagefault_out_of_memory() and retries the fault
in an infinite loop (or triggers a memcg OOM kill) instead of returning
VM_FAULT_SIGBUS.

Should filemap_fault() convert PTR_ERR(folio) via vmf_error(PTR_ERR(folio))
here instead of unconditionally returning VM_FAULT_OOM?

[ ... ]

> diff --git a/mm/shmem.c b/mm/shmem.c
> index 13e0786436cda..6507b4246c8af 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c

[ ... ]

> @@ -1822,6 +1830,10 @@ int shmem_writeout(struct swap_io_ctx *ctx, struct folio *folio,
>  	if ((info->flags & SHMEM_F_LOCKED) || sbinfo->noswap)
>  		goto redirty;
>  
> +	/* Swapping out would replace the folio in a frozen mapping. */
> +	if (mapping_frozen(mapping))
> +		goto redirty;
> +
>  	if (!total_swap_pages)
>  		goto redirty;

[Severity: Critical]
Is this lockless mapping_frozen(mapping) check in shmem_writeout() racy
against mapping_set_frozen()?

Page reclaim (shrink_folio_list() -> pageout() -> shmem_writeout()) runs
with only the folio lock held (for instance, on a folio allocated beyond
i_size with FALLOC_FL_KEEP_SIZE, which shmem_fallocate() marks dirty and
memfd_luo_preserve_folios() does not pin).

After shmem_writeout() checks mapping_frozen(mapping) without holding
xa_lock_irq(&mapping->i_pages), it zeroes any !uptodate folio, allocates swap
via folio_alloc_swap(), and updates accounting in shmem_recalc_inode() before
calling shmem_delete_from_page_cache():

    xa_lock_irq(&mapping->i_pages);
    error = shmem_replace_entry(mapping, folio->index, folio, radswap);
    folio->mapping = NULL;
    mapping->nrpages -= nr;
    shmem_update_stats(folio, -nr);
    xa_unlock_irq(&mapping->i_pages);

If mapping_set_frozen(mapping) runs on another CPU between the unlocked check
in shmem_writeout() and shmem_delete_from_page_cache(),
shmem_delete_from_page_cache() replaces the folio in mapping->i_pages with a
swap entry after the mapping was frozen.

Even worse, if swap_writeout() then returns AOP_WRITEPAGE_ACTIVATE without
writing the folio (for example, when zswap_store() fails and
!mem_cgroup_zswap_writeback_enabled()), shmem_writeout() attempts to put the
folio back into the page cache:

    error = shmem_add_to_page_cache(folio, mapping, index,
            swp_to_radix_entry(folio->swap),
            __GFP_HIGH | __GFP_NOMEMALLOC | __GFP_NOWARN);
    /* Swap entry might be erased by racing shmem_free_swap() */
    if (!error) {
        shmem_recalc_inode(inode, 0, -nr_pages);
        folio_put_swap(folio, NULL);
    }
    ...
    swap_cache_del_folio(folio);
    goto redirty;

Because AS_FROZEN is now set, shmem_add_to_page_cache() fails with -EBUSY,
so shmem_writeout() skips folio_put_swap() and calls
swap_cache_del_folio(folio), discarding the dirty unwritten folio from the
swap cache while leaving the unwritten swap entry in mapping->i_pages.

Should shmem_delete_from_page_cache() check mapping_frozen(mapping) under
xa_lock_irq(&mapping->i_pages) and abort the swap-out if the mapping has
been frozen?

> @@ -2483,6 +2495,13 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
>  	if (softleaf_is_poison_marker(index_entry))
>  		return -EIO;
>  
> +	/*
> +	 * Swapping in would change the i_pages of a frozen mapping. A frozen
> +	 * mapping should not have swap entries in the first place.
> +	 */
> +	if (unlikely(mapping_frozen(mapping)))
> +		return -EBUSY;
> +
>  	si = get_swap_device(index_entry);
>  	order = shmem_confirm_swap(mapping, index, index_entry);
>  	if (IS_ERR_OR_NULL(si)) {

[Severity: High]
What happens during swapoff in shmem_unuse_inode() if a frozen shmem mapping
still contains swap entries (for example, from pages fallocated with
FALLOC_FL_KEEP_SIZE beyond i_size that were swapped out before the mapping
was frozen, or via the shmem_writeout() race above)?

In mm/shmem.c, shmem_unuse_inode() loops over swap entries calling
shmem_unuse_swap_entries():

    do {
        folio_batch_init(&fbatch);
        if (!shmem_find_swap_entries(mapping, start, &fbatch,
                         indices, type)) {
            ret = 0;
            break;
        }

        ret = shmem_unuse_swap_entries(inode, &fbatch, indices);
        if (ret < 0)
            break;

        if (signal_pending(current)) {
            ret = -ERESTARTSYS;
            break;
        }
        start = indices[folio_batch_count(&fbatch) - 1];
    } while (true);

And shmem_unuse_swap_entries() ignores all errors from shmem_swapin_folio()
other than -ENOMEM:

    error = shmem_swapin_folio(inode, indices[i], &folio, SGP_CACHE,
                    mapping_gfp_mask(mapping), NULL, NULL);
    if (error == 0) {
        folio_unlock(folio);
        folio_put(folio);
        ret++;
    }
    if (error == -ENOMEM)
        break;
    error = 0;

When shmem_swapin_folio() returns -EBUSY on a frozen mapping without
removing the swap entry, shmem_unuse_swap_entries() resets error = 0 and
returns a non-negative value. shmem_unuse_inode() then sets
start = indices[folio_batch_count(&fbatch) - 1], and the next
shmem_find_swap_entries() call (which scans inclusively from start) finds the
same swap entry again, spinning forever in the kernel.

Should shmem_unuse_swap_entries() also propagate -EBUSY (or should
shmem_unuse_inode() bail out when mapping_frozen(mapping) is set)?

> diff --git a/mm/truncate.c b/mm/truncate.c
> index 733d2713a875a..6512f460a28e9 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -171,6 +171,8 @@ int truncate_inode_folio(struct address_space *mapping, struct folio *folio)
>  {
>  	if (folio->mapping != mapping)
>  		return -EIO;
> +	if (mapping_frozen(mapping))
> +		return -EBUSY;
>  
>  	truncate_cleanup_folio(folio);
>  	filemap_remove_folio(folio);

[Severity: High]
Could this unlocked mapping_frozen(mapping) check in truncate_inode_folio()
both race with mapping_set_frozen() and cause an infinite loop in truncation
callers?

First, callers like memory_failure() -> truncate_error_folio() ->
generic_error_remove_folio() -> truncate_inode_folio() only hold the folio
lock, not filesystem locks. Because mapping_frozen(mapping) is checked
before acquiring xa_lock_irq(&mapping->i_pages) in filemap_remove_folio(),
mapping_set_frozen() can run while truncate_cleanup_folio() is executing,
causing filemap_remove_folio() -> __filemap_remove_folio() ->
page_cache_delete() to hit VM_WARN_ON_ONCE_FOLIO(mapping_frozen(mapping),
folio) and delete the folio from the frozen mapping.

Second, the commit message notes that racing mapping_set_frozen() with
truncation causes truncation to fail mid-way, but in mm/truncate.c, the
second loop of truncate_inode_pages_range() ignores the return value of
truncate_inode_folio():

    while (index < end) {
        cond_resched();
        if (!find_get_entries(mapping, &index, end - 1, &fbatch,
                indices)) {
            /* If all gone from start onwards, we're done */
            if (index == start)
                break;
            /* Otherwise restart to make sure all gone */
            index = start;
            continue;
        }
        ...
            truncate_inode_folio(mapping, folio);
            folio_unlock(folio);
        }
        truncate_folio_batch_exceptionals(mapping, &fbatch, indices);
        folio_batch_release(&fbatch);
    }

And in mm/shmem.c, shmem_undo_range() has the same restart loop when
truncating (end == -1):

        if (!find_get_entries(mapping, &index, end - 1, &fbatch,
                indices)) {
            /* If all gone or hole-punch or unfalloc, we're done */
            if (index == start || end != -1)
                break;
            /* But if truncating, restart to make sure all gone */
            index = start;
            continue;
        }
        ...
                if (!folio_test_large(folio)) {
                    truncate_inode_folio(mapping, folio);

If AS_FROZEN is set after truncate_inode_pages_range() or shmem_undo_range()
passes its initial entry check, truncate_inode_folio() returns -EBUSY (and
truncate_inode_partial_folio() fails to split large folios with -EBUSY)
without removing the folios, while find_get_entries() advances index past
start. When find_get_entries() reaches the end of the range and returns 0
with index != start, both loops reset index = start and retry forever.

Should truncate_inode_folio() verify mapping_frozen(mapping) under
xa_lock_irq(&mapping->i_pages), and should the retry loops in
truncate_inode_pages_range() and shmem_undo_range() abort if the mapping
becomes frozen?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027173031.763213-1-pratyush@kernel.org?part=2