Re: [PATCH v7 11/19] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

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

New issues:
- [High] The WRITE_FULL_TRACK channel program emits one ECKD COUNT record per physical `bvec` segment fragment rather than per logical block, corrupting disk geometry, truncating user data, and overflowing TIDAW allocations.
- [High] Missing `bio_has_data()` guard before iterating over request segments in `dasd_eckd_build_cp_tpm_writefulltrack` causes a NULL pointer dereference on payload-less requests.
- [High] Negative bit shift in `prepare_itcw()` when track count exceeds 16 causes undefined behavior.

Pre-existing issues:
- [High] Preexisting functions `dasd_eckd_build_cp_tpm_track` and `dasd_eckd_build_cp_cmd_track` use `rq_data_dir()` to guard data access, failing to exclude dataless write operations and causing NULL pointer dereferences.
--

--- Patch [11]: [PATCH v7 11/19] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() ---
commit 194da1c99392fbbd1e70e7463ef11e36aaa6e8ba
Author: Stefan Haberland <[email protected]>

s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

This commit adds a channel program builder for WRITE_FULL_TRACK requests.
It creates an ITCW with a TIDAW list for track formatting and writing on
ESE devices.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index b54d1858db160..475c0aca4a5ae 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -4742,6 +4750,340 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(
>  	return ERR_PTR(ret);
>  }

[Severity: High]
This is a pre-existing issue, but functions like
dasd_eckd_build_cp_tpm_track() and dasd_eckd_build_cp_cmd_track() appear to
use rq_data_dir() to guard data access.

Does this safely exclude dataless write operations? If a dataless write like
REQ_OP_DISCARD or REQ_OP_WRITE_ZEROES is submitted, will the call to
rq_for_each_segment() cause a NULL pointer dereference on bi_io_vec? A
bio_has_data() guard might be required here to strictly verify the payload
before accessing data segments.

> +static __maybe_unused struct dasd_ccw_req *
> +dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *startdev,

[ ... ]

> +	trkcount = last_trk - first_trk + 1;
> +	ctidaw = trkcount * 2 * (blk_per_trk + 2);

[ ... ]

> +	if (prepare_itcw(itcw, first_trk, last_trk,
> +			 cmd, basedev, startdev,
> +			 0,
> +			 trkcount, blksize,
> +			 datasize,
> +			 tlf,
> +			 blk_per_trk) == -EAGAIN) {

[Severity: High]
Can this result in undefined behavior due to a negative bit shift?

The variable trkcount is calculated from the request dimensions and passed as
the count argument to prepare_itcw(). If trkcount exceeds 16, does the
operation lredata->extended_parameter[1] <<= (16 - count) inside
prepare_itcw() perform a shift by a negative amount?

[ ... ]

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

[Severity: High]
Does this iteration safely handle payload-less requests?

If a dataless request, such as REQ_OP_WRITE_ZEROES or REQ_OP_DISCARD, is
processed, the bi_io_vec will be NULL. Will executing rq_for_each_segment()
here cause a NULL pointer dereference? Should this iteration be guarded by a
check to bio_has_data() to ensure the payload is present?

> +		dst = bvec_virt(&bv);
> +		seg_len = bv.bv_len;
> +		while (seg_len) {

[ ... ]

> +			/*
> +			 * One count + data record per block: a bvec segment can
> +			 * be up to a page, so clamp to blksize - otherwise the
> +			 * count field would describe one oversized record instead
> +			 * of several blksize ones for sub-page block sizes.
> +			 */
> +			part_len = min(seg_len, len_to_track_end);
> +			part_len = min(part_len, blksize);
> +			seg_len -= part_len;
> +			len_to_track_end -= part_len;

[ ... ]

> +			last_tidaw = add_track_record(itcw, &filldata, &fillsize,
> +						      cyl, head, rec, dst, part_len);

[Severity: High]
Does this emit a separate ECKD COUNT record for each physical memory fragment
rather than per logical block?

If a logical block spans multiple physical pages and is split into multiple
bvec segments by the block layer, such as an unaligned 4K block crossing a
page boundary, it appears add_track_record() is called unconditionally per
physical fragment.

Will this cause the function to emit two smaller COUNT records instead of a
single 4096-byte COUNT record? This seems like it could break track geometry,
overflow the pre-calculated ctidaw limit by generating additional TIDAWs, and
prematurely increment the logical record counters.

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