Re: [PATCH 09/19] s390/dasd: Add range-based format-track collision detection

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Removing the `trkcount` snapshot protection before the new collision detection becomes active leaves a window for data corruption via double-formatting.
- [High] Defensive check `if (!cqr->block)` introduces a guaranteed NULL pointer dereference on its error path.
--

--- Patch [9]: [PATCH 09/19] s390/dasd: Add range-based format-track collision detection ---
commit 1e9c1dce6a9ae6f45c013547038cfe3ae41af93a
Author: Stefan Haberland <[email protected]>

s390/dasd: Add range-based format-track collision detection

Replace the single per-device format_entry slot with an array of 16
slots so multiple format requests can be in flight at once. This also
rewrites test_and_set_format_track() to check range overlaps and removes
the older trkcount snapshot logic.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -1402,13 +1402,6 @@ int dasd_start_IO(struct dasd_ccw_req *cqr)
>  		if (!cqr->lpm)
>  			cqr->lpm = dasd_path_get_opm(device);
>  	}
> -	/*
> -	 * remember the amount of formatted tracks to prevent double format on
> -	 * ESE devices
> -	 */
> -	if (cqr->block)
> -		cqr->trkcount = atomic_read(&cqr->block->trkcount);
> -
>  	if (cqr->cpmode == 1) {

[Severity: Critical]
Does removing this trkcount snapshot before the new collision detection is
active introduce a regression where concurrent threads could double-format 
tracks?

The commit message notes the new collision detection only becomes active
later in the series. By removing the older trkcount protection here,
it seems we might be introducing a bisection hole. For instance, if
thread B completes a format and removes the track from format_list, could
thread A's ERP routine see an old No Record Found error and proceed to
format the track again, erasing thread B's data?

[ ... ]

> @@ -2880,6 +2873,28 @@ static void __dasd_process_block_ccw_queue(struct dasd_block *block,
>  
>  static void dasd_return_cqr_cb(struct dasd_ccw_req *cqr, void *data)
>  {
> +	struct dasd_ccw_req *temp_cqr;
> +	struct dasd_block *block;
> +
> +	/* only format CQRs are candidates */
> +	if (!cqr->block || unlikely(!cqr->format))
> +		goto out;
> +
> +	block = cqr->block;

[ ... ]

> +out:
>  	dasd_schedule_block_bh(cqr->block);
>  }

[Severity: High]
If cqr->block is NULL, the if statement above evaluates to true and jumps
to the out label. Would this result in a regression with a guaranteed NULL
pointer dereference when dasd_schedule_block_bh() unconditionally accesses
it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9