[PATCH] usb: host: fhci: drain completion tasklet during removal

[PATCH] usb: host: fhci: drain completion tasklet during removal

[Jiale Yao]

The transfer confirmation path can queue process_done_task immediately
before the timer IRQ is freed.  free_irq() prevents new hard IRQ
handlers, but it does not drain a tasklet that was already queued.  The
tasklet can therefore access the FHCI schedule, timer, and HCD after the
remove path releases them.

Kill the tasklet after freeing the timer IRQ and before releasing the
resources it accesses.

Fixes: 236dd4d18f29 ("USB: Driver for Freescale QUICC Engine USB Host Controller")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/usb/host/fhci-hcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
index d572f0c1dffa..c3fd7f77a71f 100644
--- a/drivers/usb/host/fhci-hcd.c
+++ b/drivers/usb/host/fhci-hcd.c
@@ -757,6 +757,7 @@ static void fhci_remove(struct device *dev)
 
 	usb_remove_hcd(hcd);
 	free_irq(fhci->timer->irq, hcd);
+	tasklet_kill(fhci->process_done_task);
 	gtm_put_timer16(fhci->timer);
 	cpm_muram_free(cpm_muram_offset(fhci->pram));
 	for (j = 0; j < NUM_PINS; j++)
-- 
2.34.1

Re: [PATCH] usb: host: fhci: drain completion tasklet during removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: host: fhci: tasklet_kill() in fhci_remove() runs after fhci_stop()
--

commit 8d5d7e60a2f21330916d798e53dce45b931fffe0
Author: Jiale Yao <yaojiale02@163.com>

usb: host: fhci: drain completion tasklet during removal

This commit adds a tasklet_kill() call on fhci->process_done_task in
fhci_remove() after freeing the timer IRQ to drain queued completion tasklet
work during device removal.

> diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
> index d572f0c1dffaf..c3fd7f77a71ff 100644
> --- a/drivers/usb/host/fhci-hcd.c
> +++ b/drivers/usb/host/fhci-hcd.c
> @@ -757,6 +757,7 @@ static void fhci_remove(struct device *dev)
>  
>  	usb_remove_hcd(hcd);
>  	free_irq(fhci->timer->irq, hcd);
> +	tasklet_kill(fhci->process_done_task);

[Severity: High]
Is calling tasklet_kill() here in fhci_remove() too late to prevent the
tasklet from accessing freed resources?

Before fhci_remove() reaches tasklet_kill(fhci->process_done_task),
usb_remove_hcd() invokes usb_stop_hcd() -> fhci_stop():

drivers/usb/host/fhci-hcd.c:fhci_stop() {
	struct fhci_hcd *fhci = hcd_to_fhci(hcd);

	fhci_usb_disable_interrupt(fhci->usb_lld);
	fhci_usb_disable(fhci);

	fhci_usb_free(fhci->usb_lld);
	fhci->usb_lld = NULL;
	fhci_mem_free(fhci);
}

Inside fhci_stop(), fhci_usb_disable_interrupt() masks interrupts via
disable_irq_nosync() without waiting for in-flight handlers, and
fhci_usb_disable() calls fhci_port_disable() ->
fhci_flush_all_transmissions() -> fhci_transaction_confirm() ->
fhci_move_td_from_ed_to_done_list() -> fhci_transfer_confirm_callback(),
which schedules fhci->process_done_task.

Immediately after that, fhci_mem_free() frees fhci->empty_eds,
fhci->empty_tds, and fhci->hc_list, setting fhci->hc_list to NULL:

drivers/usb/host/fhci-hcd.c:fhci_mem_free() {
	...
	kfree(fhci->hc_list);
	fhci->hc_list = NULL;
}

Then usb_remove_hcd() calls free_irq(hcd->irq, hcd) and fhci_remove() calls
free_irq(fhci->timer->irq, hcd) before reaching tasklet_kill().

Since softirqs remain enabled and tasklet_kill() waits for an already
scheduled tasklet to finish executing its callback, process_done_list() can
run during or after fhci_stop():

drivers/usb/host/fhci-sched.c:process_done_list() {
	...
	disable_irq(fhci->timer->irq);
	disable_irq(fhci_to_hcd(fhci)->irq);
	spin_lock(&fhci->lock);

	td = fhci_remove_td_from_done_list(fhci->hc_list);
	...
}

drivers/usb/host/fhci-q.c:fhci_remove_td_from_done_list() {
	struct td *td;

	if (!list_empty(&p_list->done_list)) {
	...
}

Wouldn't this cause process_done_list() to call disable_irq() and
enable_irq() on already freed IRQs and dereference a NULL or freed
fhci->hc_list inside fhci_remove_td_from_done_list() (as well as leaving the
usb_add_hcd() error path in of_fhci_probe() that calls usb_stop_hcd() ->
fhci_stop() without draining the tasklet)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028045705.114039-1-yaojiale02@163.com?part=1