Re: [PATCH 13/19] s390/dasd: Add full_track_bias to control fulltrack write mode

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Using `rq_data_dir(req) == WRITE` to gate data buffer accesses allows payload-less requests (such as DISCARD or WRITE_ZEROES) to bypass type safety, leading to a potential NULL pointer dereference.
- [Medium] Inconsistent initialization of `fulltrack` between the module parameter logic and the sysfs store function, leading to divergent behavior for the exact same configuration value.
- [Medium] Concurrent sysfs writes to `full_track_bias` can leave `ft_bias` and `fulltrack` in a permanently inconsistent state.
--

--- Patch [13]: [PATCH 13/19] s390/dasd: Add full_track_bias to control fulltrack write mode ---
commit d2feac141aaa53ff2a5f42b4d0748a881207c899
Author: Stefan Haberland <[email protected]>

s390/dasd: Add full_track_bias to control fulltrack write mode

This patch introduces a per-device full_track_bias sysfs attribute to control
the full-track write path for DASD ECKD devices. It routes aligned full-track
writes through a new function based on the bias value. The patch also adds a
module parameter to set the default bias for all devices at online time.

> diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
> index 381d616ad433f..035c022255b68 100644
> --- a/drivers/s390/block/dasd_devmap.c
> +++ b/drivers/s390/block/dasd_devmap.c

[ ... ]

> @@ -1630,6 +1630,44 @@ dasd_expires_store(struct device *dev, struct device_attribute *attr,

[ ... ]

> +static ssize_t full_track_bias_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct dasd_device *device;
> +	unsigned int val;
> +
> +	if (kstrtouint(buf, 0, &val) || val > DASD_FT_BIAS_MAX)
> +		return -EINVAL;
> +
> +	device = dasd_device_from_cdev(to_ccwdev(dev));
> +	if (IS_ERR(device))
> +		return -ENODEV;
> +
> +	device->ft_bias = val;
> +	device->fulltrack = val ? 1 : 0;

[Severity: Medium]
Can concurrent sysfs writes to full_track_bias leave ft_bias and fulltrack
in a permanently inconsistent state?

If CPU1 writes 100 and CPU2 writes 0 at the same time, could they interleave
their updates to these two fields, leaving ft_bias at 0 and fulltrack at 1,
or vice-versa?

[Severity: Medium]
Does the logic here mismatch how fulltrack is initialized during online time?

In dasd_eckd_check_characteristics(), fulltrack is set to 1 only when the
bias is >= DASD_FT_BIAS_MAX (100). However, in this sysfs store function,
fulltrack is set to 1 for any non-zero value. Should these conditions match
to prevent the exact same configuration value from producing different
behaviors?

> +
> +	dasd_put_device(device);
> +	return count;
> +}

[ ... ]

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index da488c0775fcd..067ab66209b65 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -5204,11 +5221,20 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
>  		/* do nothing, just fall through to the cmd mode single case */
>  	} else if ((data_size <= private->fcx_max_data)
>  		   && (fcx_multitrack || (first_trk == last_trk))) {
> -		cqr = dasd_eckd_build_cp_tpm_track(startdev, block, req,
> -						    first_rec, last_rec,
> -						    first_trk, last_trk,
> -						    first_offs, last_offs,
> -						    blk_per_trk, blksize);
> +		if (!first_offs && (last_offs + 1 == blk_per_trk) &&
> +		    rq_data_dir(req) == WRITE && basedev->fulltrack) {

[Severity: High]
Can payload-less requests (such as DISCARD or WRITE_ZEROES) bypass type
safety here?

The rq_data_dir() check relies on op_is_write(), which returns true for
dataless operations. If dasd_eckd_build_cp_tpm_writefulltrack() then iterates
over the bio segments without verifying bio_has_data(), could this cause a
NULL pointer dereference when accessing the data fields?

> +			cqr = dasd_eckd_build_cp_tpm_writefulltrack(startdev, block, req,
> +								    first_rec, last_rec,
> +								    first_trk, last_trk,
> +								    first_offs, last_offs,
> +								    blk_per_trk, blksize, NULL);
> +		} else {
> +			cqr = dasd_eckd_build_cp_tpm_track(startdev, block, req,
> +							   first_rec, last_rec,
> +							   first_trk, last_trk,
> +							   first_offs, last_offs,
> +							   blk_per_trk, blksize);
> +		}

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