[PATCH v2] mmc: vub300: fix sleeping function called from invalid context

Ömer Mete Kaya <[email protected]>
Newsgroups org.kernel.vger.linux-mmc,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
syzbot reports:

  BUG: sleeping function called from invalid context at kernel/workqueue.c:4487
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/1
  ...
   <IRQ>
   __might_resched
   __cancel_work_sync
   mmc_free_host+0x19/0x30 [drivers/mmc/core/host.c:700]
   call_timer_fn+0x192/0x5e0 [kernel/time/timer.c:1748]
   run_timer_softirq
   ...

vub300_inactivity_timer_expired() runs in softirq (timer) context.
When the USB interface had already gone away (->interface == NULL,
cleared by vub300_disconnect() or the probe() error path), the timer
handler dropped the object's last kref via
kref_put(&vub300->kref, vub300_delete). If that was the last
reference, vub300_delete() ran from softirq context and called
mmc_free_host(), which calls cancel_delayed_work_sync() - a sleeping
function, illegal from softirq/timer context.

Root cause: inactivity_timer is armed in probe() and continuously
re-armed via mod_timer(), but - unlike sg_transfer_timer, which is
explicitly deleted after each use - it is never stopped when the
device is torn down, so it can still fire after ->interface has
been cleared.

Fix this by decoupling inactivity_timer from the object's kref
entirely: drop the kref_get() taken on its behalf in probe(); make
vub300_inactivity_timer_expired() a no-op when ->interface is NULL
instead of dropping a reference; and in both vub300_disconnect() and
the probe() err_stop_io path, call
timer_delete_sync(&vub300->inactivity_timer) right after clearing
->interface and before the final kref_put(). Since ->interface is
already NULL at that point, any concurrently running timer instance
takes the no-op branch, so timer_delete_sync() is guaranteed to
return with the timer stopped for good - removing any race with the
final kref_put()/vub300_delete()/mmc_free_host(). Before this
patch, a successful probe() left two references on the kref (one
from kref_init(), one from the timer's kref_get()); after it, only
the initial kref_init() reference remains, matching the single
kref_put() in vub300_disconnect() and err_stop_io.

While auditing the driver for the same class of bug, also switch
sg_transfer_timer's two timer_delete() call sites (in
__command_read_data() and __command_write_data()) to
timer_delete_sync(), since usb_sg_wait() returning does not
guarantee a concurrently running vub300_sg_timed_out() has finished.
__command_write_data() additionally only deleted the timer on the
success path, leaking an armed timer on the cmd->error path; the
(now synchronous) delete is moved before that check so it always
runs.

Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=0e06aa1bdc6495bac24b
Fixes: 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Ömer Mete Kaya <[email protected]>
---
v2: I used an LLM to help structure and polish the English commit
message. I did not know
this required an Assisted-by tag until Johan pointed it out - added
now.

 drivers/mmc/host/vub300.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
index 2dae474dcd06..def8c7a29e91 100644
--- a/drivers/mmc/host/vub300.c
+++ b/drivers/mmc/host/vub300.c
@@ -744,7 +744,7 @@ static void vub300_inactivity_timer_expired(struct timer_list *t)
 	struct vub300_mmc_host *vub300 = timer_container_of(vub300, t,
 							    inactivity_timer);
 	if (!vub300->interface) {
-		kref_put(&vub300->kref, vub300_delete);
+		/* Intentional no-op; see commit message. */
 	} else if (vub300->cmd) {
 		mod_timer(&vub300->inactivity_timer, jiffies + HZ);
 	} else {
@@ -1453,7 +1453,8 @@ static int __command_read_data(struct vub300_mmc_host *vub300,
 						  (linear_length / 16384));
 			add_timer(&vub300->sg_transfer_timer);
 			usb_sg_wait(&vub300->sg_request);
-			timer_delete(&vub300->sg_transfer_timer);
+			/* Sync variant needed; see commit message. */
+			timer_delete_sync(&vub300->sg_transfer_timer);
 			if (vub300->sg_request.status < 0) {
 				cmd->error = vub300->sg_request.status;
 				data->bytes_xfered = 0;
@@ -1570,10 +1571,11 @@ static int __command_write_data(struct vub300_mmc_host *vub300,
 							   linear_length / 16384);
 			add_timer(&vub300->sg_transfer_timer);
 			usb_sg_wait(&vub300->sg_request);
+			/* Unconditional + sync; see commit message. */
+			timer_delete_sync(&vub300->sg_transfer_timer);
 			if (cmd->error) {
 				data->bytes_xfered = 0;
 			} else {
-				timer_delete(&vub300->sg_transfer_timer);
 				if (vub300->sg_request.status < 0) {
 					cmd->error = vub300->sg_request.status;
 					data->bytes_xfered = 0;
@@ -2327,7 +2329,7 @@ static int vub300_probe(struct usb_interface *interface,
 	INIT_WORK(&vub300->deadwork, vub300_deadwork_thread);
 	kref_init(&vub300->kref);
 	timer_setup(&vub300->sg_transfer_timer, vub300_sg_timed_out, 0);
-	kref_get(&vub300->kref);
+	/* No kref for inactivity_timer; see commit message. */
 	timer_setup(&vub300->inactivity_timer,
 		    vub300_inactivity_timer_expired, 0);
 	vub300->inactivity_timer.expires = jiffies + HZ;
@@ -2350,6 +2352,8 @@ static int vub300_probe(struct usb_interface *interface,
 
 err_stop_io:
 	vub300->interface = NULL;
+	/* Must precede kref_put(); see commit message. */
+	timer_delete_sync(&vub300->inactivity_timer);
 	kref_put(&vub300->kref, vub300_delete);
 
 	return retval;
@@ -2384,6 +2388,8 @@ static void vub300_disconnect(struct usb_interface *interface)
 			usb_set_intfdata(interface, NULL);
 			/* prevent more I/O from starting */
 			vub300->interface = NULL;
+			/* Must precede kref_put(); see commit message. */
+			timer_delete_sync(&vub300->inactivity_timer);
 			mmc_remove_host(mmc);
 			kref_put(&vub300->kref, vub300_delete);
 			pr_info("USB vub300 remote SDIO host controller[%d]"
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.