Re: [PATCH v6 11/18] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Skipping test_and_set_format_track() for full-track writes allows concurrent partial-track formats to overwrite valid data with zero-padding.
- [Medium] Double counting of profiling statistics for aborted ESE format base requests.

Pre-existing issues:
- [Critical] Lockless access to `cqr->collision` creates a Time-Of-Check to Time-Of-Use (TOCTOU) race that can lead to redundant track formatting and data corruption.
--

--- Patch [11]: [PATCH v6 11/18] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler ---
commit d41c1ad780e71d8d577bfe81bd62b8114d2d1103
Author: Stefan Haberland <[email protected]>

s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

This commit wires dasd_eckd_build_cp_tpm_writefulltrack() into the ESE
unformatted track handler. It calculates the failing track ranges, builds a
WRITE_FULL_TRACK CQR, and aborts the origin CQR.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index dfbadd83bea21..f1235729326ee 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
[ ... ]
> @@ -2754,6 +2758,9 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr)
>  			error = BLK_STS_IOERR;
>  			break;
>  		}
> +	} else if (status == DASD_CQR_ABORTED) {
> +		/* aborted requests are replaced with a new one so do not complete this */
> +		return;
>  	}
>  
>  	/*

[Severity: Medium]
Could this result in double-counting of profiling statistics?
dasd_profile_end() is called unconditionally at the beginning of
__dasd_cleanup_cqr(). Since the aborted CQR is replaced by a format request
that will also complete and go through this cleanup path, won't the same
block request be profiled twice in the block and device accumulators?

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 8e18dad96299f..5e7a531707a0c 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -3251,60 +3239,138 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
[ ... ]
> +	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]
Is it safe to skip test_and_set_format_track() for full-track writes?

If a full-track write and a partial-track write are submitted concurrently
to the same unformatted track, the full-track write could skip acquiring the
format lock here. The partial-track write might then successfully acquire
the lock and build a zero-padded WRITE_FULL_TRACK CQR. If the partial-track
write executes on the device after the full-track write, couldn't it
overwrite the valid data from the full-track write with zeros?

[Severity: Critical]
This is a pre-existing issue, but is there a potential Time-Of-Check to
Time-Of-Use (TOCTOU) race inside test_and_set_format_track()?

Looking at test_and_set_format_track(), it checks cqr->collision without
holding a lock before acquiring block->format_lock:

test_and_set_format_track() {
    if (cqr && cqr->collision) {
        cqr->collision = false;
        return true;
    }
    spin_lock_irqsave(&block->format_lock, flags);
    ...
}

If a partial-track write fails with INV_TRACK_FORMAT while another format
operation is in progress on the same track, and this lockless read sees a
stale false value, it proceeds to acquire the lock. If the format entry was
just cleared by the other CPU's tasklet, it might find an empty format list,
assume the track is unformatted, and issue a zero-padded WRITE_FULL_TRACK,
which would destroy the data just written by the previous format operation.

Could we read the collision flag inside the lock to enforce memory ordering?

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