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

["Miquel Sabaté Solà" <mssola@mssola.com>]

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