Re: [PATCH v3] mm/shmem, swap: fix race of truncate and swap entry split

[Chris Mason]

Kairui Song <ryncsn@xxxxxxxxx> wrote:
> From: Kairui Song <kasong@xxxxxxxxxxx>
> 
> The helper for shmem swap freeing is not handling the order of swap
> entries correctly. It uses xa_cmpxchg_irq to erase the swap entry, but
> it gets the entry order before that using xa_get_order without lock
> protection, and it may get an outdated order value if the entry is split
> or changed in other ways after the xa_get_order and before the
> xa_cmpxchg_irq.
> 
> And besides, the order could grow and be larger than expected, and cause
> truncation to erase data beyond the end border. For example, if the
> target entry and following entries are swapped in or freed, then a large
> folio was added in place and swapped out, using the same entry, the
> xa_cmpxchg_irq will still succeed, it's very unlikely to happen though.
> 
> To fix that, open code the Xarray cmpxchg and put the order retrieval
> and value checking in the same critical section. Also, ensure the order
> won't exceed the end border, skip it if the entry goes across the
> border.
> 
> Skipping large swap entries crosses the end border is safe here.
> Shmem truncate iterates the range twice, in the first iteration,
> find_lock_entries already filtered such entries, and shmem will
> swapin the entries that cross the end border and partially truncate the
> folio (split the folio or at least zero part of it). So in the second
> loop here, if we see a swap entry that crosses the end order, it must
> at least have its content erased already.
> 
> I observed random swapoff hangs and kernel panics when stress testing
> ZSWAP with shmem. After applying this patch, all problems are gone.
> 

Hi everyone, another flag from running mm-next through AI review.  I'm
low confidence on this one:

> diff --git a/mm/shmem.c b/mm/shmem.c
> --- a/mm/shmem.c
> +++ b/mm/shmem.c

[ ... ]

> @@ -1191,12 +1203,23 @@
>  		folio = fbatch.folios[i];
>
>  		if (xa_is_value(folio)) {
> +			int order;
>  			long swaps_freed;
>
>  			if (unfalloc)
>  				continue;
> -			swaps_freed = shmem_free_swap(mapping, indices[i], folio);
> +			swaps_freed = shmem_free_swap(mapping, indices[i],
> +						      end - 1, folio);
>  			if (!swaps_freed) {
> +				/*
> +				 * If found a large swap entry cross the end border,
> +				 * skip it as the truncate_inode_partial_folio above
> +				 * should have at least zerod its content once.
> +				 */
> +				order = shmem_confirm_swap(mapping, indices[i],
> +							   radix_to_swp_entry(folio));
> +				if (order > 0 && indices[i] + (1 << order) > end)
> +					continue;
>  				/* Swap was replaced by page: retry */
>  				index = indices[i];
>  				break;

Could this code infinite loop if a large swap entry crosses the START
boundary instead of the END boundary?

When shmem_free_swap() returns 0, it could be either because:
(a) The entry was replaced by a page, or
(b) The entry failed the boundary check (base < index ||
    base + nr_pages - 1 > end)

The boundary check here only handles case (b) for entries crossing the
END boundary:

    if (order > 0 && indices[i] + (1 << order) > end)
        continue;

But what happens if the entry crosses the START boundary? If
find_get_entries() returns a large swap entry at indices[i] where
the entry's base (calculated as indices[i] & ~((1 << order) - 1)) is
less than the truncation start point, then shmem_free_swap() will
return 0 due to the "base < index" check. The code will then call
shmem_confirm_swap(), get the order, check if it crosses the END
boundary (which it doesn't), and retry with the same index:

    index = indices[i];
    break;

The next iteration will find the same entry again at the same index,
leading to an infinite loop. For example:

- Truncating range [18, 30]
- Large swap entry at [16, 23] (order 3, 8 pages)
- indices[i] = 18
- shmem_free_swap() sees base=16 < index=18, returns 0
- Check: 18 + 8 > 30 is false (26 <= 30)
- Retries with index=18
- Loop repeats indefinitely

Should the boundary check also handle the START case, perhaps:

    if (order > 0) {
        pgoff_t base = indices[i] & ~((1UL << order) - 1);
        if (base + (1 << order) - 1 > end || base < start)
            continue;
    }

where 'start' is preserved from before the loop?