Re: [PATCH v6 07/18] s390/dasd: Add infrastructure for ESE full-track write

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

New issues:
- [High] Logic Error / Double Completion. Aborted origin CQRs (DASD_CQR_ABORTED) are erroneously completed to the block layer, leading to double-completion and UAF of block requests, or infinite hangs for non-block requests.

Pre-existing issues:
- [High] Pre-existing Use-After-Free in `__dasd_cleanup_cqr()` when accessing `cqr->intrc` after the request memory has been freed.
--

--- Patch [7]: [PATCH v6 07/18] s390/dasd: Add infrastructure for ESE full-track write ---
commit 6568e31c69d8658129394132fae86e53a21d2edd
Author: Stefan Haberland <[email protected]>

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

This commit adds the driver internals to build WRITE_FULL_TRACK FCX channel
programs for unformatted tracks on ESE devices. It introduces track-filler
buffers, hardirq-safe staging lists, and new CQR states to handle replaced
writes.

> 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
> @@ -1223,7 +1250,18 @@ void dasd_sfree_request(struct dasd_ccw_req *cqr, struct dasd_device *device)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&device->mem_lock, flags);
> -	dasd_free_chunk(&device->ccw_chunks, cqr->mem_chunk);
> +	/*
> +	 * Free the request block from the pool it came from: smalloc() sets
> +	 * mem_chunk (ccw_chunks), fmalloc() leaves it NULL (ese_chunks). A
> +	 * full-track request also frees its track-filler buffer.
> +	 */
> +	if (cqr->filldata)
> +		dasd_free_chunk(&device->fill_chunks, cqr->filldata);
> +	if (cqr->mem_chunk)
> +		dasd_free_chunk(&device->ccw_chunks, cqr->mem_chunk);
> +	else
> +		dasd_free_chunk(&device->ese_chunks, cqr);
> +
>  	spin_unlock_irqrestore(&device->mem_lock, flags);
>  	dasd_put_device(device);
>  }

[Severity: High]
This is a pre-existing issue, but does the block layer cleanup code access
the cqr memory after it is freed here?

Looking at the call chain starting from dasd_block_tasklet():

__dasd_cleanup_cqr() ->
  discipline->free_cp() (e.g., dasd_eckd_free_cp()) ->
    dasd_sfree_request()

After free_cp() returns, __dasd_cleanup_cqr() evaluates the request state:

    else if (status == 0) {
        switch (cqr->intrc) {
            ...

Since the cqr memory is returned to the chunk allocator pool inside
dasd_sfree_request(), could another CPU concurrently reallocate and
overwrite the cqr memory before cqr->intrc is read, resulting in a
use-after-free?

[ ... ]

> @@ -1884,6 +1924,10 @@ static void __dasd_process_cqr(struct dasd_device *device,
>  	case DASD_CQR_CLEARED:
>  		cqr->status = DASD_CQR_TERMINATED;
>  		break;
> +	case DASD_CQR_ABORT:
> +		cqr->status = DASD_CQR_ABORTED;
> +		cqr->callback = NULL;
> +		break;
>  	default:
>  		dev_err(&device->cdev->dev,
>  			"Unexpected CQR status %02x", cqr->status);

[ ... ]

> @@ -2761,7 +2806,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;
>  
>  		if (cqr->status == DASD_CQR_TERMINATED) {
> @@ -2872,6 +2918,14 @@ static void dasd_block_tasklet(unsigned long data)

[Severity: High]
The commit message states that the DASD_CQR_ABORT state is used to "retire
the origin CQR of a replaced write without completing it to the block layer."

However, since __dasd_process_block_ccw_queue() accepts DASD_CQR_ABORTED
and moves it to the final_queue, will __dasd_cleanup_cqr() still process
it?

If __dasd_cleanup_cqr() processes the aborted CQR, it appears to
unconditionally call blk_mq_end_request(req, error) instead of skipping
it. When the replacement ESE format CQR later completes, won't it try to
complete the exact same struct request, leading to a double-completion in
the block layer?

Additionally, for non-block requests, does setting cqr->callback = NULL in
__dasd_process_cqr() skip dasd_wakeup_cb()? Could this cause a waiter to
hang indefinitely in wait_event() if the wakeup callback is never executed?

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