All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mmc: vub300: fix sleeping function called from invalid context
@ 2026-08-08 23:09 Ömer Mete Kaya
  2026-08-17  8:15 ` Johan Hovold
  0 siblings, 1 reply; 6+ messages in thread
From: Ömer Mete Kaya @ 2026-08-08 23:09 UTC (permalink / raw)
  To: linux-mmc
  Cc: Ömer Mete Kaya, Ulf Hansson, Johan Hovold, Chris Ball,
	Tony Olech, linux-kernel, syzbot+0e06aa1bdc6495bac24b

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: syzbot+0e06aa1bdc6495bac24b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0e06aa1bdc6495bac24b
Fixes: 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] mmc: vub300: fix sleeping function called from invalid context
  2026-08-08 23:09 [PATCH] mmc: vub300: fix sleeping function called from invalid context Ömer Mete Kaya
@ 2026-08-17  8:15 ` Johan Hovold
  2026-08-18 14:36   ` [PATCH v2] " Ömer Mete Kaya
  0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2026-08-17  8:15 UTC (permalink / raw)
  To: Ömer Mete Kaya
  Cc: linux-mmc, Ulf Hansson, Chris Ball, Tony Olech, linux-kernel,
	syzbot+0e06aa1bdc6495bac24b

On Sun, Aug 09, 2026 at 02:09:50AM +0300, Ömer Mete Kaya wrote:
> 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: syzbot+0e06aa1bdc6495bac24b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0e06aa1bdc6495bac24b
> Fixes: 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
> Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>

This is clearly LLM generated, so why didn't you add an Assisted-by tag
as required?

Johan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] mmc: vub300: fix sleeping function called from invalid context
  2026-08-17  8:15 ` Johan Hovold
@ 2026-08-18 14:36   ` Ömer Mete Kaya
  2026-08-18 15:08     ` Johan Hovold
  0 siblings, 1 reply; 6+ messages in thread
From: Ömer Mete Kaya @ 2026-08-18 14:36 UTC (permalink / raw)
  To: linux-mmc
  Cc: Ömer Mete Kaya, johan, ulfh, cjb, tony.olech, linux-kernel,
	syzbot+0e06aa1bdc6495bac24b

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: syzbot+0e06aa1bdc6495bac24b@syzkaller.appspotmail.com
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 <omermetekaya0@gmail.com>
---
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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mmc: vub300: fix sleeping function called from invalid context
  2026-08-18 14:36   ` [PATCH v2] " Ömer Mete Kaya
@ 2026-08-18 15:08     ` Johan Hovold
  2026-08-18 15:52       ` Johan Hovold
  0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2026-08-18 15:08 UTC (permalink / raw)
  To: Ömer Mete Kaya
  Cc: linux-mmc, ulfh, cjb, tony.olech, linux-kernel,
	syzbot+0e06aa1bdc6495bac24b

On Tue, Aug 18, 2026 at 05:36:56PM +0300, Ömer Mete Kaya wrote:
> 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: syzbot+0e06aa1bdc6495bac24b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0e06aa1bdc6495bac24b
> Fixes: 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")

This is not the commit that introduced the issue. See my reply to [2].

> Assisted-by: Claude:claude-sonnet-4-6
> Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>
> ---
> 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.

So you didn't use an LLM to write the proposed fix itself? The commit
message and comments in the code makes it look that way.

Also note that the syzbot LLM created a similar fix the day before you
posted yours:

  [1] https://lore.kernel.org/all/49982079-95f4-4e8c-bbbc-bcb127e2f378@mail.kernel.org/	
and there are at least two further proposals:

  [2] https://lore.kernel.org/all/20260816153809.7067-1-jakovnovak30@gmail.com/
  [3] https://lore.kernel.org/all/20260818064800.3886851-1-tao1.yu@intel.com/
	
>  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. */

Comments should be self-contained and not refer to the commit message.

Johan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mmc: vub300: fix sleeping function called from invalid context
  2026-08-18 15:08     ` Johan Hovold
