Re: [PATCH 4/4] dmaengine: dmatest: Optionally make test errors fatal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition on `info->params.fatal_errors` can cause an unintended kernel panic when the parameter is concurrently set to 0 ('never panic').
--

--- Patch [4]: [PATCH 4/4] dmaengine: dmatest: Optionally make test errors fatal ---
commit 289cdfd12c204ee46d7f18f6c08608f36bd53956
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: dmatest: Optionally make test errors fatal

This commit adds a module parameter fatal_errors to dmatest. When set to a
non-zero value, the kernel will panic after accumulating that many test errors
during a run.

> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 0d7752c2451a9..a070ce6f2fa94 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c

[ ... ]

> @@ -487,6 +497,24 @@ static void dbg_result(const char *err, unsigned int n, unsigned int src_off,
>  		dbg_result(err, n, src_off, dst_off, len, data);\
>  })
>  
> +/*
> + * Record a test error whose details were just logged via result(). When
> + * fatal_errors is set, panic once that many errors have accumulated in the
> + * current run.
> + */
> +static void dmatest_note_error(struct dmatest_info *info)
> +{
> +	unsigned int errors;
> +
> +	if (!info->params.fatal_errors)
> +		return;
> +
> +	errors = atomic_inc_return(&info->error_count);
> +	if (errors >= info->params.fatal_errors)
> +		panic("dmatest: fatal_errors=%u and %u error(s) encountered\n",
> +		      info->params.fatal_errors, errors);

[Severity: High]
Can a concurrent modification of the fatal_errors parameter cause a spurious
kernel panic here?

If a user modifies the fatal_errors parameter to 0 (meaning 'never panic') via
sysfs while test threads are executing, info->params.fatal_errors could be
concurrently overwritten with 0 via add_threaded_test().

If this happens between the initial check and the second read:

CPU1 in dmatest_note_error()
    if (!info->params.fatal_errors) // reads > 0
        return;

    errors = atomic_inc_return(&info->error_count); // acts as a barrier

CPU2 modifying sysfs
    info->params.fatal_errors = 0;

CPU1 in dmatest_note_error()
    if (errors >= info->params.fatal_errors) // reads 0

Will the condition errors >= 0 evaluate to true and inadvertently trigger the
panic? Could we read info->params.fatal_errors into a local variable once to
prevent this race?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-dmatest-fixes-and-fatal-errors-v1-0-8287709987ea@amd.com?part=4