Re: [PATCH v3 32/34] dmaengine: hidma: defer callbacks via channel BH

[sashiko-bot@kernel.org]

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

New issues:
- [High] Use-After-Free of client callbacks and descriptors due to missing BH synchronization on channel release.
- [High] NULL pointer dereference in `hidma_remove` due to incorrect teardown ordering.

Pre-existing issues:
- [High] List corruption due to data race on `mchan->free` between channel termination and BH execution.
--

--- Patch [32]: [PATCH v3 32/34] dmaengine: hidma: defer callbacks via channel BH ---
commit d556674c4ac10d8d6acb039e370582921643ddd7
Author: Allen Pais <allen.lkml@gmail.com>

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 7a7f302a96991..82790682cfc88 100644
> --- a/drivers/dma/qcom/hidma.c
> +++ b/drivers/dma/qcom/hidma.c
> @@ -91,7 +91,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) {
> +		dmaengine_kill_bh(&mchan->chan);

[Severity: High]
Can this cause a NULL pointer dereference during device teardown?

During module unload, hidma_remove() calls hidma_ll_uninit() before
hidma_free():

drivers/dma/qcom/hidma.c:hidma_remove() {
    ...
    hidma_ll_uninit(dmadev->lldev);
    hidma_free(dmadev);
    ...
}

hidma_ll_uninit() clears the lower-level structures and sets lldev->trepool
to NULL.

When hidma_free() then calls dmaengine_kill_bh(), it flushes any pending
BH work, which calls hidma_process_completed() and eventually
hidma_ll_status().

drivers/dma/qcom/hidma_ll.c:hidma_ll_status() {
    ...
    tre = &lldev->trepool[tre_ch];
    ...
}

Since lldev->trepool is already NULL, does this crash if there is pending
work?

> +		list_del(&mchan->chan.device_node);
> +	}
>  }
>  
>  static unsigned int nr_desc_prm;
> @@ -156,6 +162,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]
This is a pre-existing issue, but does moving the completion handling to an
asynchronous BH significantly widen a data race window?

In hidma_terminate_channel(), descriptors are moved to the free list without
holding mchan->lock:

drivers/dma/qcom/hidma.c:hidma_terminate_channel() {
    ...
        /* move myself to free_list */
        list_move(&mdesc->node, &mchan->free);
    ...
}

If a client terminates a channel while a completion is pending, this unlocked
access can run concurrently with the BH executing hidma_process_completed():

drivers/dma/qcom/hidma.c:hidma_process_completed() {
    ...
        spin_lock_irqsave(&mchan->lock, irqflags);
        list_move(&mdesc->node, &mchan->free);
        spin_unlock_irqrestore(&mchan->lock, irqflags);
    ...
}

Can this concurrent modification corrupt the mchan->free list?

> +}
> +
>  /*
>   * Called once for each submitted descriptor.
>   * PM is locked once for each descriptor that is currently

[ ... ]

> @@ -204,6 +217,7 @@ static int hidma_chan_init(struct hidma_dev *dmadev, u32 dma_sig)
>  	mchan->dmadev = dmadev;
>  	mchan->chan.device = ddev;
>  	dma_cookie_init(&mchan->chan);
> +	dmaengine_init_bh(&mchan->chan, hidma_chan_bh);

[Severity: High]
Does this leave the BH work unsynchronized during channel release?

If a client calls dmaengine_terminate_sync() and dma_release_channel(), the
driver currently lacks a device_synchronize callback, so
dmaengine_terminate_sync() returns immediately.

When hidma_free_chan_resources() is called, it frees descriptors:

drivers/dma/qcom/hidma.c:hidma_free_chan_resources() {
    ...
    hidma_terminate_channel(dmach);
    ...
    kfree(mdesc);
    ...
}

Since dmaengine_kill_bh() isn't called here, can an unflushed BH execute
hidma_process_completed() later and cause a use-after-free on these
descriptors and the client callbacks?

>  
>  	INIT_LIST_HEAD(&mchan->free);
>  	INIT_LIST_HEAD(&mchan->prepared);

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