Re: [PATCH v3 12/13] s390/dasd: Re-enable discard support for ESE volumes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing `REQ_OP_DISCARD` check in `dasd_eckd_build_cp_raw()` causes NULL pointer dereference.
- [Critical] `dasd_eckd_free_cp()` crashes on `REQ_OP_DISCARD` completions due to NULL pointer dereference.
- [High] Non-power-of-2 `discard_granularity` breaks block layer alignment macros.
--

--- Patch [12]: [PATCH v3 12/13] s390/dasd: Re-enable discard support for ESE volumes ---
commit 246816ee6050524d3b489a73da17fa2d1d56cbcc
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Re-enable discard support for ESE volumes

Re-enable block-layer discard for ESE ECKD volumes, releasing thin space
via release allocated space (RAS).

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 1832ef5cdeedd..571a693a33a3c 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -5768,11 +5822,13 @@ static struct dasd_ccw_req *dasd_eckd_build_alias_cp(struct dasd_device *base,
>  						     struct request *req)
>  {
>  	struct dasd_eckd_private *private;
> -	struct dasd_device *startdev;
> +	struct dasd_device *startdev = NULL;
>  	unsigned long flags;
>  	struct dasd_ccw_req *cqr;
>  
> -	startdev = dasd_alias_get_start_dev(base);
> +	/* Discard requests (space release) can only run on the base device. */
> +	if (req_op(req) != REQ_OP_DISCARD)
> +		startdev = dasd_alias_get_start_dev(base);
>  	if (!startdev)
>  		startdev = base;
>  	private = startdev->private;

[Severity: Critical]
If the device has DASD_FEATURE_USERAW enabled, does this route discard
requests to dasd_eckd_build_cp_raw() where they will cause a panic?

In dasd_eckd_build_alias_cp(), we fall through to here:

        if ((base->features & DASD_FEATURE_USERAW))
                cqr = dasd_eckd_build_cp_raw(startdev, block, req);

If req is a REQ_OP_DISCARD, dasd_eckd_build_cp_raw() iterates over the data
segments using rq_for_each_segment(bv, req, iter). Because discard bios have
a non-zero bi_size but no data buffers (bi_io_vec is NULL), will the segment
iterator blindly dereference the NULL bi_io_vec and trigger a kernel panic?

[Severity: Critical]
When these discard requests complete, will dasd_eckd_free_cp() crash due to
a similar NULL pointer dereference?

When a request completes, dasd_eckd_free_alias_cp() delegates to
dasd_eckd_free_cp(). If dasd_page_cache is initialized, dasd_eckd_free_cp()
unconditionally attempts to iterate over the request's bio segments:

        rq_for_each_segment(bv, req, iter) {
                ...
        }

Since discard requests have no data buffers (bi_io_vec is NULL) but a
non-zero bi_size, will this iteration dereference the NULL array and panic
on every discard IO completion?

[ ... ]

> @@ -7654,6 +7710,40 @@ static unsigned int dasd_eckd_max_sectors(struct dasd_block *block)
>  	return DASD_ECKD_MAX_BLOCKS << block->s2b_shift;
>  }
>  
> +/*
> + * Discard on ECKD releases space through RAS, which works on whole extents.
> + * Advertise extent granularity so the block layer only sends extent-aligned
> + * discards (avoiding partially specified extents), and only for volumes on ESE
> + * hardware. Non-ESE devices are left without discard limits.
> + */
> +static void dasd_eckd_disc_limits(struct dasd_block *block,
> +				  struct queue_limits *lim)
> +{
> +	struct dasd_device *device = block->base;
> +	struct dasd_eckd_private *private = device->private;
> +	unsigned int logical_block_size = block->bp_block;
> +	unsigned int max_discard_sectors, max_bytes, ext_bytes;
> +	int recs_per_trk, trks_per_cyl, ext_limit, ext_size;
> +
> +	if (!dasd_eckd_ese_capable(device) || dasd_eckd_ext_size(device) == 0)
> +		return;
> +
> +	trks_per_cyl = private->rdc_data.trk_per_cyl;
> +	recs_per_trk = recs_per_track(&private->rdc_data, 0, logical_block_size);
> +
> +	ext_size = dasd_eckd_ext_size(device);
> +	ext_limit = min(private->real_cyl / ext_size, DASD_ECKD_RAS_EXTS_MAX);
> +	ext_bytes = ext_size * trks_per_cyl * recs_per_trk * logical_block_size;
> +	max_bytes = UINT_MAX - (UINT_MAX % ext_bytes);
> +	if (max_bytes / ext_bytes > ext_limit)
> +		max_bytes = ext_bytes * ext_limit;
> +
> +	max_discard_sectors = max_bytes / 512;
> +
> +	lim->max_hw_discard_sectors = max_discard_sectors;
> +	lim->discard_granularity = ext_bytes;

[Severity: High]
Will this non-power-of-two discard granularity break the block layer macros?

The block layer uses round_up() and round_down() with discard_granularity to
calculate payload splits and alignment. These macros rely on bitwise masking
and strictly require the divisor to be a power of two.

Since ext_bytes (e.g., 21 tracks * 15 heads * 12 records * 4096 bytes) is
not a power of two, could this cause the block layer to compute incorrect,
unaligned discard boundaries and split bios into unsupported sizes?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=12