Re: [PATCH v5 01/17] s390/dasd: Propagate partial completion length across ERP recovery

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

Pre-existing issues:
- [High] The return value of `ese_read()` is completely ignored, leading to silent data corruption on read failures.
--

--- Patch [1]: [PATCH v5 01/17] s390/dasd: Propagate partial completion length across ERP recovery ---
commit 804bfccaf59e282c990c294a7564d63b4f3b75a4
Author: Stefan Haberland <[email protected]>

s390/dasd: Propagate partial completion length across ERP recovery

This patch updates dasd_default_erp_postaction() to copy the proc_bytes
field from the finished ERP request back to the original request. This
prevents partially completed requests from losing their completion length
during ERP recovery, avoiding silent data corruption for thin provisioned
devices.

> diff --git a/drivers/s390/block/dasd_erp.c b/drivers/s390/block/dasd_erp.c
> index 89d7516b9ec86..468f0b2cc3423 100644
> --- a/drivers/s390/block/dasd_erp.c
> +++ b/drivers/s390/block/dasd_erp.c
[ ... ]
> @@ -147,6 +149,7 @@ struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
>  	cqr->startclk = startclk;
>  	cqr->stopclk = stopclk;
>  	cqr->startdev = startdev;
> +	cqr->proc_bytes = proc_bytes;
>  	if (success)
>  		cqr->status = DASD_CQR_DONE;

While reviewing this fix for thin provisioned devices, I noticed the following
in dasd_int_handler():

drivers/s390/block/dasd.c:dasd_int_handler() {
    ...
		if (rq_data_dir(req) == READ) {
			device->discipline->ese_read(cqr, irb);
			cqr->status = DASD_CQR_SUCCESS;
			cqr->stopclk = now;
			dasd_device_clear_timer(device);
			dasd_schedule_device_bh(device);
			return;
		}
    ...
}

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but does this code ignore the return value of ese_read()?

If ese_read() encounters an error and fails to zero out the block buffer, the
request still unconditionally completes with DASD_CQR_SUCCESS.

Could this result in the block layer being handed uninitialized memory or
previous page contents instead of zeroed data?

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