Re: [PATCH 08/19] s390/dasd: Add infrastructure for ESE full-track write

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Double completion of aborted origin CQR due to flawed completion prevention logic.
- [High] Lockless TOCTOU race in `_wait_for_empty_queues` evaluates queues in the wrong order.
--

--- Patch [8]: [PATCH 08/19] s390/dasd: Add infrastructure for ESE full-track write ---
commit d9f946540847a2c32306276982008cd999ef5ab9
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 in response to unformatted tracks on ESE devices. It introduces
track-filler buffers, a hardirq-safe staging list for ESE format CQRs,
and new CQR states to retire replaced origin writes.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 12fa04537fb02..761d559101a91 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
[ ... ]
> @@ -364,7 +390,8 @@ int _wait_for_empty_queues(struct dasd_device *device)
>  {
>  	if (device->block)
>  		return list_empty(&device->ccw_queue) &&
> -			list_empty(&device->block->ccw_queue);
> +			list_empty(&device->block->ccw_queue) &&
> +			list_empty(&device->block->ese_staging);

[Severity: High]
Does this lockless evaluation introduce a time-of-check to time-of-use race
by checking the downstream queue before the upstream queue?

If CPU A checks block->ccw_queue and finds it empty, and CPU B subsequently
splices a request from ese_staging into block->ccw_queue, CPU A will then
check ese_staging and also find it empty. CPU A would return true,
prematurely signaling that all queues are clear while an I/O request
is actually in flight on block->ccw_queue.

[ ... ]
> @@ -1885,6 +1925,17 @@ 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;
> +		/*
> +		 * ABORT is only set on the block-layer origin write that a
> +		 * full-track format replaces. Clear the callback so the request
> +		 * is not completed here - the replacement completes it. Internal
> +		 * requests never take this path, so no sleep_on waiter is left
> +		 * without its wakeup.
> +		 */
> +		cqr->callback = NULL;
> +		break;
>  	default:
[ ... ]
> @@ -2779,7 +2831,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]
Can this lead to a double completion and use-after-free for the aborted
origin request?

In __dasd_process_cqr(), the aborted request has its cqr->callback set to
NULL to prevent it from completing. However, here in
__dasd_process_block_ccw_queue(), DASD_CQR_ABORTED is explicitly added to the
finished states, which routes the request to final_queue.

When final_queue is processed by __dasd_cleanup_cqr(), it unconditionally
completes the block request using cqr->callback_data, ignoring the fact that
cqr->callback is NULL.

When the replacement ESE format CQR later finishes, it will complete the same
request again, resulting in a double completion.

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