Re: [PATCH 02/19] 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] NULL pointer dereference in dasd_eckd_ese_read() when handling ERP requests.
--

--- Patch [2]: [PATCH 02/19] s390/dasd: Propagate partial completion length across ERP recovery ---
commit 3cc15afbf5f352896f1c63d6a8c8e105bd9920f3
Author: Stefan Haberland <[email protected]>

s390/dasd: Propagate partial completion length across ERP recovery

dasd_default_erp_postaction() copies the timing and device state from
the finished ERP request back to the original request but drops
proc_bytes. A request that was partially completed, an ESE read of a
not-yet-allocated track returns fewer bytes than requested, and then
recovered through the ERP chain loses its partial-completion length.

> 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;
>  	else {

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but while looking at ERP recovery, can a NULL pointer dereference
occur in dasd_eckd_ese_read() when an ERP request encounters a 'No Record
Found' error?

In drivers/s390/block/dasd.c:dasd_int_handler(), the code resolves the base
request but passes the ERP request directly to ese_read():

		req = dasd_get_callback_data(cqr);
		...
		if (rq_data_dir(req) == READ) {
			if (device->discipline->ese_read(cqr, irb))

Inside drivers/s390/block/dasd_eckd.c:dasd_eckd_ese_read(), it directly
reads callback_data:

	req = (struct request *) cqr->callback_data;
	...
	first_trk = first_blk = blk_rq_pos(req) >> block->s2b_shift;

Since ERP requests do not have callback_data assigned, could evaluating req
as NULL lead to an immediate dereference in blk_rq_pos()? Would it be safer
if dasd_eckd_ese_read() used dasd_get_callback_data(cqr) instead?

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