Re: [PATCH v2 09/64] dmaengine: ioat: convert cleanup tasklet to dma_chan BH

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

Pre-existing issues:
- [High] The teardown sequence in `ioat_stop()` cancels the timer using `timer_delete_sync()`, but it can be subsequently re-armed by the final cleanup sequence, leading to a use-after-free when channel resources are freed.
- [High] `ioat_shutdown()` does not flush inflight BH runs, which can lead to a use-after-free or crash on module unload.
--

--- Patch [9]: [PATCH v2 09/64] dmaengine: ioat: convert cleanup tasklet to dma_chan BH ---
commit 1d64b3893af8e57fe59c7d2f05e40d26e9aec652
Author: Allen Pais <[email protected]>

dmaengine: ioat: convert cleanup tasklet to dma_chan BH

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
> index ee93b029f9e3d..827b46c6354ad 100644
> --- a/drivers/dma/ioat/dma.c
> +++ b/drivers/dma/ioat/dma.c

[ ... ]

> @@ -161,11 +161,11 @@ void ioat_stop(struct ioatdma_chan *ioat_chan)
>  	/* flush inflight timers */
>  	timer_delete_sync(&ioat_chan->timer);
>  
> -	/* flush inflight tasklet runs */
> -	tasklet_kill(&ioat_chan->cleanup_task);
> +	/* flush inflight BH runs */
> +	dma_chan_kill_bh(&ioat_chan->dma_chan);
>  
>  	/* final cleanup now that everything is quiesced and can't re-arm */
> -	ioat_cleanup_event(&ioat_chan->cleanup_task);
> +	ioat_cleanup_event(&ioat_chan->dma_chan);

[Severity: High]
This is a pre-existing issue, but does this sequence inadvertently re-arm
the timer during teardown?

After timer_delete_sync() is called, ioat_cleanup_event() triggers a call
chain that can re-arm the timer unconditionally:

ioat_stop()
   timer_delete_sync(&ioat_chan->timer);
   ioat_cleanup_event()
      ioat_cleanup()
         ioat_cleanup_preamble()
            mod_timer(&ioat_chan->timer, jiffies + COMPLETION_TIMEOUT);

If the timer is re-armed here, could the delayed timer callback execute
after ioat_free_chan_resources() frees the completion pool memory?

Would upgrading timer_delete_sync() to timer_shutdown_sync() properly
prevent the timer from being re-armed?

[Severity: High]
This is also a pre-existing issue, but does ioat_shutdown() in 
drivers/dma/ioat/init.c have a similar teardown problem with the BH?

In ioat_shutdown(), the driver calls timer_delete_sync() and disables 
interrupts, but it does not appear to flush inflight BH runs like it does 
here in ioat_stop(). 

Since the IOAT_RUN bit is not cleared in ioat_shutdown():

drivers/dma/ioat/init.c:ioat_shutdown() {
    ...
    timer_delete_sync(&ioat_chan->timer);
    ioat_reset_hw(ioat_chan);
    ...
    ioat_disable_interrupts(ioat_dma);
}

Could an interrupt firing just before ioat_disable_interrupts() successfully
schedule the BH via dma_chan_schedule_bh()?

If the module is then unloaded, could the scheduled BH execute after 
release_ioatdma() has freed the channel structures, resulting in a 
use-after-free?

>  }

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