[PATCH 0/2] btrfs: fix an on-stack path leak and migrate to auto-release for on-stack paths

[PATCH 0/2] btrfs: fix an on-stack path leak and migrate to auto-release for on-stack paths

[Qu Wenruo]

I thought patch "btrfs: make sure extent and csum paths are always released in
scrub_raid56_parity_stripe()" has already taught us that tag based
manual cleanup is never reliable, now there is another similar bug in
print_data_reloc_error().

This time it is harder to expose, as we always imply if the function
returned an error, they should do the proper cleanup.
But extent_to_logical() does not follow that assumption.

The first patch is the minimal fix for backport, the second patch is
going to solve the problem by using auto-release for all on-stack btrfs
paths.

Qu Wenruo (2):
  btrfs: fix a potential path leak in print_data_reloc_error()
  btrfs: introduce BTRFS_PATH_AUTO_RELEASE() helper

 fs/btrfs/ctree.h  |  5 +++++
 fs/btrfs/defrag.c |  5 +----
 fs/btrfs/inode.c  |  5 +----
 fs/btrfs/scrub.c  | 18 ++++++------------
 4 files changed, 13 insertions(+), 20 deletions(-)

-- 
2.52.0

[PATCH 1/2] btrfs: fix a potential path leak in print_data_reloc_error()

[Qu Wenruo]

Inside print_data_reloc_error(), if extent_from_logical() failed we
return immediately.

However there are the following cases where extent_from_logical() can
return error but still holds a path:

- btrfs_search_slot() returned 0

- No backref item found in extent tree

- No @flags_ret provided
  This is not possible in this call site though.

So for the above two cases, we can return without releasing the path,
causing extent buffer leaks.

Fixes: b9a9a85059cd ("btrfs: output affected files when relocation fails")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3cf30abcdb08..46b5d29f7e29 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -255,6 +255,7 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
 	if (ret < 0) {
 		btrfs_err_rl(fs_info, "failed to lookup extent item for logical %llu: %d",
 			     logical, ret);
+		btrfs_release_path(&path);
 		return;
 	}
 	eb = path.nodes[0];
-- 
2.52.0

[PATCH 2/2] btrfs: introduce BTRFS_PATH_AUTO_RELEASE() helper

[Qu Wenruo]

There are already several bugs with on-stack btrfs_path involved, even
it is already a little safer than btrfs_path pointers (only leaks the
extent buffers, not the btrfs_path structure itself)

- Patch "btrfs: make sure extent and csum paths are always released in
  scrub_raid56_parity_stripe()"
  Which is going into v6.19 release cycle.

- Patch "btrfs: fix a potential path leak in print_datA_reloc_error()"
  The previous patch in the series.

Thus there is a real need to apply auto release for those on-stack paths.

This patch introduces a new macro, BTRFS_PATH_AUTO_RELEASE(), which
defines one on-stack btrfs_path structure, initialize it all to 0, then
call btrfs_release_path() on it when exiting the scope.

This will applies to all 3 on-stack path usages:

- defrag_get_extent() in defrag.c

- print_data_reloc_error() in inode.c

- scrub_radi56_parity_stripe() in scrub.c

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h  |  5 +++++
 fs/btrfs/defrag.c |  5 +----
 fs/btrfs/inode.c  |  6 +-----
 fs/btrfs/scrub.c  | 18 ++++++------------
 4 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 692370fc07b2..4ab27673bd9c 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -85,6 +85,10 @@ struct btrfs_path {
 #define BTRFS_PATH_AUTO_FREE(path_name)					\
 	struct btrfs_path *path_name __free(btrfs_free_path) = NULL
 
+/* This defines an on-stack path that will be auto released exiting the scope. */
+#define BTRFS_PATH_AUTO_RELEASE(path_name)					\
+	struct btrfs_path path_name __free(btrfs_release_path) = { 0 }
+
 /*
  * The state of btrfs root
  */
@@ -601,6 +605,7 @@ void btrfs_release_path(struct btrfs_path *p);
 struct btrfs_path *btrfs_alloc_path(void);
 void btrfs_free_path(struct btrfs_path *p);
 DEFINE_FREE(btrfs_free_path, struct btrfs_path *, btrfs_free_path(_T))
+DEFINE_FREE(btrfs_release_path, struct btrfs_path, btrfs_release_path(&_T))
 
 int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		   struct btrfs_path *path, int slot, int nr);
diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
index 2e3c011d410a..02a2e7736da0 100644
--- a/fs/btrfs/defrag.c
+++ b/fs/btrfs/defrag.c
@@ -610,7 +610,7 @@ static struct extent_map *defrag_get_extent(struct btrfs_inode *inode,
 {
 	struct btrfs_root *root = inode->root;
 	struct btrfs_file_extent_item *fi;
-	struct btrfs_path path = { 0 };
+	BTRFS_PATH_AUTO_RELEASE(path);
 	struct extent_map *em;
 	struct btrfs_key key;
 	u64 ino = btrfs_ino(inode);
@@ -721,16 +721,13 @@ static struct extent_map *defrag_get_extent(struct btrfs_inode *inode,
 		if (ret > 0)
 			goto not_found;
 	}
-	btrfs_release_path(&path);
 	return em;
 
 not_found:
-	btrfs_release_path(&path);
 	btrfs_free_extent_map(em);
 	return NULL;
 
 err:
-	btrfs_release_path(&path);
 	btrfs_free_extent_map(em);
 	return ERR_PTR(ret);
 }
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 46b5d29f7e29..eaf6179692b3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -217,7 +217,7 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
 				   int mirror_num)
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
-	struct btrfs_path path = { 0 };
+	BTRFS_PATH_AUTO_RELEASE(path);
 	struct btrfs_key found_key = { 0 };
 	struct extent_buffer *eb;
 	struct btrfs_extent_item *ei;
@@ -255,7 +255,6 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
 	if (ret < 0) {
 		btrfs_err_rl(fs_info, "failed to lookup extent item for logical %llu: %d",
 			     logical, ret);
-		btrfs_release_path(&path);
 		return;
 	}
 	eb = path.nodes[0];
@@ -285,13 +284,10 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
 				(ref_level ? "node" : "leaf"),
 				ref_level, ref_root);
 		}
