Re: [PATCH v3 09/34] dmaengine: ioat: convert cleanup tasklet to dmaengine BH

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

Pre-existing issues:
- [High] The channel timer can be inadvertently re-armed during the teardown sequence in `ioat_stop()`, leading to a use-after-free when the timer fires after the channel resources are freed.
--

--- Patch [9]: [PATCH v3 09/34] dmaengine: ioat: convert cleanup tasklet to dmaengine BH ---
commit a9e2b8b1c21fd37939157d4bfd1e3ccc9c58d182
Author: Allen Pais <[email protected]>

dmaengine: ioat: convert cleanup tasklet to dmaengine BH
    
This commit replaces the per-channel tasklet with the shared dmaengine
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
> --- 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 */
> +	dmaengine_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 can this teardown sequence result in a
use-after-free by inadvertently re-arming the timer?

In ioat_stop(), we use timer_delete_sync() to stop the timer. However,
we then manually call ioat_cleanup_event(), which eventually reaches
ioat_cleanup_preamble(). If there are hardware completions pending, it
will unconditionally re-arm the timer:

ioat_stop()
  ioat_cleanup_event()
    ioat_cleanup()
      ioat_cleanup_preamble() {
        ...
        clear_bit(IOAT_COMPLETION_ACK, &ioat_chan->state);
        mod_timer(&ioat_chan->timer, jiffies + COMPLETION_TIMEOUT);
        ...
      }

Since timer_delete_sync() doesn't permanently prevent re-arming, the
timer could be re-armed here right before the device is torn down and
ioat_chan is freed.

Should this code use timer_shutdown_sync() instead to permanently
deactivate the timer and prevent mod_timer() from succeeding during
this final cleanup phase?

>  }

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