Re: [PATCH v3 03/13] s390/dasd: Add range-based format-track collision detection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Uninitialized track range fields in data requests break collision detection.
- [High] Unreachable collision detection logic due to incorrect callback.
- [High] Collision detection loop ignores queued requests, skipping the ones it is meant to wake up.
- [High] Error paths perform unprotected list deletions and permanently leak format array slots.
--

--- Patch [3]: [PATCH v3 03/13] s390/dasd: Add range-based format-track collision detection ---
commit 1d2db2dae28f75e003a69468da44050bd92fdedd
Author: Stefan Haberland <sth@linux.ibm.com>

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, and extend
struct dasd_format_entry with a start_trk/end_trk/cqr range (replacing
the single track field).

> 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
[ ... ]
> @@ -2855,6 +2848,27 @@ 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;

[Severity: High]
Does this guard prevent the collision logic from ever being executed?

Regular data requests use dasd_return_cqr_cb() as their callback but they
do not have cqr->format set. ESE format requests have cqr->format set, but
they use dasd_eckd_ese_format_cb() as their callback instead of this one.

Will this path always take the goto out, making the collision logic
unreachable?

> +
> +	block = cqr->block;
> +	/*
> +	 * Mark queued CQRs that overlap this just-completed format
> +	 * range so they re-check in test_and_set_format.
> +	 */
> +	list_for_each_entry(temp_cqr, &block->ccw_queue, blocklist) {
> +		if (temp_cqr != cqr &&
> +		    temp_cqr->status != DASD_CQR_FILLED &&
> +		    temp_cqr->status != DASD_CQR_QUEUED &&

[Severity: High]
Does this explicitly skip requests in the queued state, which are exactly
the ones waiting to be flagged?

Requests delayed due to a conflict (returning -EEXIST from
test_and_set_format_track) are put in the DASD_CQR_QUEUED state. If this
loop filters out DASD_CQR_QUEUED, does it ignore the exact requests it was
designed to evaluate and flag with collision = true?

> +		    dasd_req_conflict(cqr, temp_cqr)) {
> +			temp_cqr->collision = true;
> +		}
> +	}
> +out:
>  	dasd_schedule_block_bh(cqr->block);
>  }
>  
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -3232,17 +3245,27 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
>  	fdata.intensity = private->uses_cdl ? DASD_FMT_INT_COMPAT : 0;
>  
>  	rc = dasd_eckd_format_sanity_checks(base, &fdata);
> -	if (rc)
> +	if (rc) {
> +		if (format)
> +			list_del(&format->list);
>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	/*
>  	 * We're building the request with PAV disabled as we're reusing
>  	 * the former startdev.
>  	 */
>  	fcqr = dasd_eckd_build_format(base, startdev, &fdata, 0);
> -	if (IS_ERR(fcqr))
> +	if (IS_ERR(fcqr)) {
> +		if (format)
> +			list_del(&format->list);
>  		return fcqr;
> +	}

[Severity: High]
Are we modifying the list without holding block->format_lock here?

These error paths call list_del(&format->list) without acquiring the lock.
Additionally, they do not set format->cqr = NULL. Since
test_and_set_format_track() identifies free slots by checking
!device->format_entry[i].cqr, does this permanently leak format array slots?

Could this use clear_format_track(format, block) instead to properly lock
and clear the state?

> diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
> --- a/drivers/s390/block/dasd_int.h
> +++ b/drivers/s390/block/dasd_int.h
[ ... ]
> @@ -834,6 +842,13 @@ static inline void *dasd_get_callback_data(struct dasd_ccw_req *cqr)
>  	return cqr->callback_data;
>  }
>  
> +static inline bool dasd_req_conflict(struct dasd_ccw_req *cqr1,
> +				     struct dasd_ccw_req *cqr2)
> +{
> +	return !(cqr1->format->end_trk < cqr2->start_trk ||
> +		 cqr2->end_trk < cqr1->format->start_trk);
> +}
> +

[Severity: Critical]
Are start_trk and end_trk guaranteed to be initialized for normal data
requests?

Since dasd_fmalloc_request() zero-allocates memory, cqr2->start_trk and
cqr2->end_trk default to 0 for normal data requests. When evaluated here,
the comparison cqr2->end_trk < cqr1->format->start_trk evaluates as
0 < format->start_trk, which is always true for any track > 0.

Does this silently fail to detect collisions for any tracks other than
track 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=3