Re: [PATCH v2] btrfs: fix use-after-free warning in btrfs_get_or_create_delayed_node

[Leo Martins <loemra.dev@gmail.com>]

On Wed, 3 Dec 2025 10:50:17 +0000 Filipe Manana <fdmanana@kernel.org> wrote:

> On Tue, Dec 2, 2025 at 9:21 PM Leo Martins <loemra.dev@gmail.com> wrote:
> >
> > 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
> > Tested-by: kernel test robot <oliver.sang@intel.com>
> > Signed-off-by: Leo Martins <loemra.dev@gmail.com>
> > ---
> >  fs/btrfs/delayed-inode.c | 32 +++++++++++++++++---------------
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> > index 364814642a91..81922556379d 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);
> 
> Now that we do these allocations outside the spinlock (xarray's lock),
> we can stop using GFP_ATOMIC and use GFP_NOFS.
> 
> Otherwise it looks good, thanks.

Did you want me to send out a v3 for this change?

> 
> > +
> >         /* 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;
> >         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
> >
> >