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