-		btrfs_release_path(&path);
 	} else {
 		struct btrfs_backref_walk_ctx ctx = { 0 };
 		struct data_reloc_warn reloc_warn = { 0 };
 
-		btrfs_release_path(&path);
-
 		ctx.bytenr = found_key.objectid;
 		ctx.extent_item_pos = logical - found_key.objectid;
 		ctx.fs_info = fs_info;
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 41ead4a929b0..de09cfeb3bf1 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2173,8 +2173,8 @@ static int scrub_raid56_parity_stripe(struct scrub_ctx *sctx,
 				      u64 full_stripe_start)
 {
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
-	struct btrfs_path extent_path = { 0 };
-	struct btrfs_path csum_path = { 0 };
+	BTRFS_PATH_AUTO_RELEASE(extent_path);
+	BTRFS_PATH_AUTO_RELEASE(csum_path);
 	struct scrub_stripe *stripe;
 	bool all_empty = true;
 	const int data_stripes = nr_data_stripes(map);
@@ -2226,7 +2226,7 @@ static int scrub_raid56_parity_stripe(struct scrub_ctx *sctx,
 				full_stripe_start + btrfs_stripe_nr_to_offset(i),
 				BTRFS_STRIPE_LEN, stripe);
 		if (ret < 0)
-			goto out;
+			return ret;
 		/*
 		 * No extent in this data stripe, need to manually mark them
 		 * initialized to make later read submission happy.
@@ -2248,10 +2248,8 @@ static int scrub_raid56_parity_stripe(struct scrub_ctx *sctx,
 			break;
 		}
 	}
-	if (all_empty) {
-		ret = 0;
-		goto out;
-	}
+	if (all_empty)
+		return 0;
 
 	for (int i = 0; i < data_stripes; i++) {
 		stripe = &sctx->raid56_data_stripes[i];
@@ -2292,8 +2290,7 @@ static int scrub_raid56_parity_stripe(struct scrub_ctx *sctx,
 "scrub: unrepaired sectors detected, full stripe %llu data stripe %u errors %*pbl",
 				  full_stripe_start, i, stripe->nr_sectors,
 				  &error);
-			ret = -EIO;
-			goto out;
+			return ret;
 		}
 		bitmap_or(&extent_bitmap, &extent_bitmap, &has_extent,
 			  stripe->nr_sectors);
@@ -2302,9 +2299,6 @@ static int scrub_raid56_parity_stripe(struct scrub_ctx *sctx,
 	/* Now we can check and regenerate the P/Q stripe. */
 	ret = scrub_raid56_cached_parity(sctx, scrub_dev, map, full_stripe_start,
 					 &extent_bitmap);
-out:
-	btrfs_release_path(&extent_path);
-	btrfs_release_path(&csum_path);
 	return ret;
 }
 
-- 
2.52.0

Re: [PATCH 2/2] btrfs: introduce BTRFS_PATH_AUTO_RELEASE() helper

[David Sterba]

On Tue, Nov 25, 2025 at 06:49:57PM +1030, Qu Wenruo wrote:
> There are already several bugs with on-stack btrfs_path involved, even
> it is already a little safer than btrfs_path pointers (only leaks the
> extent buffers, not the btrfs_path structure itself)
> 
> - Patch "btrfs: make sure extent and csum paths are always released in
>   scrub_raid56_parity_stripe()"
>   Which is going into v6.19 release cycle.
> 
> - Patch "btrfs: fix a potential path leak in print_datA_reloc_error()"
>   The previous patch in the series.
> 
> Thus there is a real need to apply auto release for those on-stack paths.
> 
> This patch introduces a new macro, BTRFS_PATH_AUTO_RELEASE(), which
> defines one on-stack btrfs_path structure, initialize it all to 0, then
> call btrfs_release_path() on it when exiting the scope.

Ok this makes sense. Similar to the path freeing, there should be almost
no code following the expected place of freeing/releasing. If the path
is locked then delaying the cleanup holds the locks. There are relese
calls that will act as an unlock so they should stay, one example below.

Otherwise the hint for auto freeing/releasing is "right before return".

> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -217,7 +217,7 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
>  				   int mirror_num)
>  {
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> -	struct btrfs_path path = { 0 };
> +	BTRFS_PATH_AUTO_RELEASE(path);
>  	struct btrfs_key found_key = { 0 };
>  	struct extent_buffer *eb;
>  	struct btrfs_extent_item *ei;
> @@ -255,7 +255,6 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
>  	if (ret < 0) {
>  		btrfs_err_rl(fs_info, "failed to lookup extent item for logical %llu: %d",
>  			     logical, ret);
> -		btrfs_release_path(&path);
>  		return;
>  	}
>  	eb = path.nodes[0];
> @@ -285,13 +284,10 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
>  				(ref_level ? "node" : "leaf"),
>  				ref_level, ref_root);
>  		}
> -		btrfs_release_path(&path);
>  	} else {
>  		struct btrfs_backref_walk_ctx ctx = { 0 };
>  		struct data_reloc_warn reloc_warn = { 0 };
>  
> -		btrfs_release_path(&path);

The path should be released here because there's iterate_extent_inodes()
which can potentially take long due to the iteration.

> -
>  		ctx.bytenr = found_key.objectid;
>  		ctx.extent_item_pos = logical - found_key.objectid;
>  		ctx.fs_info = fs_info;

Re: [PATCH 2/2] btrfs: introduce BTRFS_PATH_AUTO_RELEASE() helper

[David Sterba]

On Tue, Nov 25, 2025 at 01:26:06PM +0100, David Sterba wrote:
> On Tue, Nov 25, 2025 at 06:49:57PM +1030, Qu Wenruo wrote:
> > There are already several bugs with on-stack btrfs_path involved, even
> > it is already a little safer than btrfs_path pointers (only leaks the
> > extent buffers, not the btrfs_path structure itself)
> > 
> > - Patch "btrfs: make sure extent and csum paths are always released in
> >   scrub_raid56_parity_stripe()"
> >   Which is going into v6.19 release cycle.
> > 
> > - Patch "btrfs: fix a potential path leak in print_datA_reloc_error()"
> >   The previous patch in the series.
> > 
> > Thus there is a real need to apply auto release for those on-stack paths.
> > 
> > This patch introduces a new macro, BTRFS_PATH_AUTO_RELEASE(), which
> > defines one on-stack btrfs_path structure, initialize it all to 0, then
> > call btrfs_release_path() on it when exiting the scope.
> 
> Ok this makes sense. Similar to the path freeing, there should be almost
> no code following the expected place of freeing/releasing. If the path
> is locked then delaying the cleanup holds the locks. There are relese
> calls that will act as an unlock so they should stay, one example below.

One example where a path release (either direct or from inside path
free) is crucial is e110f8911ddb ("btrfs: fix lockdep splat and
potential deadlock after failure running delayed items"). So removing
them from the middle of code should be careful.

> Otherwise the hint for auto freeing/releasing is "right before return".

With the above let me refine that a bit: use auto path
release/free before the 'return' when there's no code that depends on
the path and related extent buffers, or the remainging code is lighweight.

> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -217,7 +217,7 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
> >  				   int mirror_num)
> >  {
> >  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> > -	struct btrfs_path path = { 0 };
> > +	BTRFS_PATH_AUTO_RELEASE(path);
> >  	struct btrfs_key found_key = { 0 };
> >  	struct extent_buffer *eb;
> >  	struct btrfs_extent_item *ei;
> > @@ -255,7 +255,6 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
> >  	if (ret < 0) {
> >  		btrfs_err_rl(fs_info, "failed to lookup extent item for logical %llu: %d",
> >  			     logical, ret);
> > -		btrfs_release_path(&path);
> >  		return;
> >  	}
> >  	eb = path.nodes[0];
> > @@ -285,13 +284,10 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
> >  				(ref_level ? "node" : "leaf"),
> >  				ref_level, ref_root);
> >  		}
> > -		btrfs_release_path(&path);
> >  	} else {
> >  		struct btrfs_backref_walk_ctx ctx = { 0 };
> >  		struct data_reloc_warn reloc_warn = { 0 };
> >  
> > -		btrfs_release_path(&path);
> 
> The path should be released here because there's iterate_extent_inodes()
> which can potentially take long due to the iteration.

It would be good to add a comment why the release is there, especially
when the function is using the auto release so it's not considered a
mistake.

In many other cases no comment is needed as the general rule is to free
the path right after it's not needed.

Re: [PATCH 2/2] btrfs: introduce BTRFS_PATH_AUTO_RELEASE() helper

[Qu Wenruo]

在 2025/11/26 01:21, David Sterba 写道:
> On Tue, Nov 25, 2025 at 01:26:06PM +0100, David Sterba wrote:
>> On Tue, Nov 25, 2025 at 06:49:57PM +1030, Qu Wenruo wrote:
>>> There are already several bugs with on-stack btrfs_path involved, even
>>> it is already a little safer than btrfs_path pointers (only leaks the
>>> extent buffers, not the btrfs_path structure itself)
>>>
>>> - Patch "btrfs: make sure extent and csum paths are always released in
>>>    scrub_raid56_parity_stripe()"
>>>    Which is going into v6.19 release cycle.
>>>
>>> - Patch "btrfs: fix a potential path leak in print_datA_reloc_error()"
>>>    The previous patch in the series.
>>>
>>> Thus there is a real need to apply auto release for those on-stack paths.
>>>
>>> This patch introduces a new macro, BTRFS_PATH_AUTO_RELEASE(), which
>>> defines one on-stack btrfs_path structure, initialize it all to 0, then
>>> call btrfs_release_path() on it when exiting the scope.
>>
>> Ok this makes sense. Similar to the path freeing, there should be almost
>> no code following the expected place of freeing/releasing. If the path
>> is locked then delaying the cleanup holds the locks. There are relese
>> calls that will act as an unlock so they should stay, one example below.

Yeah, I missed a location where the early release has its meanings.

Thankfully the auto-release here (not auto-free) is safe against any 
existing manual release.

So we should treat the auto-release usage as an extra safenet, and be 
more carefully removing the existing manual release.

> 
> One example where a path release (either direct or from inside path
> free) is crucial is e110f8911ddb ("btrfs: fix lockdep splat and
> potential deadlock after failure running delayed items"). So removing
> them from the middle of code should be careful.
> 
>> Otherwise the hint for auto freeing/releasing is "right before return".
> 
> With the above let me refine that a bit: use auto path
> release/free before the 'return' when there's no code that depends on
> the path and related extent buffers, or the remainging code is lighweight.

I'd say the hint should be, use it everywhere unless required otherwise, 
and review if the existing release can be safely removed.
At least for this AUTO_RELEASE behavior.

Unlike AUTO_FREE, we can mix auto-release and existing release safely,
while we can not mix the existing freeing code with auto-free, as the 
existing code won't re-set the path pointer to NULL after freeing, can 
lead to double freeing.


But the current objective of RAII and scope-based auto-cleanup are just 
to ensure we never miss a release.
There will be some quirks as you exposed and the auto-free problem I 
mentioned, but at least it would be easier to review than catching the 
original missing release/free.


For the long run I hope we can refine our structure lifespan design to 
be better, so it can be better suited for something like this (ignore 
the ret_guard() naming, and put aside your hate against RAII):

	ret_guard(btrfs_new_path, path = btrfs_search_slot_new_path())
	{
		/*
		 * Where the lifespan of the path is only inside the
		 * the code block, and get auto-freed exiting the scope.
		 */
		do_something(path);
	}
	if (IS_ERR(path)) {
		/* Error handling, no @path involved. */
	}

Although this is against our current error-handling-first coding style, 
and not all callsites should be replaced (e.g. callsites using 
pre-allocated path).

Thanks,
Qu


> 
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -217,7 +217,7 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
>>>   				   int mirror_num)
>>>   {
>>>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>>> -	struct btrfs_path path = { 0 };
>>> +	BTRFS_PATH_AUTO_RELEASE(path);
>>>   	struct btrfs_key found_key = { 0 };
>>>   	struct extent_buffer *eb;
>>>   	struct btrfs_extent_item *ei;
>>> @@ -255,7 +255,6 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
>>>   	if (ret < 0) {
>>>   		btrfs_err_rl(fs_info, "failed to lookup extent item for logical %llu: %d",
>>>   			     logical, ret);
>>> -		btrfs_release_path(&path);
>>>   		return;
>>>   	}
>>>   	eb = path.nodes[0];
>>> @@ -285,13 +284,10 @@ static void print_data_reloc_error(const struct btrfs_inode *inode, u64 file_off
>>>   				(ref_level ? "node" : "leaf"),
>>>   				ref_level, ref_root);
>>>   		}
>>> -		btrfs_release_path(&path);
>>>   	} else {
>>>   		struct btrfs_backref_walk_ctx ctx = { 0 };
>>>   		struct data_reloc_warn reloc_warn = { 0 };
>>>   
>>> -		btrfs_release_path(&path);
>>
>> The path should be released here because there's iterate_extent_inodes()
>> which can potentially take long due to the iteration.
> 
> It would be good to add a comment why the release is there, especially
> when the function is using the auto release so it's not considered a
> mistake.
> 
> In many other cases no comment is needed as the general rule is to free
> the path right after it's not needed.
>