[PATCH] mmc: vub300: never tear the host down from the inactivity timer

[Yogesh Gaur]

vub300_probe() takes a second kref reference on behalf of the inactivity
timer and arms it:

	kref_init(&vub300->kref);
	...
	kref_get(&vub300->kref);
	timer_setup(&vub300->inactivity_timer,
		    vub300_inactivity_timer_expired, 0);
	vub300->inactivity_timer.expires = jiffies + HZ;
	add_timer(&vub300->inactivity_timer);

and expects the timer to release that reference from its own expiry
function, once it observes that vub300->interface has been cleared:

	if (!vub300->interface) {
		kref_put(&vub300->kref, vub300_delete);
	} else if (vub300->cmd) {

That is wrong in both directions, because the expiry function runs in
softirq context.

If the timer happens to hold the last reference, the kref_put() runs
vub300_delete() -> mmc_free_host() -> cancel_delayed_work_sync(), which
sleeps. The ->probe() error path arranges exactly that: it clears
->interface and drops only its own reference, leaving the timer armed
and owning the last one.

	BUG: sleeping function called from invalid context at kernel/workqueue.c:4487
	in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 191, name: kworker/0:2
	Call Trace:
	 <IRQ>
	 __might_resched.cold+0x1ec/0x232 kernel/sched/core.c:9197
	 __cancel_work_sync kernel/workqueue.c:4487 [inline]
	 cancel_delayed_work_sync+0xb8/0xf0 kernel/workqueue.c:4568
	 mmc_free_host+0x19/0x30 drivers/mmc/core/host.c:700
	 vub300_delete drivers/mmc/host/vub300.c:379 [inline]
	 kref_put include/linux/kref.h:65 [inline]
	 vub300_inactivity_timer_expired drivers/mmc/host/vub300.c:747
	 call_timer_fn+0x11f/0x610 kernel/time/timer.c:1745
	 </IRQ>

If instead the timer drops its reference and stops rearming, one of the
mod_timer() calls in the command and dead work threads can arm it again
-- those do not take a reference of their own. The kref_put() in
vub300_disconnect() then drops what is now the last reference and
vub300_delete() frees the host together with the still armed timer
embedded in it.

	ODEBUG: free active (active state 0) object: ffff88803cd81420 object type: timer_list hint: vub300_inactivity_timer_expired+0x0/0x3f0
	WARNING: lib/debugobjects.c:632 at debug_print_object+0xec/0x230 lib/debugobjects.c:629
	Call Trace:
	 <TASK>
	 debug_check_no_obj_freed+0x2e3/0x450 lib/debugobjects.c:1201
	 kfree+0x13e/0x6d0 mm/slub.c:6792
	 kobject_put+0x222/0x550 lib/kobject.c:737
	 vub300_delete drivers/mmc/host/vub300.c:379 [inline]
	 vub300_disconnect+0x280/0x2f0 drivers/mmc/host/vub300.c:2388
	 usb_unbind_interface+0x295/0x9f0 drivers/usb/core/driver.c:458
	 usb_disconnect+0x32d/0x990 drivers/usb/core/hub.c:2345
	 hub_event+0x1bb7/0x4cf0 drivers/usb/core/hub.c:5961
	 </TASK>

Take the timer out of the reference counting altogether. The expiry
function no longer inspects ->interface and no longer drops a reference;
it just queues the dead work and rearms as before. Both teardown paths
now call the new vub300_stop_inactivity_timer(), which runs
timer_shutdown_sync() from process context and then drops the reference
that ->probe() took for the timer. timer_shutdown_sync() additionally
turns any later mod_timer() into a no-op, so the work threads can no
longer resurrect the timer, and vub300_delete() only ever runs somewhere
it is allowed to sleep.

vub300_deadwork_thread() has the same "->interface is NULL, so put the
reference" shape, but it runs in process context and releases the
reference that vub300_queue_dead_work() took for it, so it is left as
is.

Fixes: 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
Reported-by: syzbot+f4a0159ce6802a0a4774@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=f4a0159ce6802a0a4774
Reported-by: syzbot+1ee4f3b9228e35f14677@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=1ee4f3b9228e35f14677
Signed-off-by: Yogesh Gaur <yogeshgaur.83@xxxxxxxxx>
---
 drivers/mmc/host/vub300.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
index 2dae474dcd06..b8e72a054439 100644
--- a/drivers/mmc/host/vub300.c
+++ b/drivers/mmc/host/vub300.c
@@ -743,14 +743,22 @@ static void vub300_inactivity_timer_expired(struct timer_list *t)
 {				/* softirq */
 	struct vub300_mmc_host *vub300 = timer_container_of(vub300, t,
 							    inactivity_timer);
-	if (!vub300->interface) {
-		kref_put(&vub300->kref, vub300_delete);
-	} else if (vub300->cmd) {
-		mod_timer(&vub300->inactivity_timer, jiffies + HZ);
-	} else {
+	if (!vub300->cmd)
 		vub300_queue_dead_work(vub300);
-		mod_timer(&vub300->inactivity_timer, jiffies + HZ);
-	}
+
+	mod_timer(&vub300->inactivity_timer, jiffies + HZ);
+}
+
+/*
+ * Stop the inactivity timer and drop the reference that ->probe() took on its
+ * behalf.  Must be called from process context: once timer_shutdown_sync() has
+ * returned the timer can neither run nor be rearmed by the mod_timer() calls
+ * made from the command and dead work threads.
+ */
+static void vub300_stop_inactivity_timer(struct vub300_mmc_host *vub300)
+{
+	timer_shutdown_sync(&vub300->inactivity_timer);
+	kref_put(&vub300->kref, vub300_delete);
 }
 
 static int vub300_response_error(u8 error_code)
@@ -2350,6 +2358,7 @@ static int vub300_probe(struct usb_interface *interface,
 
 err_stop_io:
 	vub300->interface = NULL;
+	vub300_stop_inactivity_timer(vub300);
 	kref_put(&vub300->kref, vub300_delete);
 
 	return retval;
@@ -2384,6 +2393,7 @@ static void vub300_disconnect(struct usb_interface *interface)
 			usb_set_intfdata(interface, NULL);
 			/* prevent more I/O from starting */
 			vub300->interface = NULL;
+			vub300_stop_inactivity_timer(vub300);
 			mmc_remove_host(mmc);
 			kref_put(&vub300->kref, vub300_delete);
 			pr_info("USB vub300 remote SDIO host controller[%d]"
-- 
2.55.0.windows.5