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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 1/12/26 1:53 AM, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> 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. As a result the order could be a stalled value
> if the entry is split after the xa_get_order and before the
> xa_cmpxchg_irq. In fact that are more way for other races to occur
> during the time window.
> 
> To fix that, open code the Xarray cmpxchg and put the order retrivial and
> value checking in the same critical section. Also ensure the order won't
> exceed the truncate border.
> 
> I observed random swapoff hangs and swap entry leaks when stress
> testing ZSWAP with shmem. After applying this patch, the problem is resolved.
> 
> Fixes: 809bc86517cc ("mm: shmem: support large folio swap out")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>   mm/shmem.c | 35 +++++++++++++++++++++++------------
>   1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 0b4c8c70d017..e160da0cd30f 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -961,18 +961,28 @@ static void shmem_delete_from_page_cache(struct folio *folio, void *radswap)
>    * the number of pages being freed. 0 means entry not found in XArray (0 pages
>    * being freed).
>    */
> -static long shmem_free_swap(struct address_space *mapping,
> -			    pgoff_t index, void *radswap)
> +static long shmem_free_swap(struct address_space *mapping, pgoff_t index,
> +			    unsigned int max_nr, void *radswap)
>   {
> -	int order = xa_get_order(&mapping->i_pages, index);
> -	void *old;
> +	XA_STATE(xas, &mapping->i_pages, index);
> +	unsigned int nr_pages = 0;
> +	void *entry;
>   
> -	old = xa_cmpxchg_irq(&mapping->i_pages, index, radswap, NULL, 0);
> -	if (old != radswap)
> -		return 0;
> -	swap_put_entries_direct(radix_to_swp_entry(radswap), 1 << order);
> +	xas_lock_irq(&xas);
> +	entry = xas_load(&xas);
> +	if (entry == radswap) {
> +		nr_pages = 1 << xas_get_order(&xas);
> +		if (index == round_down(xas.xa_index, nr_pages) && nr_pages < max_nr)
> +			xas_store(&xas, NULL);
> +		else
> +			nr_pages = 0;
> +	}
> +	xas_unlock_irq(&xas);
> +
> +	if (nr_pages)
> +		swap_put_entries_direct(radix_to_swp_entry(radswap), nr_pages);
>   
> -	return 1 << order;
> +	return nr_pages;
>   }

Thanks for the analysis, and it makes sense to me. Would the following 
implementation be simpler and also address your issue (we will not 
release the lock in __xa_cmpxchg() since gfp = 0)?

static long shmem_free_swap(struct address_space *mapping,
                             pgoff_t index, void *radswap)
{
         XA_STATE(xas, &mapping->i_pages, index);
         int order;
         void *old;

         xas_lock_irq(&xas);
         order = xas_get_order(&xas);
         old = __xa_cmpxchg(xas.xa, index, radswap, NULL, 0);
         if (old != radswap) {
                 xas_unlock_irq(&xas);
                 return 0;
         }
         xas_lock_irq(&xas);

         swap_put_entries_direct(radix_to_swp_entry(radswap), 1 << order);

         return 1 << order;
}