Re: [PATCH v5 13/17] s390/dasd: Stamp a format label into newly formatted volumes

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

New issues:
- [Medium] Data race on the `ese_format_quick` flag across concurrent format and space release ioctls.

Pre-existing issues:
- [High] Preexisting heap buffer overflow in dasd_eckd_build_format for non-standard intensities when formatting multiple tracks.
--

--- Patch [13]: [PATCH v5 13/17] s390/dasd: Stamp a format label into newly formatted volumes ---
commit 48510a9c8812b7b8251c801cd33769f3a6c5745d
Author: Stefan Haberland <[email protected]>
s390/dasd: Stamp a format label into newly formatted volumes

When a CDL volume is formatted, write a small on-disk label so the format
can later be recognised by the kernel. The next patch will use this for
ESE detection.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 1df861f80c268..1e349dbec44de 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -2770,6 +2794,15 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
>  
>  	use_prefix = base_priv->features.feature[8] & 0x01;
>  
> +	/*
> +	 * Stamp the format label into R4 of the very first track. Only for CDL
> +	 * (R4 is the first non-special record there), only when this request
> +	 * covers track 0, only for the record-writing format intensities (not
> +	 * track invalidation), and only if the track actually has an R4.
> +	 */
> +	write_label = (intensity & 0x08) && !((intensity & ~0x08) & 0x04) &&
> +		      fdata->start_unit == 0 && rpt > 3;
> +
>  	switch (intensity) {
>  	case 0x00:	/* Normal format */
>  	case 0x08:	/* Normal format, use cdl. */

[Severity: High]
This is a pre-existing issue, but does the calculation for non-standard format
intensities in dasd_eckd_build_format() under-allocate memory?

Looking at intensity 0x01, cplength is allocated as:

	case 0x01:	/* Write record zero and format track. */
	case 0x09:	/* Write record zero and format track, use cdl. */
		cplength = 2 + rpt * nr_tracks;

But the subsequent loop unconditionally adds (1 + rpt) CCWs per track for
intensity 0x01, which exceeds the allocated cplength when nr_tracks > 1.

Can this overflow the heap allocation and corrupt memory if an administrator
issues a BIODASDFMT ioctl with a non-standard intensity across multiple tracks?

> @@ -2816,6 +2849,10 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	/* room for the label data that R4 carries in addition to its count */
> +	if (write_label)
> +		datasize += fdata->blksize;
> +
>  	fcp = dasd_fmalloc_request(DASD_ECKD_MAGIC, cplength, datasize, startdev);

[ ... ]

> @@ -4068,6 +4132,7 @@ dasd_eckd_dso_ras(struct dasd_device *device, struct dasd_block *block,
>  
>  static int dasd_eckd_release_space_full(struct dasd_device *device)
>  {
> +	struct dasd_eckd_private *private;
>  	struct dasd_ccw_req *cqr;
>  	int rc;
>  
> @@ -4079,10 +4144,16 @@ static int dasd_eckd_release_space_full(struct dasd_device *device)
>  
>  	if (!rc) {
>  		/*
> -		 * Releasing all space (RAS) wipes every track and the device is fully
> -		 * sparse again, so restart the heuristic from ft1.
> +		 * Releasing all space (RAS) wipes every track and the device is
> +		 * fully sparse again, so restart the heuristic from ft1.
>  		 */
>  		dasd_ft_bias_apply(device);
> +		/*
> +		 * A full release is what makes a subsequent format a quick
> +		 * (thin) one; remember it so the format label records that.
> +		 */
> +		private = device->private;
> +		private->ese_format_quick = 1;

[Severity: Medium]
Are there concurrent accesses to the ese_format_quick flag during ioctls?

If a user issues concurrent BIODASDRAS and BIODASDFMT ioctls on the same block
device, could they race on reading and writing this lockless variable?

dasd_eckd_release_space_full() sets it to 1 here without locks, while
dasd_eckd_format_device() clears it, and dasd_eckd_fill_format_label() reads
it. Could this lead to a non-deterministic state where a formatted volume is
incorrectly tagged as quick or full in its on-disk label?

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