Re: [PATCH] dmaengine: dmatest: fix race between wait for thread and thread start

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

New issues:
- [Medium] TOCTOU race in pending flag update allows premature wait completion.

Pre-existing issues:
- [High] Lockless list traversal in wait condition causes Use-After-Free kernel crash.
--

--- Patch [1]: [PATCH] dmaengine: dmatest: fix race between wait for thread and thread start ---
commit 6a5e8f24ddb4b9e471d1801d58f2c91b07f83d13
Author: Cosmin Tanislav <[email protected]>

dmaengine: dmatest: fix race between wait for thread and thread start

This commit addresses a race condition in the dmatest module where the wait
module parameter could cause an early exit if the worker thread has not yet
started. It attempts to resolve this by clearing the pending flag immediately
after waking up the worker thread.

> When the wait module parameter is set to true, dmatest_init() waits for
> the !is_threaded_test_run() condition to become true.

[Severity: High]
This is a pre-existing issue, but does evaluating this condition repeatedly
in wait_event() without holding a lock lead to a use-after-free?

Looking at is_threaded_test_run(), it iterates over info->channels and
dtc->threads without holding info->lock:

drivers/dma/dmatest.c:is_threaded_test_run() {
    ...
    list_for_each_entry(dtc, &info->channels, node) {
        ...
    }
    ...
}

If stop_threaded_test() is executed concurrently via sysfs, it can modify
the list nodes. Since the traversal in is_threaded_test_run() happens
without holding the lock, could this hit freed memory and result in a
direct use-after-free?

> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 2ae3469397f37..2d2eb43eee7cc 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
[ ... ]
> @@ -1142,6 +1141,7 @@ static void run_pending_tests(struct dmatest_info *info)
>  		thread_count = 0;
>  		list_for_each_entry(thread, &dtc->threads, node) {
>  			wake_up_process(thread->task);
> +			thread->pending = false;

[Severity: Medium]
Could this order of operations introduce a race window?

If wake_up_process() makes the thread runnable immediately, could a
concurrent wait_event() evaluate a stale pending state before it is
cleared here?

Would swapping these operations to clear the pending flag strictly before
the wakeup prevent concurrent wait_event() evaluations from exiting
prematurely?

>  			thread_count++;
>  		}
>  		pr_info("Started %u threads using %s\n",

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