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

[sashiko-bot@kernel.org]

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