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

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

[Qu Wenruo]

[CHANGELOG]
v2:
- Add back an early btrfs_release_path() call in
  print_data_reloc_error()
  This is to avoid holding the path (and its extent buffers locked)
  during time consuming iterate_extent_inodes() calls.

  And add a comment on that early release, with updated commit message.

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  |  9 +++++++++
 fs/btrfs/defrag.c |  5 +----
 fs/btrfs/inode.c  |  7 +++++--
 fs/btrfs/scrub.c  | 18 ++++++------------
 4 files changed, 21 insertions(+), 18 deletions(-)

-- 
2.52.0

[PATCH v2 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 v2 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
  There is a special case where we want to release the path early before
  the time consuming iterate_extent_inodes() call, thus that manual
  early release is kept as is, with an extra comment added.

- scrub_radi56_parity_stripe() in scrub.c

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 692370fc07b2..d5bd01c4e414 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -85,6 +85,14 @@ 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.
+ *
+ * This one is compatible with any existing manual btrfs_release_path() calls.
+ */
+#define BTRFS_PATH_AUTO_RELEASE(path_name)					\
+	struct btrfs_path path_name __free(btrfs_release_path) = { 0 }
+
 /*
  * The state of btrfs root
  */
@@ -601,6 +609,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..6ae94b7777bb 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,11 +284,14 @@ 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 };
 
+		/*
+		 * Do not hold the path as later iterate_extent_inodes() call
+		 * can be time consuming.
+		 */
 		btrfs_release_path(&path);
 
 		ctx.bytenr = found_key.objectid;
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 v2 2/2] btrfs: introduce BTRFS_PATH_AUTO_RELEASE() helper

[David Sterba]

On Wed, Nov 26, 2025 at 08:20:21AM +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.
> 
> This will applies to all 3 on-stack path usages:
> 
> - defrag_get_extent() in defrag.c
> 
> - print_data_reloc_error() in inode.c
>   There is a special case where we want to release the path early before
>   the time consuming iterate_extent_inodes() call, thus that manual
>   early release is kept as is, with an extra comment added.
> 
> - scrub_radi56_parity_stripe() in scrub.c
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ctree.h  |  9 +++++++++
>  fs/btrfs/defrag.c |  5 +----
>  fs/btrfs/inode.c  |  8 +++++---
>  fs/btrfs/scrub.c  | 18 ++++++------------
>  4 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 692370fc07b2..d5bd01c4e414 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -85,6 +85,14 @@ 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.
> + *
> + * This one is compatible with any existing manual btrfs_release_path() calls.
> + */
> +#define BTRFS_PATH_AUTO_RELEASE(path_name)					\
> +	struct btrfs_path path_name __free(btrfs_release_path) = { 0 }

For the on-stack path it makes sense though it's a bit confusing because
BTRFS_PATH_AUTO_FREE defines a pointer. It's way more common to get a
path from a parameter and then call btrfs_release_path() on the pointer,
so it's not possible to use the AUTO_RELEASE pattern anyway.

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

[Qu Wenruo]

在 2025/11/27 00:57, David Sterba 写道:
> On Wed, Nov 26, 2025 at 08:20:21AM +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.
>>
>> This will applies to all 3 on-stack path usages:
>>
>> - defrag_get_extent() in defrag.c
>>
>> - print_data_reloc_error() in inode.c
>>    There is a special case where we want to release the path early before
>>    the time consuming iterate_extent_inodes() call, thus that manual
>>    early release is kept as is, with an extra comment added.
>>
>> - scrub_radi56_parity_stripe() in scrub.c
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/ctree.h  |  9 +++++++++
>>   fs/btrfs/defrag.c |  5 +----
>>   fs/btrfs/inode.c  |  8 +++++---
>>   fs/btrfs/scrub.c  | 18 ++++++------------
>>   4 files changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 692370fc07b2..d5bd01c4e414 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -85,6 +85,14 @@ 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.
>> + *
>> + * This one is compatible with any existing manual btrfs_release_path() calls.
>> + */
>> +#define BTRFS_PATH_AUTO_RELEASE(path_name)					\
>> +	struct btrfs_path path_name __free(btrfs_release_path) = { 0 }
> 
> For the on-stack path it makes sense though it's a bit confusing because
> BTRFS_PATH_AUTO_FREE defines a pointer. It's way more common to get a
> path from a parameter and then call btrfs_release_path() on the pointer,
> so it's not possible to use the AUTO_RELEASE pattern anyway.

I can remove the last sentence during commit.

Thanks,
Qu

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

[David Sterba]

On Wed, Nov 26, 2025 at 08:20:19AM +1030, Qu Wenruo wrote:
> [CHANGELOG]
> v2:
> - Add back an early btrfs_release_path() call in
>   print_data_reloc_error()
>   This is to avoid holding the path (and its extent buffers locked)
>   during time consuming iterate_extent_inodes() calls.
> 
>   And add a comment on that early release, with updated commit message.
> 
> 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

Reviewed-by: David Sterba <dsterba@suse.com>