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

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

New issues:
- [Critical] Split memory segments (bvecs) result in truncated physical records, corrupting the track format.
- [High] Missing `bio_has_data()` guard before iterating over request segments.

Pre-existing issues:
- [High] Negative bitwise shift and truncated extended parameter length when formatting >16 tracks.
--

--- Patch [11]: [PATCH 11/19] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() ---
commit 756c6129bbfe82549e28cc85e3b42100ba732cd0
Author: Stefan Haberland <[email protected]>
Subject: s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

Adds the channel program builder for WRITE_FULL_TRACK requests, used
by dasd_eckd_ese_format() to format and write a set of tracks
atomically.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index ae122fbdca3ed..8379c8a40382f 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> +static __maybe_unused struct dasd_ccw_req *
> +dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *startdev,
> +				      struct dasd_block *block,
> +				      struct request *req,
[ ... ]
> +	if (prepare_itcw(itcw, first_trk, last_trk,
> +			 cmd, basedev, startdev,
> +			 0,
> +			 trkcount, blksize,
> +			 datasize,
> +			 tlf,
> +			 blk_per_trk) == -EAGAIN) {

[Severity: High]
This is a pre-existing issue in prepare_itcw(), but formatting more than
16 tracks could cause undefined behavior here.

When DASD_ECKD_CCW_WRITE_FULL_TRACK is processed and trkcount (passed as
count to prepare_itcw) exceeds 16, the expression (16 - count) inside
prepare_itcw() evaluates to a negative number:

drivers/s390/block/dasd_eckd.c:prepare_itcw() {
    ...
    if (count > 8) {
        lredata->extended_parameter[0] = 0xFF;
        lredata->extended_parameter[1] = 0xFF;
        lredata->extended_parameter[1] <<= (16 - count);
    ...
}

Does shifting left by a negative amount cause a corrupted extended parameter
bitmask?

Additionally, since extended_parameter_length is hardcoded to 2 bytes,
requests spanning more than 16 tracks may truncate the track bitmask.

> +		/* Clock not in sync and XRC is enabled.
> +		 * Try again later.
> +		 */
> +		ret = -EAGAIN;
> +		goto out_error;
> +	}
[ ... ]
> +	/* process data records */
> +	rec = first_offs + 1;
> +	rec_count = 0;
> +	rq_for_each_segment(bv, req, iter) {

[Severity: High]
Could this cause a NULL pointer dereference if a data-less operation (like
REQ_OP_WRITE_ZEROES) is routed here?

The block subsystem guidelines mention that accesses to bio data fields
require a bio_has_data() guard before iteration, as operations like discard
or write zeroes lack data buffers.

> +		dst = bvec_virt(&bv);
> +		seg_len = bv.bv_len;
> +		while (seg_len) {
[ ... ]
> +			/*
> +			 * This block ends the track; the next one starts a new
> +			 * track. The track-end marker emitted below carries the
> +			 * CBC flag.
> +			 */
> +			if (!len_to_track_end)
> +				new_track = 1;
> +
> +			last_tidaw = add_track_record(itcw, &filldata, &fillsize,
> +						      cyl, head, rec, dst, part_len);

[Severity: Critical]
If a logical block is fragmented across memory boundaries (for example, a
4096-byte block split into two 2048-byte segments due to page boundary
crossing), does this erroneously emit multiple eckd_count headers with
truncated lengths?

Instead of chaining TIDAWs for a single physical record, calling
add_track_record() for each segment piece might advance the record
counter twice and permanently corrupt the track format.

> +			if (!last_tidaw)
> +				goto out_error;
> +

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