Re: [PATCH v4 02/33] dmaengine: back channel BH helpers with WQ_BH

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] dmaengine: dmaengine_kill_bh() returns with bh_work still
  queued on re-schedule
- [Medium] dmaengine: flush_work() in dmaengine_kill_bh() triggers flush
  dependency warn
--

commit e183b161b65ac0da135cdea21e6916bb3483d703
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: back channel BH helpers with WQ_BH

This commit replaces the per-channel BH tasklet implementation with a
dedicated WQ_BH | WQ_PERCPU workqueue while keeping the dmaengine_*_bh() API
unchanged.

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index f3c1283b161b6..97d8f9667ae5f 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
[ ... ]
> @@ -1449,9 +1451,34 @@ static void dmaengine_destroy_unmap_pool(void)
[ ... ]
> +static void dmaengine_flush_bh_work(struct work_struct *work)
> +{
> +	if (!work)
> +		return;
> +
> +	flush_work(work);
> +}
> +
> +static void dma_chan_bh_entry(struct work_struct *work)
> +{
> +	struct dma_chan *chan = container_of(work, struct dma_chan, bh_work);
>  	dmaengine_bh_work_fn fn = READ_ONCE(chan->bh_work_fn);
>  
>  	if (fn)
> @@ -1467,7 +1494,7 @@ void dmaengine_init_bh(struct dma_chan *chan, dmaengine_bh_work_fn fn)
[ ... ]
> @@ -1487,7 +1513,7 @@ void dmaengine_kill_bh(struct dma_chan *chan)
>  	if (!chan->bh_work_initialized)
>  		return;
>  
> -	tasklet_kill(&chan->bh_tasklet);
> +	dmaengine_flush_bh_work(&chan->bh_work);
>  }
>  EXPORT_SYMBOL_GPL(dmaengine_kill_bh);

[Severity: High]
Can dmaengine_kill_bh() return with chan->bh_work still queued on the
workqueue?

Unlike tasklet_kill(), which holds TASKLET_STATE_SCHED while waiting for a
running tasklet to finish so concurrent tasklet_schedule() calls cannot
re-queue it, process_one_work() clears WORK_STRUCT_PENDING_BIT before
invoking dma_chan_bh_entry().  When flush_work() runs while chan->bh_work is
executing, insert_wq_barrier() places the flush barrier on worker->scheduled,
ahead of pool->worklist:

CPU0                                  CPU1 (bh_worker)
                                      process_one_work(&chan->bh_work)
                                        set_work_pool_and_clear_pending()
                                        dma_chan_bh_entry()
dmaengine_kill_bh(chan)
  flush_work(&chan->bh_work)
    insert_wq_barrier()
                                          ...
                                            dmaengine_schedule_bh(chan)
                                              queue_work(..., &chan->bh_work)
                                        /* dma_chan_bh_entry() returns */
                                      process_scheduled_works()
                                        /* runs barrier from
                                         * worker->scheduled */
  /* flush_work() returns */
kfree(chan)
                                      /* processes pool->worklist */
                                      dma_chan_bh_entry()
                                        chan->bh_work_fn  <-- use-after-free

This is a regression when chan->bh_work is re-queued while
dma_chan_bh_entry() is running, for example when a BH callback reschedules
itself (such as dma_tasklet() in ste_dma40 or mv_chan_slot_cleanup() in
mv_xor) or when a driver calls dmaengine_kill_bh() before tearing down its
IRQ or timer (such as mtk_cqdma_remove() or tegra_adma_free_chan_resources()).
Using cancel_work_sync(&chan->bh_work) or disable_work_sync(&chan->bh_work)
depending on whether the channel may be used again would wait for any
in-flight execution while preventing or draining concurrent re-schedules.

> @@ -1687,15 +1713,28 @@ EXPORT_SYMBOL_GPL(dma_run_dependencies);
>  
>  static int __init dma_bus_init(void)
>  {
> -	int err = dmaengine_init_unmap_pool();
> +	int err;
>  
> +	dmaengine_bh_wq = alloc_workqueue("dmaengine_bh",
> +					  WQ_BH | WQ_PERCPU, 0);

[Severity: Medium]
Does calling dmaengine_kill_bh() from a WQ_MEM_RECLAIM workqueue or a
PF_MEMALLOC task trigger a WARN_ONCE() in check_flush_dependency()?

dmaengine_bh_wq does not have WQ_MEM_RECLAIM set (and WQ_BH workqueues cannot
set WQ_MEM_RECLAIM).  Because dmaengine_kill_bh() calls flush_work(), which
passes from_cancel = false to __flush_work() -> check_flush_dependency(),
flushing a pending or running chan->bh_work from a WQ_MEM_RECLAIM context
hits the dependency warning:

kernel/workqueue.c:check_flush_dependency() {
	if (from_cancel || target_wq->flags & WQ_MEM_RECLAIM)
		return;

	WARN_ONCE(current->flags & PF_MEMALLOC, ...);
	WARN_ONCE((worker && ((worker->current_pwq->wq->flags &
			       (WQ_MEM_RECLAIM | __WQ_LEGACY)) == WQ_MEM_RECLAIM)), ...);
}

For example, sdhci allocates host->complete_wq with WQ_MEM_RECLAIM and runs
sdhci_complete_work() -> sdhci_request_done() -> dmaengine_terminate_sync()
-> dmaengine_synchronize() -> vchan_synchronize() -> dmaengine_kill_bh().
Using cancel_work_sync() or disable_work_sync() in dmaengine_kill_bh() sets
from_cancel = true and avoids this warning.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794315350.git.allen.lkml@gmail.com?part=2