Re: [PATCH 12/19] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Full-track format requests bypass `test_and_set_format_track()` locking, leading to TOCTOU data corruption from concurrent partial-track writes.
- [High] Setting cqr->callback = NULL for aborted requests during teardown overwrites the flush queue wakeup callback, causing an indefinite deadlock during device offline.
--

--- Patch [12]: [PATCH 12/19] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler ---
commit c79b7434acd3a79782b3c648eaa1de078edf01da
Author: Stefan Haberland <[email protected]>

s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

Wire dasd_eckd_build_cp_tpm_writefulltrack() into the ESE unformated
track handler.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 8379c8a40382f..da488c0775fcd 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -3253,50 +3241,94 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
> [ ... ]
> +	if (first_trk != last_trk && last_offs + 1 < recs_per_trk) {
> +		diff = last_offs + 1;
> +		last_rec = last_rec - diff;
> +		last_trk = last_rec;
> +		last_offs = sector_div(last_trk, recs_per_trk);
> +		proc_bytes = (last_rec - first_rec + 1) * blksize;
> +	}
> +	if (first_offs > 0 || last_offs + 1 < recs_per_trk) {
> +		/* test if tracks are already in formatting by another thread */
> +		if (test_and_set_format_track(first_trk, last_trk, cqr,
> +					      cqr->block, cqr->startdev, &format)) {

[Severity: Critical]
Does bypassing test_and_set_format_track() for full-track requests allow
concurrent partial-track writes to the same track to proceed without
detecting a conflict?

If a partial-track format CCW chain executes after the full-track write, its
zero-padding could silently overwrite the valid data just written by the
full-track write.

> +			/* this is no real error so do not count down retries */
> +			cqr->retries++;
> +			goto out_retry;
> +		}
> +	}
> +
> [ ... ]
> @@ -3304,10 +3336,44 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
> [ ... ]
> +	/*
> +	 * stage under ese_lock; dasd_block_tasklet splices it into ccw_queue.
> +	 * Direct enqueue here would invert queue_lock / ccwdev_lock.
> +	 */
> +	spin_lock(&block->ese_lock);
> +	list_add(&fcqr->blocklist, &block->ese_staging);
> +	spin_unlock(&block->ese_lock);
> +	/* mark origin CQR as aborted; ccwdev_lock is held by the IRQ handler */
> +	cqr->status = DASD_CQR_ABORT;

[Severity: High]
If a device is taken offline while an I/O request is being aborted by an ESE
format operation, _dasd_requests_to_flushqueue() sets
cqr->callback = _dasd_wake_block_flush_cb.

Will marking the status as DASD_CQR_ABORT here trigger a path in
__dasd_process_cqr() that unconditionally clears cqr->callback = NULL?

This would strip the wakeup callback installed by the offline process,
causing the offline process to sleep forever since wake_up(&dasd_flush_wq)
is never called.

> +	goto out;
> +
> +out_retry:
> +	/*
> +	 * If we can't format now, let the request go
> +	 * one extra round. Maybe we can format later.

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