Re: [PATCH v2] block: fix zones_cond memory leak in blk_revalidate_disk_zones()

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

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


> 
> On 3/31/26 19:01, 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 this by freeing args->zones_cond at each error site: in
> >  blk_revalidate_disk_zones() when disk_revalidate_zone_resources() or zone
> >  reporting fails, and in disk_update_zone_resources() before jumping to
> >  the unfreeze label.
> >  
> >  Fixes: 6e945ffb6555 ("block: use zone condition to determine conventional zones")
> >  Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
> >  ---
> >  block/blk-zoned.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >  
> >  diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> >  index 9d1dd6ccfad7..be99ab785dcd 100644
> >  --- a/block/blk-zoned.c
> >  +++ b/block/blk-zoned.c
> >  @@ -1960,6 +1960,7 @@ static int disk_update_zone_resources(struct gendisk *disk,
> >  queue_limits_cancel_update(q);
> >  pr_warn("%s: Invalid number of conventional zones %u / %u\n",
> >  disk->disk_name, args->nr_conv_zones, disk->nr_zones);
> >  + kfree(args->zones_cond);
> > 
> Let's keep the cleanups together. So move this before pr_warn().
> Also, the call to queue_limits_commit_update() may fail, so you need a free
> after the unfreeze label in that function too.
> 
> > 
> > ret = -ENODEV;
> >  goto unfreeze;
> >  }
> >  @@ -2239,6 +2240,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
> >  ret = disk_revalidate_zone_resources(disk, &args);
> >  if (ret) {
> >  memalloc_noio_restore(noio_flag);
> >  + kfree(args.zones_cond);
> > 
> This should be in disk_revalidate_zone_resources().
> 
> > 
> > return ret;
> >  }
> >  
> >  @@ -2264,6 +2266,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
> >  
> >  pr_warn("%s: failed to revalidate zones\n", disk->disk_name);
> >  
> >  + kfree(args.zones_cond);
> >  memflags = blk_mq_freeze_queue(q);
> >  disk_free_zone_resources(disk);
> >  blk_mq_unfreeze_queue(q, memflags);
> > 
> I thinks something like this may be cleaner as it avoids having that kfree() all
> over the place:
> 
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 10655213e8e1..18cd9a1c6c53 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -2028,6 +2028,7 @@ static int disk_revalidate_zone_resources(struct gendisk
> *disk,
>  {
>  struct queue_limits *lim = &disk->queue->limits;
>  unsigned int pool_size;
> + int ret = 0;
> 
>  args->disk = disk;
>  args->nr_zones =
> @@ -2050,10 +2051,13 @@ static int disk_revalidate_zone_resources(struct gendisk
> *disk,
>  pool_size =
>  min(BLK_ZONE_WPLUG_DEFAULT_POOL_SIZE, args->nr_zones);
> 
> - if (!disk->zone_wplugs_hash)
> - return disk_alloc_zone_resources(disk, pool_size);
> + if (!disk->zone_wplugs_hash) {
> + ret = disk_alloc_zone_resources(disk, pool_size);
> + if (ret)
> + kfree(args->zones_cond);
> + }
> 
> - return 0;
> + return ret;
>  }
> 
>  /*
> @@ -2085,6 +2089,7 @@ 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);
> + args->zones_cond = NULL;
> 
>  /*
>  * Some devices can advertise zone resource limits that are larger than
> @@ -2365,21 +2370,30 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
>  }
>  memalloc_noio_restore(noio_flag);
> 
> + if (ret <= 0)
> + goto free_resources;
> +
>  /*
>  * If zones where reported, make sure that the entire disk capacity
>  * has been checked.
>  */
> - if (ret > 0 && args.sector != capacity) {
> + if (args.sector != capacity) {
>  pr_warn("%s: Missing zones from sector %llu\n",
>  disk->disk_name, args.sector);
>  ret = -ENODEV;
> + goto free_resources;
>  }
> 
> - if (ret > 0)
> - return disk_update_zone_resources(disk, &args);
> + ret = disk_update_zone_resources(disk, &args);
> + if (ret)
> + goto free_resources;
> +
> + return 0;
> 
> +free_resources:
>  pr_warn("%s: failed to revalidate zones\n", disk->disk_name);
> 
> + kfree(args.zones_cond);
>  memflags = blk_mq_freeze_queue(q);
>  disk_free_zone_resources(disk);
>  blk_mq_unfreeze_queue(q, memflags);
> 

Haha, the patch I wrote at the beginning had similar advantages to yours. 
I also wanted to refactor it so that the correct path wouldn't be hidden, 
but that would involve more code than it does now, so I didn't do it. Since
you mentioned it, I'll use your method, which is actually quite good.

Thanks.

-- 
Jackie Liu

> -- 
> Damien Le Moal
> Western Digital Research
>