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

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

[Cosmin Tanislav]

Each dmatest worker thread has a pending and a done flag which describe
the thread's lifecycle.

pending is initialized to true when the thread is added to the threads
list. pending is set to false once the thread is actually running. done
is set to true once the thread has finished its testing.

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

If the worker thread is not started and running by the time
dmatest_init() waits, the !is_threaded_test_run() condition will be true
and the wait will stop early, before the worker thread is even started.
dmatest_init() exits, defeating the whole purpose of the wait.

If then we try to remove the module, kthread_stop() will try to cleanup
the worker threads, interrupting wait_event_freezable_timeout(), which
will return -ERESTARTSYS, causing the "test timed out" message to be
printed.

Clear the pending flag once we wake up the worker thread to avoid this
race.

Fixes: d53513d5dc28 ("dmaengine: dmatest: Add support for multi channel testing")
Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
---

V2:
 * move pending = false before wake_up_process() as suggested by Sashiko

 drivers/dma/dmatest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 2ae3469397f3..f654a00e0040 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -610,7 +610,6 @@ static int dmatest_func(void *data)
 	ret = -ENOMEM;
 
 	smp_rmb();
-	thread->pending = false;
 	info = thread->info;
 	params = &info->params;
 	chan = thread->chan;
@@ -1141,6 +1140,7 @@ static void run_pending_tests(struct dmatest_info *info)
 
 		thread_count = 0;
 		list_for_each_entry(thread, &dtc->threads, node) {
+			thread->pending = false;
 			wake_up_process(thread->task);
 			thread_count++;
 		}
-- 
2.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless list traversal in `is_threaded_test_run()` leads to a Use-After-Free if tests are stopped concurrently via sysfs.
- [High] DMA buffers are unmapped while the DMA channel might still be active on test timeout.
--

commit 4cbd6a56dbdbb6c68ca92f35dcbdd675bb35d0b1
Author: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>

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

This commit clears the pending flag before waking up the worker thread to
prevent a race condition where dmatest_init() stops waiting early. This avoids
a situation where kthread_stop() interrupts the wait during module removal,
which previously caused an incorrect timeout message.

> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 2ae3469397f37..f654a00e00404 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -610,7 +610,6 @@ static int dmatest_func(void *data)
>  	ret = -ENOMEM;
>  
>  	smp_rmb();
> -	thread->pending = false;
>  	info = thread->info;
>  	params = &info->params;
>  	chan = thread->chan;

[Severity: High]
This isn't a bug introduced by this patch, but it is a pre-existing issue.
Are the DMA buffers unmapped while the DMA channel might still be active on a
test timeout?

When a test times out (which this patch helps prevent during teardown),
dmatest_func() jumps to unmap the buffers:

drivers/dma/dmatest.c:dmatest_func() {
    ...
		if (!done->done) {
			result("test timed out", total_tests, src->off, dst->off,
			       len, 0);
			goto error_unmap_continue;
		}
    ...
error_unmap_continue:
		dmaengine_unmap_put(um);
		failed_tests++;
    ...
}

However, the hardware DMA transfer is never explicitly terminated via
dmaengine_terminate_sync() before continuing to the next iteration.

If swiotlb bounce buffers are in use, unmapping returns the bounce buffer to
the free pool while the DMA engine is still running and writing to it. When
the bounce buffer is reallocated to another device (e.g., a network card), the
stuck DMA engine will corrupt that device's data.

Could we explicitly terminate the DMA transfer on timeout before unmapping?

> @@ -1141,6 +1140,7 @@ static void run_pending_tests(struct dmatest_info *info)
>  
>  		thread_count = 0;
>  		list_for_each_entry(thread, &dtc->threads, node) {
> +			thread->pending = false;
>  			wake_up_process(thread->task);
>  			thread_count++;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but it is a pre-existing issue.
Does wait_event() evaluate is_threaded_test_run() locklessly, causing a
use-after-free if tests are stopped concurrently via sysfs?

As mentioned in the commit message, dmatest_init() calls wait_event():

drivers/dma/dmatest.c:dmatest_init() {
    ...
	if (params->iterations && wait)
		wait_event(thread_wait, !is_threaded_test_run(info));
    ...
}

The wait_event() macro evaluates the condition continuously by traversing the
info->channels and dtc->threads lists locklessly:

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

Concurrently, an unprivileged or privileged user can write to module parameters
(e.g., echo 0 > /sys/module/dmatest/parameters/run), which acquires
info->lock, calls stop_threaded_test(), removes the elements from the list
with list_del(), and frees them.

drivers/dma/dmatest.c:stop_threaded_test() {
    ...
	list_for_each_entry_safe(dtc, _dtc, &info->channels, node) {
		list_del(&dtc->node);
        ...
		dmatest_cleanup_channel(dtc);
    ...
}

This concurrent modification drops the element from the list and poisons
pointers before freeing, leading to a use-after-free for the lockless reader.
Could we use appropriate locking when evaluating the condition in wait_event()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828151024.2081478-1-cosmin-gabriel.tanislav.xa@renesas.com?part=1