[PATCH] btrfs: fix memory leak when rejecting a non SINGLE data profile without an RST

[PATCH] btrfs: fix memory leak when rejecting a non SINGLE data profile without an RST

[Miquel Sabaté Solà]

At the end of btrfs_load_block_group_zone_info() the first thing we do
is to ensure that if the mapping type is not a SINGLE one and there is
no RAID stripe tree, then we return early with an error.

Doing that, though, prevents the code from running the last calls from
this function which are about freeing memory allocated during its
run. Hence, in this case, instead of returning early go to the freeing
section of this function and leave then.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
---
 fs/btrfs/zoned.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index e3341a84f4ab..b0f5d61dbfd2 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1753,7 +1753,8 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
 	    !fs_info->stripe_root) {
 		btrfs_err(fs_info, "zoned: data %s needs raid-stripe-tree",
 			  btrfs_bg_type_to_raid_name(map->type));
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_free;
 	}
 
 	if (unlikely(cache->alloc_offset > cache->zone_capacity)) {
@@ -1785,6 +1786,8 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
 		btrfs_free_chunk_map(cache->physical_map);
 		cache->physical_map = NULL;
 	}
+
+out_free:
 	bitmap_free(active);
 	kfree(zone_info);
 
-- 
2.51.0

Re: [PATCH] btrfs: fix memory leak when rejecting a non SINGLE data profile without an RST

[Johannes Thumshirn]

On 10/7/25 7:55 AM, Miquel Sabaté Solà wrote:
> At the end of btrfs_load_block_group_zone_info() the first thing we do
> is to ensure that if the mapping type is not a SINGLE one and there is
> no RAID stripe tree, then we return early with an error.
>
> Doing that, though, prevents the code from running the last calls from
> this function which are about freeing memory allocated during its
> run. Hence, in this case, instead of returning early go to the freeing
> section of this function and leave then.
>
> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
> ---
>   fs/btrfs/zoned.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index e3341a84f4ab..b0f5d61dbfd2 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1753,7 +1753,8 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>   	    !fs_info->stripe_root) {
>   		btrfs_err(fs_info, "zoned: data %s needs raid-stripe-tree",
>   			  btrfs_bg_type_to_raid_name(map->type));
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out_free;
>   	}
>   
>   	if (unlikely(cache->alloc_offset > cache->zone_capacity)) {
> @@ -1785,6 +1786,8 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>   		btrfs_free_chunk_map(cache->physical_map);
>   		cache->physical_map = NULL;
>   	}
> +
> +out_free:
>   	bitmap_free(active);
>   	kfree(zone_info);
>   

Wouldn't it make more sense to only set "ret = -EINVAL" and run the rest 
of the functions cleanup? I.e. with your patch the chunk_map isn't freed 
as well.

Re: [PATCH] btrfs: fix memory leak when rejecting a non SINGLE data profile without an RST

[Miquel Sabaté Solà]

[-- Attachment #1: Type: text/plain, Size: 2869 bytes --]

Johannes Thumshirn @ 2025-10-07 10:13 GMT:

> On 10/7/25 7:55 AM, Miquel Sabaté Solà wrote:
>> At the end of btrfs_load_block_group_zone_info() the first thing we do
>> is to ensure that if the mapping type is not a SINGLE one and there is
>> no RAID stripe tree, then we return early with an error.
>>
>> Doing that, though, prevents the code from running the last calls from
>> this function which are about freeing memory allocated during its
>> run. Hence, in this case, instead of returning early go to the freeing
>> section of this function and leave then.
>>
>> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
>> ---
>>   fs/btrfs/zoned.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
>> index e3341a84f4ab..b0f5d61dbfd2 100644
>> --- a/fs/btrfs/zoned.c
>> +++ b/fs/btrfs/zoned.c
>> @@ -1753,7 +1753,8 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>>   	    !fs_info->stripe_root) {
>>   		btrfs_err(fs_info, "zoned: data %s needs raid-stripe-tree",
>>   			  btrfs_bg_type_to_raid_name(map->type));
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +		goto out_free;
>>   	}
>>
>>   	if (unlikely(cache->alloc_offset > cache->zone_capacity)) {
>> @@ -1785,6 +1786,8 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>>   		btrfs_free_chunk_map(cache->physical_map);
>>   		cache->physical_map = NULL;
>>   	}
>> +
>> +out_free:
>>   	bitmap_free(active);
>>   	kfree(zone_info);
>>
>
> Wouldn't it make more sense to only set "ret = -EINVAL" and run the rest
> of the functions cleanup? I.e. with your patch the chunk_map isn't freed
> as well.

