Re: [linus:master] [btrfs] e8513c012d: addition_on#;use-after-free

[Oliver Sang <oliver.sang@intel.com>]

hi, Leo Martins,

On Mon, Dec 01, 2025 at 04:51:41PM -0800, Leo Martins wrote:

[...]

> 
> Hello,
> 
> I believe I have identified the root cause of the warning.
> However, I'm having some troubles running the reproducer as I
> haven't setup lkp-tests yet. Could you test the patch below
> against your reproducer to see if it fixes the issue?

we confirmed your patch fixed the issues we reported in origial report. thanks!

Tested-by: kernel test robot <oliver.sang@intel.com>

> 
> ---8<---
> 
> [PATCH] btrfs: fix use-after-free in btrfs_get_or_create_delayed_node
> 
> Previously, btrfs_get_or_create_delayed_node sets the delayed_node's
> refcount before acquiring the root->delayed_nodes lock.
> Commit e8513c012de7 ("btrfs: implement ref_tracker for delayed_nodes")
> moves refcount_set inside the critical section which means
> there is no longer a memory barrier between setting the refcount and
> setting btrfs_inode->delayed_node = node.
> 
> This allows btrfs_get_or_create_delayed_node to set
> btrfs_inode->delayed_node before setting the refcount.
> A different thread is then able to read and increase the refcount
> of btrfs_inode->delayed_node leading to a refcounting bug and
> a use-after-free warning.
> 
> The fix is to move refcount_set back to where it was to take
> advantage of the implicit memory barrier provided by lock
> acquisition.
> 
> Fixes: e8513c012de7 ("btrfs: implement ref_tracker for delayed_nodes")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202511262228.6dda231e-lkp@intel.com
> Signed-off-by: Leo Martins <loemra.dev@gmail.com>
> ---
>  fs/btrfs/delayed-inode.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 364814642a91..f61f10000e33 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -152,37 +152,39 @@ static struct btrfs_delayed_node *btrfs_get_or_create_delayed_node(
>  		return ERR_PTR(-ENOMEM);
>  	btrfs_init_delayed_node(node, root, ino);
>  
> +	/* Cached in the inode and can be accessed. */
> +	refcount_set(&node->refs, 2);
> +	btrfs_delayed_node_ref_tracker_alloc(node, tracker, GFP_ATOMIC);
> +	btrfs_delayed_node_ref_tracker_alloc(node, &node->inode_cache_tracker, GFP_ATOMIC);
> +
>  	/* Allocate and reserve the slot, from now it can return a NULL from xa_load(). */
>  	ret = xa_reserve(&root->delayed_nodes, ino, GFP_NOFS);
> -	if (ret == -ENOMEM) {
> -		btrfs_delayed_node_ref_tracker_dir_exit(node);
> -		kmem_cache_free(delayed_node_cache, node);
> -		return ERR_PTR(-ENOMEM);
> -	}
> +	if (ret == -ENOMEM)
> +		goto cleanup;
> +
>  	xa_lock(&root->delayed_nodes);
>  	ptr = xa_load(&root->delayed_nodes, ino);
>  	if (ptr) {
>  		/* Somebody inserted it, go back and read it. */
>  		xa_unlock(&root->delayed_nodes);
> -		btrfs_delayed_node_ref_tracker_dir_exit(node);
> -		kmem_cache_free(delayed_node_cache, node);
> -		node = NULL;
> -		goto again;
> +		goto cleanup;
>  	}
>  	ptr = __xa_store(&root->delayed_nodes, ino, node, GFP_ATOMIC);
>  	ASSERT(xa_err(ptr) != -EINVAL);
>  	ASSERT(xa_err(ptr) != -ENOMEM);
>  	ASSERT(ptr == NULL);
> -
> -	/* Cached in the inode and can be accessed. */
> -	refcount_set(&node->refs, 2);
> -	btrfs_delayed_node_ref_tracker_alloc(node, tracker, GFP_ATOMIC);
> -	btrfs_delayed_node_ref_tracker_alloc(node, &node->inode_cache_tracker, GFP_ATOMIC);
> -
> -	btrfs_inode->delayed_node = node;
> +	WRITE_ONCE(btrfs_inode->delayed_node, node);
>  	xa_unlock(&root->delayed_nodes);
>  
>  	return node;
> +cleanup:
> +	btrfs_delayed_node_ref_tracker_free(node, tracker);
> +	btrfs_delayed_node_ref_tracker_free(node, &node->inode_cache_tracker);
> +	btrfs_delayed_node_ref_tracker_dir_exit(node);
> +	kmem_cache_free(delayed_node_cache, node);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	goto again;
>  }
>  
>  /*
> -- 
> 2.47.3