Re: [PATCH v3 05/13] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Incorrect `len_to_track_end` initialization for subsequent tracks leads to malformed channel programs with duplicate Track End markers.
- [High] The function iterates over bio data segments without verifying operation type safety, violating block layer guidelines.
--

--- Patch [5]: [PATCH v3 05/13] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() ---
commit 86379d92de11de4619dd9c8f1c829907036c1f56
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

Add the channel program builder for WRITE_FULL_TRACK requests, used
by dasd_eckd_ese_format() to format and write a set of tracks
atomically and avoid the format cycle on ESE devices.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index a570fe2e85693..0962c9f72c864 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -4714,6 +4722,337 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(

[ ... ]

> +	/* process data records */
> +	rec = first_offs + 1;
> +	rec_count = 0;
> +	rq_for_each_segment(bv, req, iter) {

[Severity: High]
Can this cause a NULL pointer dereference if a dataless request (like
REQ_OP_DISCARD or REQ_OP_FLUSH) is dispatched here?

The block layer guidelines indicate that iterating over bio data segments
requires checking blk_rq_has_data() or bio_has_data() first, as op_is_write()
evaluates to true for dataless operations, which have a NULL bi_io_vec.

> +		dst = bvec_virt(&bv);
> +		seg_len = bv.bv_len;
> +		while (seg_len) {
> +			if (new_track) {
> +				trkid = recid;
> +				offs = sector_div(trkid, blk_per_trk);
> +				count_to_trk_end = blk_per_trk - offs;
> +				count = min((last_rec - recid + 1),
> +					    (sector_t)count_to_trk_end);
> +				len_to_track_end = count * blksize;

[Severity: Critical]
Is len_to_track_end correctly initialized here for subsequent tracks?

It looks like this clamps the track length to the remaining data length
(count * blksize) instead of the physical track capacity
(blk_per_trk * blksize).

If len_to_track_end is set to the data length, it will reach zero prematurely.
Will this cause the track end marker to be emitted early inside this loop:

	if (new_track) {
		last_tidaw = add_track_end(itcw, &filldata,
					   &fillsize);
		...
	}

And then a second track end marker to be appended later in out_skip after the
padding records? This might result in a malformed channel program that is
rejected by the hardware.

> +				recid += count;
> +				new_track = 0;
> +				/* the R0 header of the next track */
> +				cyl = curr_trk / heads;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=5