Re: [PATCH 3/4] dmaengine: dmatest: Terminate channel before freeing threads

[sashiko-bot@kernel.org]

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

New issues:
- [High] Spurious WARN_ON due to racing callback execution and thread shutdown.

Pre-existing issues:
- [High] Incomplete fix leaves Use-After-Free for drivers lacking terminate_all support.
--

--- Patch [3]: [PATCH 3/4] dmaengine: dmatest: Terminate channel before freeing threads ---
commit 45380ad31d07f2d946845ad6c99217e3ae5ea8ae
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: dmatest: Terminate channel before freeing threads

This commit modifies dmatest_cleanup_channel() to stop all threads first,
flush the channel, and only then free the thread memory. This prevents
pending callbacks from accessing freed memory.

> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 3267c7e86f5d5..0d7752c2451a9 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -959,18 +959,23 @@ static void dmatest_cleanup_channel(struct dmatest_chan *dtc)
>  	struct dmatest_thread	*_thread;
>  	int			ret;
>  
> -	list_for_each_entry_safe(thread, _thread, &dtc->threads, node) {
> +	/* stop all threads first */
> +	list_for_each_entry(thread, &dtc->threads, node) {
>  		ret = kthread_stop(thread->task);
>  		pr_debug("thread %s exited with status %d\n",
>  			 thread->task->comm, ret);
> +	}
> +
> +	/* flush the channel (e.g. vchan_synchronize() -> tasklet_kill()) */
> +	dmaengine_terminate_sync(dtc->chan);

[Severity: High]
Could this trigger a spurious WARN_ON when flushing pending callbacks?

When a test is stopped, kthread_stop() runs, and dmatest_func() sets
thread->done to true before exiting.

dmatest_cleanup_channel() then calls dmaengine_terminate_sync() which flushes
any pending callbacks.

Since thread->done is already true, will the pending dmatest_callback() read
it and incorrectly fall into the warning path?

drivers/dma/dmatest.c:dmatest_callback() {
    ...
    } else {
        /* ... */
        WARN(1, "dmatest: Kernel memory may be corrupted!!\n");
    }
}

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
dmaengine_terminate_sync() leave a use-after-free risk for drivers that lack
terminate_all support?

If a driver doesn't support device_terminate_all, dmaengine_terminate_sync()
returns -EINVAL and skips dmaengine_synchronize():

include/linux/dmaengine.h:dmaengine_terminate_sync() {
    ...
    ret = dmaengine_terminate_async(chan);
    if (ret)
        return ret;

    dmaengine_synchronize(chan);
    return 0;
}

If synchronization is skipped, couldn't the hardware transfer still finish
and fire the tasklet after we proceed to kfree(thread) below, causing
dmatest_callback() to access freed memory?

> +
> +	/* now it is safe to free the thread memory */
> +	list_for_each_entry_safe(thread, _thread, &dtc->threads, node) {
>  		list_del(&thread->node);
>  		put_task_struct(thread->task);
>  		kfree(thread);
>  	}

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