Skip to content

Commit 83f5907

Browse files
loemrawkdave
authored andcommitted
btrfs: fix use-after-free warning in btrfs_get_or_create_delayed_node()
Previously, btrfs_get_or_create_delayed_node() set the delayed_node's refcount before acquiring the root->delayed_nodes lock. Commit e8513c0 ("btrfs: implement ref_tracker for delayed_nodes") moved 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. Without that barrier, the stores to node->refs and btrfs_inode->delayed_node may become visible out of order. Another thread can then read btrfs_inode->delayed_node and attempt to increment a refcount that hasn't been set yet, 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. Because the allocations now happen outside of the lock's critical section, they can use GFP_NOFS instead of GFP_ATOMIC. Reported-by: kernel test robot <oliver.sang@intel.com> Closes: https://lore.kernel.org/oe-lkp/202511262228.6dda231e-lkp@intel.com Fixes: e8513c0 ("btrfs: implement ref_tracker for delayed_nodes") Tested-by: kernel test robot <oliver.sang@intel.com> Reviewed-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: Leo Martins <loemra.dev@gmail.com> Signed-off-by: David Sterba <dsterba@suse.com>
1 parent 7ba0b64 commit 83f5907

1 file changed

Lines changed: 17 additions & 15 deletions

File tree

fs/btrfs/delayed-inode.c

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -152,37 +152,39 @@ static struct btrfs_delayed_node *btrfs_get_or_create_delayed_node(
152152
return ERR_PTR(-ENOMEM);
153153
btrfs_init_delayed_node(node, root, ino);
154154

155+
/* Cached in the inode and can be accessed. */
156+
refcount_set(&node->refs, 2);
157+
btrfs_delayed_node_ref_tracker_alloc(node, tracker, GFP_NOFS);
158+
btrfs_delayed_node_ref_tracker_alloc(node, &node->inode_cache_tracker, GFP_NOFS);
159+
155160
/* Allocate and reserve the slot, from now it can return a NULL from xa_load(). */
156161
ret = xa_reserve(&root->delayed_nodes, ino, GFP_NOFS);
157-
if (ret == -ENOMEM) {
158-
btrfs_delayed_node_ref_tracker_dir_exit(node);
159-
kmem_cache_free(delayed_node_cache, node);
160-
return ERR_PTR(-ENOMEM);
161-
}
162+
if (ret == -ENOMEM)
163+
goto cleanup;
164+
162165
xa_lock(&root->delayed_nodes);
163166
ptr = xa_load(&root->delayed_nodes, ino);
164167
if (ptr) {
165168
/* Somebody inserted it, go back and read it. */
166169
xa_unlock(&root->delayed_nodes);
167-
btrfs_delayed_node_ref_tracker_dir_exit(node);
168-
kmem_cache_free(delayed_node_cache, node);
169-
node = NULL;
170-
goto again;
170+
goto cleanup;
171171
}
172172
ptr = __xa_store(&root->delayed_nodes, ino, node, GFP_ATOMIC);
173173
ASSERT(xa_err(ptr) != -EINVAL);
174174
ASSERT(xa_err(ptr) != -ENOMEM);
175175
ASSERT(ptr == NULL);
176-
177-
/* Cached in the inode and can be accessed. */
178-
refcount_set(&node->refs, 2);
179-
btrfs_delayed_node_ref_tracker_alloc(node, tracker, GFP_ATOMIC);
180-
btrfs_delayed_node_ref_tracker_alloc(node, &node->inode_cache_tracker, GFP_ATOMIC);
181-
182176
btrfs_inode->delayed_node = node;
183177
xa_unlock(&root->delayed_nodes);
184178

185179
return node;
180+
cleanup:
181+
btrfs_delayed_node_ref_tracker_free(node, tracker);
182+
btrfs_delayed_node_ref_tracker_free(node, &node->inode_cache_tracker);
183+
btrfs_delayed_node_ref_tracker_dir_exit(node);
184+
kmem_cache_free(delayed_node_cache, node);
185+
if (ret)
186+
return ERR_PTR(ret);
187+
goto again;
186188
}
187189

188190
/*

0 commit comments

Comments
 (0)