Re: [PATCH v6 10/18] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

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

New issues:
- [Critical] Missing bio_has_data() guard before iterating request segments.
- [High] Negative bitshift in prepare_itcw() exposed by unrestricted track count.
- [High] Premature loop termination and disk format corruption due to split bio_vec segments being mapped 1:1 to disk records.
- [Medium] Silent bypass of errors from prepare_itcw() other than -EAGAIN.

Pre-existing issues:
- [Critical] Pre-existing missing bio_has_data() guards in CMD and TPM track builders.
--

--- Patch [10]: [PATCH v6 10/18] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() ---
commit c49275335fe037682ab8442e739559068d7eb61c
Author: Stefan Haberland <[email protected]>
Subject: s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

This commit adds the channel program builder for WRITE_FULL_TRACK requests.
It builds an ITCW with a TIDAW list for formatting and writing tracks
atomically on ESE devices.

> --- 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,
[ ... ]
> +	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: Medium]
Does this code handle other negative error codes from prepare_itcw() properly?

If prepare_itcw() returns an error like -EINVAL, the check for -EAGAIN
evaluates to false, and the function will continue as if it succeeded. Could
this submit an incomplete ITCW program to the hardware?

[Severity: High]
Can this lead to a negative bitshift in prepare_itcw()?

The trkcount variable is calculated based on last_trk - first_trk + 1 and
passed as the count parameter to prepare_itcw(). Looking at prepare_itcw()
for the DASD_ECKD_CCW_WRITE_FULL_TRACK case:

prepare_itcw() {
    ...
    case DASD_ECKD_CCW_WRITE_FULL_TRACK:
        ...
        if (count > 8) {
            lredata->extended_parameter[0] = 0xFF;
            lredata->extended_parameter[1] = 0xFF;
            lredata->extended_parameter[1] <<= (16 - count);

If a request spans more than 16 tracks, the count passed in will exceed 16,
resulting in a negative shift amount. Should there be bounds checking on
trkcount before passing it here?

[ ... ]
> +	rec_count = 0;
> +	rq_for_each_segment(bv, req, iter) {

[Severity: Critical]
Does this loop need to verify that the bio actually contains data?

According to block layer subsystem guidelines, iterating segments without
checking bio_has_data() can cause a NULL pointer dereference for operations
that have no data buffers (such as REQ_OP_WRITE_ZEROES). Even if op_is_write()
is true, data fields shouldn't be accessed without this guard.

[ ... ]
> +			last_tidaw = add_track_record(itcw, &filldata, &fillsize,
> +						      cyl, head, rec, dst, part_len);
> +			if (!last_tidaw)
> +				goto out_error;
[ ... ]
> +			rec++;
> +			dst += part_len;
> +			rec_count++;
> +			if (rec_count >= (last_rec - first_rec + 1))
> +				goto out_skip;
> +		}
> +	}

[Severity: High]
Could this terminate the loop prematurely and corrupt the disk format?

If a logical block crosses a page boundary, the block layer will split it into
multiple smaller bio_vec segments. Because this loop increments rec_count for
every segment iteration and emits a track record, split segments would be
written as multiple short records rather than a single full block.

Should rec_count be incremented based on the accumulated logical block size
instead of mapping one-to-one with the bio_vec segments?

[ ... ]
> static struct dasd_ccw_req *dasd_eckd_build_cp_cmd_track(
[ ... ]
> 	rq_for_each_segment(bv, req, iter) {

[Severity: Critical]
This is a pre-existing issue, but does this function also need a
bio_has_data() guard before iterating segments?

Similar to the newly added dasd_eckd_build_cp_tpm_writefulltrack(),
iterating over request segments here after only checking
rq_data_dir(req) == WRITE will cause a NULL pointer dereference on
operations like REQ_OP_DISCARD or REQ_OP_WRITE_ZEROES that do not carry
data buffers.

[ ... ]
> static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(
[ ... ]
> 	if (rq_data_dir(req) == WRITE) {
> 		new_track = 1;
> 		recid = first_rec;
> 		rq_for_each_segment(bv, req, iter) {

[Severity: Critical]
This is also a pre-existing issue, but does the loop in
dasd_eckd_build_cp_tpm_track() suffer from the same missing bio_has_data()
guard?

Operations without data buffers will trigger a NULL pointer dereference
during segment iteration here as well.

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