Re: [PATCH v3 06/13] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

[sashiko-bot@kernel.org]

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

New issues:
- [High] Generating a WRITE_FULL_TRACK CCW for a request spanning more than 16 tracks causes a negative bit shift, producing an invalid hardware track mask.
- [High] Bounce buffers allocated from `dasd_page_cache` are permanently leaked when an origin request is aborted and replaced by an ESE format request.
- [High] The `fillsize` allocation for `filldata` does not account for padding injected by `reserve_nocross()`, leading to out-of-bounds allocation failures and dropped I/O.

Pre-existing issues:
- [Critical] Partial I/O completions on ERP requests lose the `proc_bytes` value, causing the block layer to incorrectly treat the entire request as successful.
- [High] Error paths in `dasd_eckd_ese_format` fail to schedule the device tasklet or clear the timer, causing the I/O request to hang.
--

--- Patch [6]: [PATCH v3 06/13] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler ---
commit 2efec0cd23e150d9cbd9db63fa5fad5ebdee9860
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

Wire dasd_eckd_build_cp_tpm_writefulltrack() into the ESE unformated
track handler. dasd_eckd_ese_format() now returns void and builds a
writefulltrack CQR directly.

> 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
> @@ -3224,60 +3212,135 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
[ ... ]
> +	rc = dasd_eckd_track_from_irb(irb, base, &curr_trk);
> +	if (rc) {
> +		/* sense data could not be parsed - this will not resolve by retrying */
> +		cqr->status = DASD_CQR_ERROR;
> +		return;
> +	}

[Severity: High]
This is a pre-existing issue, but error paths in dasd_eckd_ese_format() fail
to schedule the device tasklet or clear the timer. If sense data cannot be
parsed or the track is out-of-bounds, dasd_eckd_ese_format() sets the status
to DASD_CQR_ERROR and returns. Since dasd_int_handler() expects the
discipline hook to handle scheduling on error, doesn't this leave the I/O
request stranded in the queue until the 5-minute fallback timeout fires?

[ ... ]
> +	fcqr = dasd_eckd_build_cp_tpm_writefulltrack(startdev, block, req,
> +						     first_rec, last_rec,
> +						     first_trk, last_trk,
> +						     first_offs, last_offs,
> +						     recs_per_trk, blksize, cqr);

[Severity: High]
Can trkcount exceed 16 here? When dasd_eckd_build_cp_tpm_writefulltrack()
calls prepare_itcw(), the count argument is used in a bit shift:

lredata->extended_parameter[1] <<= (16 - count);

If a user issues a large write request that spans more than 16 tracks,
won't this result in a negative bit shift, causing undefined behavior and
generating an invalid hardware track mask?

[Severity: High]
Does the fillsize allocation inside dasd_eckd_build_cp_tpm_writefulltrack()
account for the padding injected by reserve_nocross()?

fillsize = trkcount * (sizeof(struct eckd_r0) +
                       (sizeof(struct eckd_count) * (blk_per_trk + 2)));
filldata = dasd_alloc_chunk(&startdev->fill_chunks, fillsize);

When reserve_nocross() appends elements, it injects padding bytes to
prevent them from crossing page boundaries. Because this padding is not
budgeted in fillsize, the remaining space might drop below zero before all
track elements are written, causing reserve_nocross() to return NULL and
abort the request.

> @@ -5297,6 +5363,14 @@ dasd_eckd_free_cp(struct dasd_ccw_req *cqr, struct request *req)
>  	sector_t recid;
>  	int status;
>  
> +	/*
> +	 * special handling for format aborted requests
> +	 * only clean up the request, nothing finished
> +	 */
> +	if (cqr->status == DASD_CQR_ABORTED) {
> +		status = DASD_CQR_ABORTED;
> +		goto abort;
> +	}
>  	if (!dasd_page_cache)
>  		goto out;

[Severity: High]
Does this early exit skip freeing the bounce buffers? If dasd_page_cache
is active and an origin request is marked DASD_CQR_ABORTED (because it
was replaced by an ESE format request), the goto abort statement bypasses
the loop responsible for calling kmem_cache_free(). Could this permanently
leak the bounce buffer memory allocated for the I/O segments?

> diff --git a/drivers/s390/block/dasd_erp.c b/drivers/s390/block/dasd_erp.c
> --- a/drivers/s390/block/dasd_erp.c
> +++ b/drivers/s390/block/dasd_erp.c
> @@ -120,13 +120,14 @@ dasd_default_erp_action(struct dasd_ccw_req *cqr)
>   */
>  struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
>  {
> -	int success;
> +	int success, aborted;
>  	unsigned long startclk, stopclk;
>  	struct dasd_device *startdev;
>  
>  	BUG_ON(cqr->refers == NULL || cqr->function == NULL);
>  
>  	success = cqr->status == DASD_CQR_DONE;
> +	aborted = cqr->status == DASD_CQR_ABORTED;

[Severity: Critical]
This is a pre-existing issue, but does the loop that unwinds the ERP chain
just below here lose the proc_bytes value from a partial completion?

while (cqr->refers != NULL) {
        struct dasd_ccw_req *refers = cqr->refers;
        list_del(&cqr->blocklist);
        dasd_free_erp_request(cqr, cqr->memdev);
        cqr = refers;
}

When an ERP retry hits an unformatted track, dasd_eckd_ese_format() or
ese_read() sets cqr->proc_bytes on the ERP request and marks it successful.
During this unwinding, the ERP request holding this partial completion is
freed without propagating proc_bytes to the base request. When the base
request is later processed, it will have proc_bytes == 0, causing the block
layer to incorrectly treat the entire request as successful and silently
drop the unwritten or unread portion.

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