Re: [PATCH v5 08/17] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK

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

New issues:
- [Medium] The explicit assignment `lredata->length = 0` for `WRITE_FULL_TRACK` is unconditionally overwritten.

Pre-existing issues:
- [High] Lockless access to `conf.ned` and `uid.type` in I/O submission paths races with concurrent path events, leading to a NULL pointer dereference.
--

--- Patch [8]: [PATCH v5 08/17] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK ---
commit 2948648b7fc2c93cb4193e7c55bd961de4d7e8c2
Author: Stefan Haberland <[email protected]>

s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK
    
prepare_itcw() builds the FCX prefix block (PFX + LRE) for track-mode
I/O. Extend it to handle DASD_ECKD_CCW_WRITE_FULL_TRACK.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 2f6d97c06237b..c1a26485f506d 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -4397,26 +4399,31 @@ static int prepare_itcw(struct itcw *itcw,
[ ... ]
> -	pfxdata.format = 1; /* PFX with LRE */
> -	pfxdata.base_address = basepriv->conf.ned->unit_addr;
> -	pfxdata.base_lss = basepriv->conf.ned->ID;
> -	pfxdata.validity.define_extent = 1;
> +	pfxdata->format = 1; /* PFX with LRE */
> +	pfxdata->base_address = basepriv->conf.ned->unit_addr;
> +	pfxdata->base_lss = basepriv->conf.ned->ID;

[Severity: High]
This is a pre-existing issue, but can this lockless access to conf.ned lead
to a NULL pointer dereference?

If a concurrent path event triggers dasd_eckd_pe_handler(), it calls
rebuild_device_uid(), which then calls dasd_eckd_identify_conf_parts().
That function transiently sets conf.ned = NULL without holding any locks.

If prepare_itcw() accesses basepriv->conf.ned->unit_addr at that moment, it
will dereference a NULL pointer.

> +	pfxdata->validity.define_extent = 1;
>  
>  	/* private uid is kept up to date, conf_data may be outdated */
>  	if (startpriv->uid.type == UA_BASE_PAV_ALIAS)

[Severity: High]
This is a pre-existing issue, but does accessing uid.type here race with a
concurrent memset?

The create_uid() function memsets the uid to 0 under the ccwdev_lock, but this
I/O submission path reads startpriv->uid.type locklessly, exposing it to
transient zeroes.

> -		pfxdata.validity.verify_base = 1;
> +		pfxdata->validity.verify_base = 1;
[ ... ]
> @@ -4446,7 +4453,39 @@ static int prepare_itcw(struct itcw *itcw,
[ ... ]
> +	case DASD_ECKD_CCW_WRITE_FULL_TRACK:
> +		dedata->mask.perm = 0x3;
> +		dedata->mask.auth = 0x00;
> +		dedata->attributes.operation = basepriv->attrib.operation;
> +		dedata->blk_size = blksize;
> +		dedata->ga_extended |= 0x42;
> +		rc = set_timestamp(NULL, dedata, basedev);
> +		lredata->operation.orientation = 0x0;
> +		lredata->operation.operation = 0x3F;
> +		lredata->extended_operation = 0x11;
> +		lredata->auxiliary.check_bytes = 0x2;
> +		lredata->length = 0;

[Severity: Medium]
Will this lredata->length value be unconditionally overwritten?

Later in prepare_itcw(), there is this assignment:

    lredata->length = tlf;

If WRITE_FULL_TRACK is used with a non-zero transfer length factor, the
tlf value will overwrite the explicitly requested 0.

> +		lredata->extended_parameter_length = 0x02;

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