Re: [PATCH 2/4] dmaengine: dmatest: Suppress bogus failures when stopping a run

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bypassing the error path during thread stop prevents dmaengine_terminate_sync() from being called, leading to a Use-After-Free of the dmatest_thread struct by the completion callback.
- [Medium] Bypassed DMA status check when stopping results in unhandled DMA errors.
--

--- Patch [2]: [PATCH 2/4] dmaengine: dmatest: Suppress bogus failures when stopping a run ---
commit 65f43e5ca342a54064656c1ebe548edaa5c088bd
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: dmatest: Suppress bogus failures when stopping a run

Writing 0 to the module 'run' attribute stops the test threads with
kthread_stop(). This commit introduces checks against kthread_should_stop()
to suppress stale failures against transfers that actually succeeded.

> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
[ ... ]
> @@ -848,14 +848,19 @@ static int dmatest_func(void *data)
>  					done->done,
>  					msecs_to_jiffies(params->timeout));
>  
> -			status = dma_async_is_tx_complete(chan, cookie, NULL,
> -							  NULL);
> +			stopping = kthread_should_stop();
> +			status = stopping ?
> +				dma_sync_wait(chan, cookie) :
> +				dma_async_is_tx_complete(chan, cookie, NULL, NULL);
>  		}
>  
>  		if (!done->done) {
> -			result("test timed out", total_tests, src->off, dst->off,
> -			       len, 0);
> -			goto error_unmap_continue;
> +			/* stopping: dma_sync_wait() let the transfer finish, not a timeout */
> +			if (!stopping) {
> +				result("test timed out", total_tests, src->off,
> +				       dst->off, len, 0);
> +				goto error_unmap_continue;
> +			}

[Severity: Medium]
By skipping this error path when stopping is true, does this bypass evaluating
the status returned from dma_sync_wait()?

If a legitimate hardware error or timeout occurs exactly when the thread is
stopped, it looks like it would either falsely report a successful test (if
noverify is set) or misreport it as a data mismatch.

[Severity: High]
Could skipping the goto error_unmap_continue here lead to a use-after-free
in a delayed callback?

If a delayed callback leaves done->done false when stopping, failed_tests
won't be incremented. Later, when dmatest_func() exits:

    if (ret || failed_tests)
        dmaengine_terminate_sync(chan);

Since failed_tests remained 0, the thread exits without synchronizing the
channel. Then dmatest_cleanup_channel() executes:

    put_task_struct(thread->task);
    kfree(thread);
}

/* terminate all transfers on specified channels */
dmaengine_terminate_sync(dtc->chan);

Is there a window where a pending tasklet executes dmatest_callback()
after kfree(thread) but before dmaengine_terminate_sync() is finally called,
leading to a use-after-free on the thread structure?

>  		} else if (status != DMA_COMPLETE &&
>  			   !(dma_has_cap(DMA_COMPLETION_NO_ORDER,
>  					 dev->cap_mask) &&

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