@ 2026-08-18 15:52       ` Johan Hovold
  2026-08-18 18:04         ` [PATCH v3] " Ömer Mete Kaya
  0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2026-08-18 15:52 UTC (permalink / raw)
  To: Ömer Mete Kaya
  Cc: linux-mmc, ulfh, cjb, tony.olech, linux-kernel,
	syzbot+0e06aa1bdc6495bac24b

On Tue, Aug 18, 2026 at 05:08:43PM +0200, Johan Hovold wrote:
> On Tue, Aug 18, 2026 at 05:36:56PM +0300, Ömer Mete Kaya wrote:
> > 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: syzbot+0e06aa1bdc6495bac24b@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=0e06aa1bdc6495bac24b
> > Fixes: 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
> 
> This is not the commit that introduced the issue. See my reply to [2].
> 
> > Assisted-by: Claude:claude-sonnet-4-6
> > Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>
> > ---
> > 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.
> 
> So you didn't use an LLM to write the proposed fix itself? The commit
> message and comments in the code makes it look that way.
> 
> Also note that the syzbot LLM created a similar fix the day before you
> posted yours:
> 
>   [1] https://lore.kernel.org/all/49982079-95f4-4e8c-bbbc-bcb127e2f378@mail.kernel.org/	

Sorry, that was supposed to say:

   [1] https://lore.kernel.org/all/397da4bd-97e2-4367-b3f4-d69e0f58dd91@mail.kernel.org/

Apparently the bot produced two different fixes for the same issue...

> and there are at least two further proposals:
> 
>   [2] https://lore.kernel.org/all/20260816153809.7067-1-jakovnovak30@gmail.com/
>   [3] https://lore.kernel.org/all/20260818064800.3886851-1-tao1.yu@intel.com/

Johan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3] mmc: vub300: fix sleeping function called from invalid context
  2026-08-18 15:52       ` Johan Hovold
@ 2026-08-18 18:04         ` Ömer Mete Kaya
  0 siblings, 0 replies; 6+ messages in thread
From: Ömer Mete Kaya @ 2026-08-18 18:04 UTC (permalink / raw)
  To: linux-mmc
  Cc: Ömer Mete Kaya, johan, ulfh, cjb, tony.olech, linux-kernel,
	syzbot+0e06aa1bdc6495bac24b

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: syzbot+0e06aa1bdc6495bac24b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0e06aa1bdc6495bac24b
Fixes: 1036f69e2513 ("mmc: core: Cancel delayed work before releasing host")
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>
---
v3: To clarify,

LLM usage clarity:
The fix logic itself (decoupling inactivity_timer from the kref, the
ordering of timer_delete_sync() before kref_put() in both teardown
paths, and the sg_transfer_timer leak fix) is produced by me, not
the LLM. LLM was used solely to clarify the English wording of the
commit message at first (English is not my native language). The
"see commit message" comment style was also a recommendation from
the LLM (argument that "in-code comments are unnecessary when
there's a detailed commit message") - I now see that this is bad
practice for the patch, so I reverted comments to the classic,
self-explanatory in-code format, first time I tried this and I
won't repeat this pattern going forward.

Proof that mmc_free_host() is not a core issue:
In response to Jakov, you said that the fix might lie in the MMC
core.
a) The doc-comment of mmc_free_host() does not specify any context
(process/softirq) requirements, as in:

/**
 *	mmc_free_host - free the host structure
 *	@host: mmc host
 *
 *	Free the host once all references to it have been dropped.
 */
void mmc_free_host(struct mmc_host *host)

b) grep -rn "mmc_free_host(" drivers/mmc/ result showed that all
callers to mmc_free_host() (including SDHI, dw_mmc, sdhci-style
drivers) call it in the process context, on the remove/probe-error
path - usually after mmc_remove_host() or on the probe's error
branches, both part of the system's normal non-atomic teardown
flow. vub300 is the only exception: it calls it from the softirq
context via inactivity_timer.

If it were the core's fault, we would expect:
- The API documentation to have made a promise that "I am safe in
  every context" (it doesn't)
