twl4030_charger: worker not cancelled on remove?

twl4030_charger: worker not cancelled on remove?

[Maoyi Xie]

Hi Sebastian,

twl4030_charger looks like it has the worker race on remove that bq25890
fixed (commit 7e6fb67808ab). I would appreciate it if you could take a look.

The driver data is allocated with devm in probe:

	bci = devm_kzalloc(&pdev->dev, sizeof(*bci), GFP_KERNEL);

It has two workers that both dereference bci:

	INIT_WORK(&bci->work, twl4030_bci_usb_work);
	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);

twl4030_bci_remove() disables charging and masks interrupts. It cancels
neither worker. A worker still pending at remove runs after devm frees bci,
so it touches freed memory.

bq25890 fixed the same shape by unregistering the USB notifier, then
cancel_work_sync() on remove. twl4030 is different. Its notifier comes from
devm_usb_get_phy_by_node(), so devm unregisters it after remove() returns.
A plain cancel in remove() could then still race a reschedule. So a single
cancel in remove() is not enough.

current_worker does not depend on the notifier. It reschedules itself, so it
needs cancel_delayed_work_sync regardless.

Does this look like a real bug? If it does, I am happy to send a patch. I am
not sure of the best shape here. One option is to move the phy off devm and
mirror bq25890. Another is to coordinate the cancel differently. What would
you prefer?

I do not have the hardware. The use-after-free is from a KASAN harness. It
runs the worker after the bci free.

Thanks,
Maoyi

Re: twl4030_charger: worker not cancelled on remove?

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]

Hello Maoyi,

On Fri, Jul 03, 2026 at 01:21:28AM +0800, Maoyi Xie wrote:
> twl4030_charger looks like it has the worker race on remove that bq25890
> fixed (commit 7e6fb67808ab). I would appreciate it if you could take a look.
> 
> The driver data is allocated with devm in probe:
> 
> 	bci = devm_kzalloc(&pdev->dev, sizeof(*bci), GFP_KERNEL);
> 
> It has two workers that both dereference bci:
> 
> 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
> 	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
> 
> twl4030_bci_remove() disables charging and masks interrupts. It cancels
> neither worker. A worker still pending at remove runs after devm frees bci,
> so it touches freed memory.
> 
> bq25890 fixed the same shape by unregistering the USB notifier, then
> cancel_work_sync() on remove. twl4030 is different. Its notifier comes from
> devm_usb_get_phy_by_node(), so devm unregisters it after remove() returns.
> A plain cancel in remove() could then still race a reschedule. So a single
> cancel in remove() is not enough.
> 
> current_worker does not depend on the notifier. It reschedules itself, so it
> needs cancel_delayed_work_sync regardless.
> 
> Does this look like a real bug? If it does, I am happy to send a patch. I am
> not sure of the best shape here. One option is to move the phy off devm and
> mirror bq25890. Another is to coordinate the cancel differently. What would
> you prefer?
> 
> I do not have the hardware. The use-after-free is from a KASAN harness. It
> runs the worker after the bci free.

The proper fix is to use devm_delayed_work_autocancel() instead
of INIT_DELAYED_WORK() and devm_work_autocancel() instead of
INIT_WORK(), so that everything happens at the right time.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]