Re: [PATCH 01/19] s390/dasd: Do not complete a failed ESE read as successful

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

Pre-existing issues:
- [Critical] When dasd_page_cache is enabled, successful ese_read operations for thin-provisioned tracks result in an information leak of uninitialized kernel slab memory to userspace.
- [High] Returning early with DASD_CQR_ERROR without clearing the timer and scheduling the bottom half causes a queue hang for ERP requests.
- [High] A failure to allocate an ERP request when retries are exhausted causes the block request to hang indefinitely on the ccw_queue.
--

--- Patch [1]: [PATCH 01/19] s390/dasd: Do not complete a failed ESE read as successful ---
commit 63ead90543228523947461ca90de80a29ba45fc6
Author: Stefan Haberland <[email protected]>

s390/dasd: Do not complete a failed ESE read as successful

The patch modifies dasd_int_handler() to check the return value of
ese_read() and fail the request via the normal error path if it fails.
This prevents stale or uninitialized memory from being handed to the block
layer when reading an unallocated ESE track.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index d8d912a3b3fec..56ef38243f823 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -1692,8 +1692,10 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
>  	if (dasd_ese_needs_format(cqr->block, irb)) {
>  		req = dasd_get_callback_data(cqr);
>  		if (!req) {
>  			cqr->status = DASD_CQR_ERROR;
>  			return;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does this early return bypass
timer clearing and queue scheduling when req is NULL?

If an ERP request triggers a NO_REC_FOUND condition on an ESE device,
dasd_ese_needs_format() returns true. dasd_int_handler() then retrieves a
NULL req and returns early without calling dasd_device_clear_timer() or
dasd_schedule_device_bh(). Can this stall the queue until the device timer
eventually expires?

>  		if (rq_data_dir(req) == READ) {
> -			device->discipline->ese_read(cqr, irb);
> -			cqr->status = DASD_CQR_SUCCESS;
> +			if (device->discipline->ese_read(cqr, irb))
> +				cqr->status = DASD_CQR_ERROR;
> +			else
> +				cqr->status = DASD_CQR_SUCCESS;
>  			cqr->stopclk = now;
>  			dasd_device_clear_timer(device);
>  			dasd_schedule_device_bh(device);

I also noticed a few other pre-existing issues in this area.

> drivers/s390/block/dasd_eckd.c:dasd_eckd_free_cp()
> 	if (dst != cda) {
> 		if (rq_data_dir(req) == READ)
> 			memcpy(dst, cda, bv.bv_len);

[Severity: Critical]
This is a pre-existing issue, but does this unconditionally copy the
uninitialized bounce buffer over the original bio page, destroying the zeroes
written by ese_read()?

If dasd_page_cache is enabled, dasd_eckd_build_cp_cmd_single() allocates a
bounce buffer without __GFP_ZERO. If the device returns NO_REC_FOUND,
dasd_int_handler() calls ese_read(), which zero-fills the original bio
pages. On completion, does this copy overwrite the zeroes with uninitialized
memory, potentially causing an information leak to userspace?


> drivers/s390/block/dasd.c:__dasd_process_block_ccw_queue()
> 	if (cqr->status == DASD_CQR_NEED_ERP) {
> 		erp_fn = base->discipline->erp_action(cqr);
> 		if (IS_ERR(erp_fn(cqr)))
> 			continue;
> 		goto restart;
> 	}

[Severity: High]
This is a pre-existing issue, but does the continue statement here bypass
the necessary cleanup and queue movement for the failed request?

If dasd_alloc_erp_request() fails to allocate memory under memory pressure
and retries are exhausted, it sets cqr->status = DASD_CQR_FAILED and returns
an ERR_PTR. Because the code continues to the next loop iteration without
calling list_move_tail(), does the request remain on block->ccw_queue,
causing a permanent block device queue hang?

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