- Multiple drivers to be using this pattern (calling from softirq)
  (it doesn't, only vub300)
- The line added by 1036f69e2513 to have broken a pre-existing
  guarantee (it doesn't - there was no guarantee before, it was
  just safe by chance)

Proof for the Fixes: tag is 1036f69e2513:
I noticed that the commit preceding 1036f69e2513 didn't have
cancel_delayed_work_sync(), only mmc_pwrseq_free() and
put_device(), and contained no sleeping calls. The git show
1036f69e2513 diff proves that the
cancel_delayed_work_sync(&host->detect) line was added in that
exact commit. Conclusion: VUB300's pattern, which has been safe for
13 years, has become insecure with 1036f69e2513 adding a sleeping
call to mmc_free_host() - this commit is the true origin of the
bug. My previous Fixes: tag, 88095e7b473a, was wrong.

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

diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
index 2dae474dcd06..474625ad29d8 100644
--- a/drivers/mmc/host/vub300.c
+++ b/drivers/mmc/host/vub300.c
@@ -744,7 +744,12 @@ 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);
+		/*
+		 * The interface is already gone; timer_delete_sync()
+		 * in vub300_disconnect() or the probe() error path is
+		 * guaranteed to run after this and stop the timer for
+		 * good, so no kref handling is needed here.
+		 */
 	} else if (vub300->cmd) {
 		mod_timer(&vub300->inactivity_timer, jiffies + HZ);
 	} else {
@@ -1453,7 +1458,12 @@ 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);
+			/*
+			 * Use the sync variant: usb_sg_wait() returning
+			 * does not guarantee vub300_sg_timed_out() has
+			 * finished if it fired concurrently.
+			 */
+			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 +1580,17 @@ 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);
+			/*
+			 * Always delete synchronously and before checking
+			 * cmd->error: usb_sg_wait() returning does not
+			 * guarantee vub300_sg_timed_out() has finished,
+			 * and the old success-only delete leaked an armed
+			 * timer on the error path.
+			 */
+			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 +2344,12 @@ 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);
+	/*
+	 * inactivity_timer does not hold its own kref (see the
+	 * no-op branch in vub300_inactivity_timer_expired()); it
+	 * is stopped via timer_delete_sync() in the teardown
+	 * paths instead of dropping a reference.
+	 */
 	timer_setup(&vub300->inactivity_timer,
 		    vub300_inactivity_timer_expired, 0);
 	vub300->inactivity_timer.expires = jiffies + HZ;
@@ -2350,6 +2372,14 @@ static int vub300_probe(struct usb_interface *interface,
 
 err_stop_io:
 	vub300->interface = NULL;
+	/*
+	 * Stop the timer before the final kref_put(): once
+	 * ->interface is NULL, any concurrently running timer
+	 * instance takes the no-op branch above, so this call
+	 * is guaranteed to return with the timer stopped for
+	 * good.
+	 */
+	timer_delete_sync(&vub300->inactivity_timer);
 	kref_put(&vub300->kref, vub300_delete);
 
 	return retval;
@@ -2384,6 +2414,14 @@ static void vub300_disconnect(struct usb_interface *interface)
 			usb_set_intfdata(interface, NULL);
 			/* prevent more I/O from starting */
 			vub300->interface = NULL;
+			/*
+			 * Stop the timer before the final kref_put(): once
+			 * ->interface is NULL, any concurrently running
+			 * timer instance takes the no-op branch above, so
+			 * this call is guaranteed to return with the timer
+			 * stopped for good.
+			 */
+			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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-08-18 18:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-08 23:09 [PATCH] mmc: vub300: fix sleeping function called from invalid context Ömer Mete Kaya
2026-08-17  8:15 ` Johan Hovold
2026-08-18 14:36   ` [PATCH v2] " Ömer Mete Kaya
2026-08-18 15:08     ` Johan Hovold
2026-08-18 15:52       ` Johan Hovold
2026-08-18 18:04         ` [PATCH v3] " Ömer Mete Kaya

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.