Re: [PATCH v5 06/17] s390/dasd: Add infrastructure for ESE full-track write

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Aborted origin CQRs (DASD_CQR_ABORTED) are erroneously completed to the block layer with an I/O error or requeued, causing double-completion or use-after-free.
--

--- Patch [6]: [PATCH v5 06/17] s390/dasd: Add infrastructure for ESE full-track write ---
commit 6ca5a3ba40b43e7a20a1f06beb37ce4d8e6866d8
Author: Stefan Haberland <[email protected]>

s390/dasd: Add infrastructure for ESE full-track write

This commit adds driver internals to build WRITE_FULL_TRACK FCX channel
programs for unformatted tracks on ESE devices. It introduces new CQR states
(DASD_CQR_ABORT and DASD_CQR_ABORTED) intended to retire the origin CQR of a
replaced write without completing it to the block layer. It also expands
the CCW and ESE format pools to keep maximum-size requests in flight.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 3658a714abd62..d6ccf0186ada4 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
[ ... ]
> @@ -2759,7 +2804,8 @@ static void __dasd_process_block_ccw_queue(struct dasd_block *block,
>  		if (cqr->status != DASD_CQR_DONE &&
>  		    cqr->status != DASD_CQR_FAILED &&
>  		    cqr->status != DASD_CQR_NEED_ERP &&
> -		    cqr->status != DASD_CQR_TERMINATED)
> +		    cqr->status != DASD_CQR_TERMINATED &&
> +		    cqr->status != DASD_CQR_ABORTED)
>  			continue;

[Severity: High]
Will this cause aborted origin CQRs to be erroneously completed to the block
layer?

By allowing DASD_CQR_ABORTED to pass through here, the request is moved to
final_queue at the end of __dasd_process_block_ccw_queue().

Later in dasd_block_tasklet(), all requests in final_queue are unconditionally
passed to __dasd_cleanup_cqr():

dasd_block_tasklet() {
    ...
    list_for_each_safe(l, n, &final_queue) {
        cqr = list_entry(l, struct dasd_ccw_req, blocklist);
        ...
        __dasd_cleanup_cqr(cqr);
        ...
    }
}

Since __dasd_cleanup_cqr() lacks a specific check for DASD_CQR_ABORTED, it
will set the error to BLK_STS_IOERR and complete the request:

__dasd_cleanup_cqr() {
    ...
    status = cqr->block->base->discipline->free_cp(cqr, req);
    ...
    else if (status == 0) {
        switch (cqr->intrc) {
        ...
        default:
            error = BLK_STS_IOERR;
            break;
        }
    }

    if (error)
        blk_mq_end_request(req, error);
}

If the replacement format-CQR also attempts to complete or requeue this
same block request, could this lead to a double-completion or a
use-after-free in the block layer?

As a related question, does dasd_generic_requeue_all_requests() also need
to check if the CQR was aborted? It currently appears to requeue the block
request without checking for the aborted status:

dasd_generic_requeue_all_requests() {
    ...
    _dasd_requeue_request(cqr);
    list_del_init(&cqr->blocklist);
    ...
}

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