The short answer is that I wanted to keep the patch as minimal as
possible while preserving the intent of the original code. From the
original code (see commit 5906333cc4af ("btrfs: zoned: don't skip block
group profile checks on conventional zones")), I get that the intent was
to return as early as possible, so to not go through all the if
statements below as they were not relevant on that case (that is, not
just the one you mention where the cache->physical_map is
freed). Falling through as you suggest would go into these if/else
blocks, which I don't think is what we want to do.

But it still sounds good that we should probably also free the chunk map
as you say. Hence, maybe we could move the new "out_free:" label before
the `if (!ret)` block right above where I've put it now. This way we
ensure that the chunk map is freed, and we avoid going through the other
if/else blocks which the aforementioned commit wanted to avoid.

As a last note, maybe for v2 I should add:

Fixes: 5906333cc4af ("btrfs: zoned: don't skip block group profile checks on conventional zones")

Thanks,
Miquel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH] btrfs: fix memory leak when rejecting a non SINGLE data profile without an RST

[Miquel Sabaté Solà]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

Johannes Thumshirn @ 2025-10-07 11:21 GMT:

> On 10/7/25 1:05 PM, Miquel Sabaté Solà wrote:
>
>  Johannes Thumshirn @ 2025-10-07 10:13 GMT:
>
>
>
> Wouldn't it make more sense to only set "ret = -EINVAL" and run the rest
> of the functions cleanup? I.e. with your patch the chunk_map isn't freed
> as well.
>
>
> The short answer is that I wanted to keep the patch as minimal as
> possible while preserving the intent of the original code. From the
> original code (see commit 5906333cc4af ("btrfs: zoned: don't skip block
> group profile checks on conventional zones")), I get that the intent was
> to return as early as possible, so to not go through all the if
> statements below as they were not relevant on that case (that is, not
> just the one you mention where the cache->physical_map is
> freed). Falling through as you suggest would go into these if/else
> blocks, which I don't think is what we want to do.
>
> But it still sounds good that we should probably also free the chunk map
> as you say. Hence, maybe we could move the new "out_free:" label before
> the `if (!ret)` block right above where I've put it now. This way we
> ensure that the chunk map is freed, and we avoid going through the other
> if/else blocks which the aforementioned commit wanted to avoid.
>
> No it really should only be ret = -EINVAL without any new labels AFAICS.
>
> 1)  the alloc_offset vs zone_capacity check is still usable

I don't think so as the ret value will be changed from -EINVAL (as set
from the previous if block) to -EIO. I believe that the intent from the
aforementioned commit was to return an -EINVAL on this case.

Maybe the reviewers from commit 5906333cc4af ("btrfs: zoned: don't skip
block group profile checks on conventional zones") can shed some light
into this...

>
> 2) the check if we're post the WP is skipped as ret != 0
>
> 3) we're hitting the else path and freeing the chunk map.
>
>  As a last note, maybe for v2 I should add:
>
> Fixes: 5906333cc4af ("btrfs: zoned: don't skip block group profile checks on conventional zones")
>
> Correct

My email client detected your email as an HTML one. Is that so? Just as
a heads up for others as the reply format might look a bit funky :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH] btrfs: fix memory leak when rejecting a non SINGLE data profile without an RST

[Johannes Thumshirn]

On 10/7/25 2:04 PM, Miquel Sabaté Solà wrote:
> Johannes Thumshirn @ 2025-10-07 11:21 GMT:
>
>> On 10/7/25 1:05 PM, Miquel Sabaté Solà wrote:
>>
>>   Johannes Thumshirn @ 2025-10-07 10:13 GMT:
>>
>>
>>
>> Wouldn't it make more sense to only set "ret = -EINVAL" and run the rest
>> of the functions cleanup? I.e. with your patch the chunk_map isn't freed
>> as well.
>>
>>
>> The short answer is that I wanted to keep the patch as minimal as
>> possible while preserving the intent of the original code. From the
>> original code (see commit 5906333cc4af ("btrfs: zoned: don't skip block
>> group profile checks on conventional zones")), I get that the intent was
>> to return as early as possible, so to not go through all the if
>> statements below as they were not relevant on that case (that is, not
>> just the one you mention where the cache->physical_map is
>> freed). Falling through as you suggest would go into these if/else
>> blocks, which I don't think is what we want to do.
>>
>> But it still sounds good that we should probably also free the chunk map
>> as you say. Hence, maybe we could move the new "out_free:" label before
>> the `if (!ret)` block right above where I've put it now. This way we
>> ensure that the chunk map is freed, and we avoid going through the other
>> if/else blocks which the aforementioned commit wanted to avoid.
>>
>> No it really should only be ret = -EINVAL without any new labels AFAICS.
>>
>> 1)  the alloc_offset vs zone_capacity check is still usable
> I don't think so as the ret value will be changed from -EINVAL (as set
> from the previous if block) to -EIO. I believe that the intent from the
> aforementioned commit was to return an -EINVAL on this case.

I don't think this matters at all as there's still the error print above 
(for both cases).

And the check is very valuable, as it shows a discrepancy between the on 
disk representation and the in memory representation.

Especially a WP mismatch on a zoned drive is fatal.

> Maybe the reviewers from commit 5906333cc4af ("btrfs: zoned: don't skip
> block group profile checks on conventional zones") can shed some light
> into this...
>
>> 2) the check if we're post the WP is skipped as ret != 0
>>
>> 3) we're hitting the else path and freeing the chunk map.
>>
>>   As a last note, maybe for v2 I should add:
>>
>> Fixes: 5906333cc4af ("btrfs: zoned: don't skip block group profile checks on conventional zones")
>>
>> Correct
> My email client detected your email as an HTML one. Is that so? Just as
> a heads up for others as the reply format might look a bit funky :/

Yeah my mailsetup is a bit funky these days, I'm sorry.