Re: [PATCH 1/2] block: fix zones_cond memory leak in blk_revalidate_disk_zones()

[Damien Le Moal <dlemoal@kernel.org>]

On 3/31/26 17:47, Jackie Liu wrote:
> From: Jackie Liu <liuyun01@kylinos.cn>
> 
> Fix memory leaks of args.zones_cond allocated in
> disk_revalidate_zone_resources() on multiple error paths:
> 
> 1) When disk_revalidate_zone_resources() itself fails (e.g.
>    disk_alloc_zone_resources() returns an error), blk_revalidate_disk_zones()
>    returns directly without freeing args.zones_cond.
> 
> 2) When report_zones() fails or the capacity check fails,
>    disk_free_zone_resources() only frees the old disk->zones_cond, not
>    the newly allocated args.zones_cond.
> 
> 3) When the nr_conv_zones validation fails in disk_update_zone_resources(),
>    the code jumps to unfreeze before disk_set_zones_cond_array() transfers
>    ownership of args->zones_cond to disk->zones_cond.
> 
> Fix cases 1 and 2 by adding a free_zones_cond label at the end of
> blk_revalidate_disk_zones() to centralize the cleanup. Fix case 3 by
> moving disk_set_zones_cond_array() before the nr_conv_zones check in
> disk_update_zone_resources() so that ownership is transferred early and
> disk_free_zone_resources() at the unfreeze label properly frees it.
> 
> Fixes: 6e945ffb6555 ("block: use zone condition to determine conventional zones")
> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
> ---
>  block/blk-zoned.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 9d1dd6ccfad7..2ea790e4f320 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -1956,6 +1956,8 @@ static int disk_update_zone_resources(struct gendisk *disk,
>  	memflags = blk_mq_freeze_queue(q);
>  
>  	disk->nr_zones = args->nr_zones;
> +	disk_set_zones_cond_array(disk, args->zones_cond);
> +
>  	if (args->nr_conv_zones >= disk->nr_zones) {
>  		queue_limits_cancel_update(q);
>  		pr_warn("%s: Invalid number of conventional zones %u / %u\n",
> @@ -1966,7 +1968,6 @@ static int disk_update_zone_resources(struct gendisk *disk,
>  
>  	disk->zone_capacity = args->zone_capacity;
>  	disk->last_zone_capacity = args->last_zone_capacity;
> -	disk_set_zones_cond_array(disk, args->zones_cond);
>  
>  	/*
>  	 * Some devices can advertise zone resource limits that are larger than
> @@ -2239,7 +2240,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
>  	ret = disk_revalidate_zone_resources(disk, &args);
>  	if (ret) {
>  		memalloc_noio_restore(noio_flag);
> -		return ret;
> +		goto free_zones_cond;
>  	}
>  
>  	ret = disk->fops->report_zones(disk, 0, UINT_MAX, &rep_args);
> @@ -2268,6 +2269,8 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
>  	disk_free_zone_resources(disk);
>  	blk_mq_unfreeze_queue(q, memflags);
>  
> +free_zones_cond:
> +	kfree(args.zones_cond);

This does not look correct: on success case, this will free the array despite
that array being set already. So rather than this, I think it is better to
change disk_revalidate_zone_resources() to free the array it allocated in the
case of an error. That will be a lot cleaner than this.

>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(blk_revalidate_disk_zones);


-- 
Damien Le Moal
Western Digital Research