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

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

On Fri, 12 Dec 2025 09:32:06 +0000 Filipe Manana <fdmanana@kernel.org> wrote:

> On Fri, Dec 12, 2025 at 1:01 AM Leo Martins <loemra.dev@gmail.com> wrote:
> >
> > 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?
> 
> You can make it either in a v3 or as a separate patch, I don't have a
> strong preference for this.
> 
> Either way:
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> (Applies both to this, a possible v3 and a standalone patch for
> replacing GFP_ATOMIC with GFP_NOFS).
> 
> If you can't push to for-next, let me know and I can do it.

Just sent out a v3. I can't push to for-next, if you could that
would be great.

Thanks.

> 
> Thanks.
> 
> >
> > >
> > > > +
> > > >         /* 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
> > > >
> > > >

Sent using hkml (https://github.com/sjp38/hackermail)