Re: [PATCH] mm: khugepaged: free xarray nodes left behind by failed collapse_file()

[Jinjiang Tu <tujinjiang@huawei.com>]

在 2026/6/16 23:29, Matthew Wilcox 写道:
> On Tue, Jun 16, 2026 at 10:54:13AM -0400, Rik van Riel wrote:
>> syzkaller reproduces it trivially with MADV_COLLAPSE on a sparse shmem
>> mapping (collapse aborts with SCAN_TRUNCATED because the range is empty),
>> and also via slab fault injection, which forces xas_create_range() down
>> the xas_nomem() path before the same abort.  kmemleak then reports the
>> 576-byte struct xa_node objects allocated in xas_alloc()/xas_nomem().
> I think the ways to produce this problem are sufficiently rare/unlikely

I can reproduce it easily with following steps. [1]
1) create file /tmp/test_madvise_collapse and ftruncate to 4MB size, and
then mmap the file
2) memset for the first 2MB
3) madvise(MADV_COLLAPSE) for the second 2MB
4) unlink the file

This might be the only path easy to trigger; all other failure paths seem to be difficult to trigger.

[1] https://lore.kernel.org/all/20260121062243.1893129-1-tujinjiang@huawei.com/

> to not merit this level of cleanup.  Why don't we defer it to
> clear_inode() instead?
>
> reasons not to:
>
> 1. the page cache is not the only user of xarrays and
> theoretically either of these things could happen elsewhere
>
> 2. can a user do this on purpose to screw other users over?  i don't
> think so, any more than they can dos the system by bringing a lot of
> inodes into memory; we have cgroups and ulimits to protect against that
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 6a3cbc7dcd28..d92c5a296504 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -779,21 +779,20 @@ void clear_inode(struct inode *inode)
>   		fsverity_cleanup_inode(inode);
>   
>   	/*
> -	 * We have to cycle the i_pages lock here because reclaim can be in the
> -	 * process of removing the last page (in __filemap_remove_folio())
> -	 * and we must not free the mapping under it.
> +	 * We have to cycle the i_pages lock here because reclaim
> +	 * can be in the process of removing the last page (in
> +	 * __filemap_remove_folio()) and we must not free the mapping
> +	 * under it.  We also remove nodes which are empty; these
> +	 * can occur in two different ways.  The first is that radix
> +	 * tree expansion can fail partway and the second is that THP
> +	 * collapse_file() can allocate some temporary nodes and not
> +	 * clean them up.
>   	 */
>   	xa_lock_irq(&inode->i_data.i_pages);
>   	BUG_ON(inode->i_data.nrpages);
> -	/*
> -	 * Almost always, mapping_empty(&inode->i_data) here; but there are
> -	 * two known and long-standing ways in which nodes may get left behind
> -	 * (when deep radix-tree node allocation failed partway; or when THP
> -	 * collapse_file() failed). Until those two known cases are cleaned up,
> -	 * or a cleanup function is called here, do not BUG_ON(!mapping_empty),
> -	 * nor even WARN_ON(!mapping_empty).
> -	 */
> +	__xa_destroy(&inode->i_data.i_pages);
>   	xa_unlock_irq(&inode->i_data.i_pages);
> +
>   	BUG_ON(!(inode_state_read_once(inode) & I_FREEING));
>   	BUG_ON(inode_state_read_once(inode) & I_CLEAR);
>   	BUG_ON(!list_empty(&inode->i_wb_list));
> diff --git a/include/linux/xarray.h b/include/linux/xarray.h
> index be850174e802..d776a6e9ad18 100644
> --- a/include/linux/xarray.h
> +++ b/include/linux/xarray.h
> @@ -571,6 +571,7 @@ int __must_check __xa_alloc_cyclic(struct xarray *, u32 *id, void *entry,
>   		struct xa_limit, u32 *next, gfp_t);
>   void __xa_set_mark(struct xarray *, unsigned long index, xa_mark_t);
>   void __xa_clear_mark(struct xarray *, unsigned long index, xa_mark_t);
> +void __xa_destroy(struct xarray *);
>   
>   /**
>    * xa_store_bh() - Store this entry in the XArray.
> diff --git a/lib/xarray.c b/lib/xarray.c
> index 9a8b4916540c..ee2459ecdc9b 100644
> --- a/lib/xarray.c
> +++ b/lib/xarray.c
> @@ -2370,6 +2370,21 @@ void xa_delete_node(struct xa_node *node, xa_update_node_t update)
>   }
>   EXPORT_SYMBOL_GPL(xa_delete_node);	/* For the benefit of the test suite */
>   
> +void __xa_destroy(struct xarray *xa)
> +{
> +	XA_STATE(xas, xa, 0);
> +	void *entry;
> +
> +	xas.xa_node = NULL;
> +	entry = xa_head_locked(xa);
> +	RCU_INIT_POINTER(xa->xa_head, NULL);
> +	xas_init_marks(&xas);
> +	if (xa_zero_busy(xa))
> +		xa_mark_clear(xa, XA_FREE_MARK);
> +	if (xa_is_node(entry))
> +		xas_free_nodes(&xas, xa_to_node(entry));
> +}
> +
>   /**
>    * xa_destroy() - Free all internal data structures.
>    * @xa: XArray.
> @@ -2382,21 +2397,11 @@ EXPORT_SYMBOL_GPL(xa_delete_node);	/* For the benefit of the test suite */
>    */
>   void xa_destroy(struct xarray *xa)
>   {
> -	XA_STATE(xas, xa, 0);
>   	unsigned long flags;
> -	void *entry;
>   
> -	xas.xa_node = NULL;
> -	xas_lock_irqsave(&xas, flags);
> -	entry = xa_head_locked(xa);
> -	RCU_INIT_POINTER(xa->xa_head, NULL);
> -	xas_init_marks(&xas);
> -	if (xa_zero_busy(xa))
> -		xa_mark_clear(xa, XA_FREE_MARK);
> -	/* lockdep checks we're still holding the lock in xas_free_nodes() */
> -	if (xa_is_node(entry))
> -		xas_free_nodes(&xas, xa_to_node(entry));
> -	xas_unlock_irqrestore(&xas, flags);
> +	xa_lock_irqsave(xa, flags);
> +	__xa_destroy(xa);
> +	xa_unlock_irqrestore(xa, flags);
>   }
>   EXPORT_SYMBOL(xa_destroy);
>   
>