Re: [PATCH v4 5/9] zloop: add storage element emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] block: IDR and module leak on zloop_ctl_add error path
- [High] block: zone reset vfs_truncate races with in-flight IO
- [Medium] block/zloop: elements left in RESTORE_IN_PROGRESS on error
--

commit 2aa446d71ddca7634ded310055adccb68fd83788
Author: Damien Le Moal <dlemoal@kernel.org>
Subject: zloop: add storage element emulation

Emulate the storage element depopulation feature of SMR disks in the
zloop driver. This emulation is controlled using the new stor_elements
option.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c
> @@ -420,7 +486,22 @@ static int zloop_close_zone(struct zloop_device *zlo, unsigned int zone_no)
>  	return ret;
>  }
>  
> -static int zloop_reset_zone(struct zloop_device *zlo, unsigned int zone_no)
> +static int zloop_do_reset_zone(struct zloop_device *zlo,
> +			       struct zloop_zone *zone)
> +{
> +	if (vfs_truncate(&zone->file->f_path, 0))
> +		return -EIO;
> +

[Severity: High]
Is there a potential race between this background truncation and active I/O?

When zloop_restore_elements() sets zlo->restore_in_progress = true and
schedules the background restore work, it rejects new I/O but doesn't
appear to drain in-flight I/O (for example, by using blk_mq_freeze_queue()).

If an in-flight REQ_OP_WRITE in zloop_cmd_workfn() executes
zloop_seq_write_prep() (which acquires zone->wp_lock, but not zone->lock)
at the same time this worker calls vfs_truncate(), could the worker
truncate the file to 0 just before the write completes at the old write
pointer? This might desync zone->wp and create a massive sparse file.

[ ... ]

> +static void zloop_restore_elements_work(struct work_struct *work)
> +{
> +	struct zloop_device *zlo = container_of(work, struct zloop_device,
> +						restore_elements_work.work);
> +	struct zloop_zone *zone;
> +	struct blk_se *se;
> +	unsigned int i;
> +	int ret;
> +
> +	mutex_lock(&zlo->stor_elements_lock);
> +
> +	/* Reset all zones. */
> +	for (i = 0, zone = zlo->zones; i < zlo->nr_zones; i++, zone++) {
> +		if (test_bit(ZLOOP_ZONE_CONV, &zone->flags)) {
> +			zloop_restore_conv_zone(zlo, zone);
> +			continue;
> +		}
> +		ret = zloop_restore_seq_zone(zlo, zone);
> +		if (ret)
> +			goto out_unlock;

[Severity: Medium]
If a zone file truncation fails here, are the storage elements left
indefinitely in the BLK_SE_STS_RESTORE_IN_PROGRESS state?

The error path jumps to out_unlock, which clears zlo->restore_in_progress
but skips updating the se->status for individual elements. Should this error
path transition the elements to BLK_SE_STS_RESTORE_ERROR instead, so that
userspace queries (like zloop_report_elements()) return an accurate status?

> +	}
> +
> +	/* Restore all removed elements. */
> +	for (i = 0, se = zlo->elements; i < zlo->nr_elements; i++, se++) {
> +		if (READ_ONCE(se->status) == BLK_SE_STS_RESTORE_IN_PROGRESS)
> +			WRITE_ONCE(se->status, BLK_SE_STS_OK);
> +	}
> +
> +	zlo->nr_removed_elements = 0;
> +
> +out_unlock:
> +	WRITE_ONCE(zlo->restore_in_progress, false);
> +	mutex_unlock(&zlo->stor_elements_lock);
> +}

[ ... ]

> @@ -1361,6 +1870,10 @@ static int zloop_ctl_add(struct zloop_options *opts)
>  		zlo->id, zlo->nr_zones,
>  		((sector_t)zlo->zone_size << SECTOR_SHIFT) >> 20,
>  		zlo->block_size);
> +	if (zlo->nr_elements)
> +		pr_info("zloop%d: %d storage elements\n",
> +			zlo->id, zlo->nr_elements);
> +
>  	pr_info("zloop%d: using %s%s zone append\n",
>  		zlo->id,
>  		zlo->ordered_zone_append ? "ordered " : "",
> @@ -1370,6 +1883,8 @@ static int zloop_ctl_add(struct zloop_options *opts)
>  
>  out_cleanup_disk:
>  	put_disk(zlo->disk);
> +	return ret;
> +

[Severity: High]
Does this early return bypass the IDR removal and module reference put?

If we return here, the out_free_idr label (which calls idr_remove()) and
the final out label (which calls module_put(THIS_MODULE)) are completely
skipped.

Since put_disk() ultimately triggers zloop_free_disk() which frees the
zlo structure via kvfree(zlo), does leaving the freed pointer in
zloop_index_idr create a dangling pointer for subsequent lookups? It also
appears the module reference would be leaked permanently.

>  out_cleanup_tags:
>  	blk_mq_free_tag_set(&zlo->tag_set);
>  out_close_files:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009090945.1504194-1-dlemoal@kernel.org?part=5