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

["Jackie Liu" <liu.yun@linux.dev>]

2026年3月31日 17:18, "Damien Le Moal" <dlemoal@kernel.org mailto:dlemoal@kernel.org?to=%22Damien%20Le%20Moal%22%20%3Cdlemoal%40kernel.org%3E > 写到:


> 
> 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.

Thanks for the review.

Actually, the free_zones_cond label is only reachable on error paths
(ret <= 0). On the success path (ret > 0), the function returns
directly via "return disk_update_zone_resources(disk, &args)" and never
reaches this label. So the logic should be correct.

That said, I agree that having disk_revalidate_zone_resources() free
the array itself on error is cleaner and easier to follow. I'll send
a v2 with that approach.

-- 
Jackie

> 
> > 
> > return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(blk_revalidate_disk_zones);
> > 
> -- 
> Damien Le Moal
> Western Digital Research
>