linux-kernel - Re: [linus:master] [btrfs] e8513c012d: addition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAL3q7H5L2Yc3aNCfffuimET6LLup9iB2T4_1caJkgV+c8avRMg@mail.gmail.com>
Date: Tue, 2 Dec 2025 15:04:51 +0000
From: Filipe Manana <fdmanana@...nel.org>
To: Oliver Sang <oliver.sang@...el.com>
Cc: Leo Martins <loemra.dev@...il.com>, oe-lkp@...ts.linux.dev, lkp@...el.com, 
	linux-kernel@...r.kernel.org, David Sterba <dsterba@...e.com>, 
	linux-btrfs@...r.kernel.org
Subject: Re: [linus:master] [btrfs] e8513c012d: addition_on#;use-after-free

On Tue, Dec 2, 2025 at 8:40 AM Oliver Sang <oliver.sang@...el.com> wrote:
>
> 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@...el.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@...el.com>
> > Closes: https://lore.kernel.org/oe-lkp/202511262228.6dda231e-lkp@intel.com
> > Signed-off-by: Leo Martins <loemra.dev@...il.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);

Why the WRITE_ONCE() change?

Can you explain in the changelog why it's being introduced?
This seems unrelated and it was not there before the commit mentioned
in the Fixes tag.

Thanks.

> >       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
>