Re: [PATCH v2 62/64] dmaengine: hidma: defer callbacks via channel BH

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

New issues:
- [Critical] The per-channel bottom-half (BH) is cancelled too late during driver unbind, allowing it to execute after the hardware context is destroyed and dereference a NULL pointer.
- [High] Missing synchronization and cancellation of BH during channel termination leads to use-after-free and memory leaks.

Pre-existing issues:
- [Critical] List manipulation without holding the channel lock in `hidma_terminate_channel` causes list corruption and data races.
- [High] Sleeping function called in atomic context and unsafe PM hardware access.
- [High] Unbalanced pm_runtime usage counter causing premature device suspension.
- [High] Invalid pointer dereference due to missing list emptiness check in `hidma_callback`.
--

--- Patch [62]: [PATCH v2 62/64] dmaengine: hidma: defer callbacks via channel BH ---
commit 3c9c3e0127e064313b46165b6ad21a91d6575ed1
Author: Allen Pais <[email protected]>
Subject: dmaengine: hidma: defer callbacks via channel BH

Move descriptor callback processing out of the low-level completion
path and schedule it through per-channel BH work. Drain that work while
freeing channels so callbacks cannot outlive channel storage.

> diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
> index c939635be21d2..1f875a499ab3f 100644
> --- a/drivers/dma/qcom/hidma.c
> +++ b/drivers/dma/qcom/hidma.c
> @@ -90,7 +90,13 @@ static inline struct hidma_chan *to_hidma_chan(struct dma_chan *dmach)
>  
>  static void hidma_free(struct hidma_dev *dmadev)
>  {
> -	INIT_LIST_HEAD(&dmadev->ddev.channels);
> +	struct hidma_chan *mchan, *next;
> +
> +	list_for_each_entry_safe(mchan, next, &dmadev->ddev.channels,
> +				 chan.device_node) {
> +		dma_chan_kill_bh(&mchan->chan);

[Severity: Critical]
Does flushing the bottom half here happen too late in the teardown sequence?

During driver unbind, hidma_remove() calls hidma_ll_uninit() before
hidma_free().

hidma_ll_uninit() explicitly sets lldev->trepool = NULL. When hidma_free()
then calls dma_chan_kill_bh(), it synchronously flushes the pending bottom
half:

hidma_chan_bh()
  hidma_process_completed()
    hidma_ll_status()
      tre = &lldev->trepool[tre_ch];

Since trepool is already NULL, won't this cause a deterministic NULL pointer
dereference?

> +		list_del(&mchan->chan.device_node);
> +	}
>  }
>  
>  static unsigned int nr_desc_prm;

[ ... ]

> @@ -155,6 +161,13 @@ static void hidma_process_completed(struct hidma_chan *mchan)
>  	}
>  }
>  
> +static void hidma_chan_bh(struct dma_chan *chan)
> +{
> +	struct hidma_chan *mchan = to_hidma_chan(chan);
> +
> +	hidma_process_completed(mchan);
> +}
> +

[Severity: High]
Does the bottom half work need to be cancelled when a client terminates the
channel?

When a client calls dma_release_channel() or dmaengine_terminate_sync(), the
framework eventually calls hidma_free_chan_resources(), which frees the
descriptors.

If the bottom half executes concurrently or after this teardown:

hidma_chan_bh()
  hidma_process_completed()
    list_for_each_entry_safe(mdesc, next, &list, node) {
      dmaengine_desc_callback_invoke(&cb, &result);
    }

Won't it dereference descriptors that were just freed, causing a use-after-free,
and subsequently leak orphaned descriptors onto mchan->free?

>  /*
>   * Called once for each submitted descriptor.
>   * PM is locked once for each descriptor that is currently
> @@ -181,7 +194,7 @@ static void hidma_callback(void *data)
>  	}
>  	spin_unlock_irqrestore(&mchan->lock, irqflags);

[Severity: Critical]
This is a pre-existing issue, but does hidma_terminate_channel() safely
handle concurrent callbacks?

In hidma_terminate_channel(), the lock is dropped before the hardware is
paused:

hidma_terminate_channel()
  spin_unlock_irqrestore(&mchan->lock, irqflags);
  rc = hidma_ll_disable(dmadev->lldev);

During this window, a completed transaction can trigger hidma_callback()
which modifies the list without holding a lock on the local list:

hidma_callback()
  if (mdesc->node.next) {
    list_move_tail(&mdesc->node, &mchan->completed);
  }

Since hidma_terminate_channel() subsequently modifies the same list
locklessly, can this race with hidma_prep_dma_memcpy() and cause severe
list corruption?

[Severity: High]
This is a pre-existing issue, but does hidma_callback() correctly handle an
empty active list?

When updating the running descriptor, the code uses list_first_entry():

hidma_callback()
  mchan->running = list_first_entry(&mchan->active,
                                    struct hidma_desc, node);

If mchan->active is empty, list_first_entry() does not return NULL but
rather an invalid pointer computed from the list head itself.

If hidma_tx_status() is subsequently called, won't it dereference this
invalid pointer via mchan->running->desc.cookie and cause an out-of-bounds
memory read?

> -	hidma_process_completed(mchan);
> +	dma_chan_schedule_bh(&mchan->chan);
>  
>  	if (queued) {
>  		pm_runtime_mark_last_busy(dmadev->ddev.dev);
>  		pm_runtime_put_autosuspend(dmadev->ddev.dev);

[Severity: High]
This isn't a bug introduced by this patch, but is the pm_runtime usage
counter decrement balanced with the increment?

In hidma_issue_pending(), pm_runtime_get() is called only once per
submitted batch of descriptors.

However, the hardware triggers hidma_callback() for each individual
descriptor, and pm_runtime_put_autosuspend() is unconditionally called here
for every single descriptor.

If a client submits a batch of multiple descriptors, won't this underflow
the PM usage count and trigger device auto-suspension while hardware
operations are still in flight?

[Severity: High]
This is a pre-existing issue, but does the tasklet safely call sleeping PM
functions?

The driver implements hidma_issue_task() as a tasklet, which runs in
softirq (atomic) context, but it calls pm_runtime_get_sync():

hidma_issue_task()
  pm_runtime_get_sync(dmadev->ddev.dev);

Since pm_runtime_get_sync() will sleep if the device is suspended, doesn't
this trigger a sleeping in atomic bug?

Furthermore, in hidma_issue_pending(), if the asynchronous pm_runtime_get()
returns 0, the code falls through and immediately calls hidma_ll_start().
Doesn't this access hardware registers while the device is still unclocked
or powered off?

>  	}

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