Re: [PATCH v3 08/13] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

[sashiko-bot@kernel.org]

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

New issues:
- [High] Writing to the `full_track_bias` sysfs attribute triggers a NULL pointer dereference if the DASD device is offline or has no discipline assigned.
- [High] The adaptive heuristic FSM state and configuration variables are modified concurrently without synchronization, leading to permanent state corruption.

Pre-existing issues:
- [High] The condition in `dasd_release_space()` incorrectly uses `&&` instead of `||`, breaking validation and evaluating a NULL function pointer for non-ESE disciplines.
--

--- Patch [8]: [PATCH v3 08/13] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias ---
commit 6212107d38eb431780604d50fae290b46a53ceff
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

Turn the middle of the ft_bias range (1..99) into an adaptive heuristic
that switches between fulltrack write (ft1) and plain write ft0 depending
on how sparse the device still is. A state machine measures the NRF rate
in short ft0 probe windows and flips back to ft1 when it is high.

> diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
> index 035c022255b68..c587817f5cf9e 100644
> --- a/drivers/s390/block/dasd_devmap.c
> +++ b/drivers/s390/block/dasd_devmap.c
[ ... ]
> @@ -1660,7 +1660,7 @@ static ssize_t full_track_bias_store(struct device *dev,
>  		return -ENODEV;
>  
>  	device->ft_bias = val;
> -	device->fulltrack = val ? 1 : 0;
> +	dasd_ft_bias_apply(device);

[Severity: High]
Is it possible for device->discipline to be NULL when the device is offline?

If a user writes to the full_track_bias sysfs attribute while the device is
offline, dasd_ft_bias_apply() calls dasd_ese_adaptive(device) which directly
dereferences device->discipline->is_ese:

drivers/s390/block/dasd_int.h:dasd_ese_adaptive() {
    return device->discipline->is_ese &&
           device->discipline->is_ese(device) && ...
}

Could this result in a NULL pointer dereference during the sysfs write?

>  
>  	dasd_put_device(device);
>  	return count;
[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index c10fe20e7d3ff..8b5f592b04d95 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -3201,6 +3208,65 @@ static void clear_format_track(struct dasd_format_entry *format,
>  	spin_unlock_irqrestore(&block->format_lock, flags);
>  }
>  
> +/*
> + * Adaptive ft_bias heuristic, called once per IO from dasd_eckd_build_cp().
> + * Probes the device formatting state by briefly switching to ft0 and measuring
> + * the NRF rate; parameters are derived from ft_bias.
> + */
> +static void dasd_ese_heuristic_tick(struct dasd_device *basedev)
> +{
> +	int ios, nrf, rate;
> +
> +	if (atomic_inc_return(&basedev->ese_io_cnt) < (int)basedev->ese_probe_interval)
> +		return;
> +
> +	/*
> +	 * One wins the race to evaluate, the rest see ios == 0 after the
> +	 * xchg and return early, preventing redundant state transitions.
> +	 */
> +	ios = atomic_xchg(&basedev->ese_io_cnt, 0);
> +	if (ios <= 0)
> +		return;
> +
> +	switch (basedev->ese_probe_state) {
> +	case DASD_ESE_HEU_FT1_ACTIVE:
> +		/* Start ft0 probe window, reset NRF counter for clean measurement */
> +		basedev->fulltrack          = 0;
> +		basedev->ese_probe_state    = DASD_ESE_HEU_PROBING;

[Severity: High]
Is there a race condition here between this state update and a concurrent
sysfs write to full_track_bias?

If a sysfs write occurs concurrently with a block I/O request executing
dasd_ese_heuristic_tick(), the I/O thread might read DASD_ESE_HEU_FT1_ACTIVE.

If the sysfs thread then updates ft_bias (for example, to 100) and forces
fulltrack = 1 via dasd_ft_bias_apply(), the I/O thread could subsequently
overwrite fulltrack = 0 and change the FSM state. 

Since ft_bias is now 100, dasd_ese_adaptive() will evaluate to false for
future I/Os, meaning the heuristic tick is never called again and fulltrack
remains stuck at 0. Does this need synchronization to prevent permanently
locking the heuristic in the wrong state?

> +		basedev->ese_probe_interval = basedev->ese_heu_probe_window;
> +		atomic_set(&basedev->ese_nrf_window, 0);
> +		break;

[Severity: High]
This is a pre-existing issue, but looking at dasd_release_space() in
drivers/s390/block/dasd_ioctl.c:

    if (!device->discipline->is_ese && !device->discipline->is_ese(device))
        return -ENOTSUPP;

Does this condition incorrectly use && instead of ||? 

If the discipline does not define the is_ese callback (such as for FBA
devices), the first half evaluates to true. Because of the &&, it then
evaluates the second half, which calls the NULL function pointer. Could an
ioctl invoking dasd_release_space() on such a device cause a kernel panic?

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