[PATCH v2] usbip: vudc: Prevent transfer timer rearm during teardown

[PATCH v2] usbip: vudc: Prevent transfer timer rearm during teardown

[Myeonghun Pak]

Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
vudc_remove due to race condition") deletes the transfer timer before
vudc_remove() frees the vudc, but the receive thread keeps running
until usb_del_gadget_udc().  v_kick_timer() calls mod_timer() for
CMD_SUBMIT and CMD_UNLINK even in VUDC_TR_STOPPED, so a packet after
timer_delete_sync() rearms the timer and v_timer() uses the freed
vudc.

Use timer_shutdown_sync(), which ignores later arming.  The transfer
state is freed with the timer.  Annotate v_start_timer() and
v_kick_timer() with __must_hold(&udc->lock).

A KASAN and DEBUG_OBJECTS x86_64 QEMU guest reproduced the free-active
timer warning and the use-after-free in v_timer().  The same harness
completed 4000 iterations with this change.

Fixes: d96209626a29 ("usbip: vudc: Fix use after free bug in vudc_remove due to race condition")
Link: https://lore.kernel.org/all/20230316180940.1601515-1-zyytlz.wz@163.com/
Cc: stable@vger.kernel.org
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Changes in v2:
- Drop the lock-held comment and mark v_start_timer() and
  v_kick_timer() with __must_hold(&udc->lock), on the prototypes and
  the definitions (Greg Kroah-Hartman).

 drivers/usb/usbip/vudc.h          | 6 ++++--
 drivers/usb/usbip/vudc_transfer.c | 8 +++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/usbip/vudc.h b/drivers/usb/usbip/vudc.h
index 5ef0e7d9b23a..c129f4c56e8a 100644
--- a/drivers/usb/usbip/vudc.h
+++ b/drivers/usb/usbip/vudc.h
@@ -157,8 +157,10 @@ int v_rx_loop(void *data);
 /* vudc_transfer.c */
 
 void v_init_timer(struct vudc *udc);
-void v_start_timer(struct vudc *udc);
-void v_kick_timer(struct vudc *udc, unsigned long time);
+void v_start_timer(struct vudc *udc)
+	__must_hold(&udc->lock);
+void v_kick_timer(struct vudc *udc, unsigned long time)
+	__must_hold(&udc->lock);
 void v_stop_timer(struct vudc *udc);
 
 /* vudc_dev.c */
diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
index d4ce85c4c6a2..ba625622975e 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -441,8 +441,6 @@ static void v_timer(struct timer_list *t)
 	spin_unlock_irqrestore(&udc->lock, flags);
 }
 
-/* All timer functions are run with udc->lock held */
-
 void v_init_timer(struct vudc *udc)
 {
 	struct transfer_timer *t = &udc->tr_timer;
@@ -452,6 +450,7 @@ void v_init_timer(struct vudc *udc)
 }
 
 void v_start_timer(struct vudc *udc)
+	__must_hold(&udc->lock)
 {
 	struct transfer_timer *t = &udc->tr_timer;
 
@@ -470,6 +469,7 @@ void v_start_timer(struct vudc *udc)
 }
 
 void v_kick_timer(struct vudc *udc, unsigned long time)
+	__must_hold(&udc->lock)
 {
 	struct transfer_timer *t = &udc->tr_timer;
 
@@ -490,8 +490,6 @@ void v_stop_timer(struct vudc *udc)
 {
 	struct transfer_timer *t = &udc->tr_timer;
 
-	/* Delete the timer synchronously before teardown frees udc. */
 	dev_dbg(&udc->pdev->dev, "timer stop");
-	timer_delete_sync(&t->timer);
-	t->state = VUDC_TR_STOPPED;
+	timer_shutdown_sync(&t->timer);
 }

base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
-- 
2.53.0

Re: [PATCH v2] usbip: vudc: Prevent transfer timer rearm during teardown

[Shuah Khan]

On 9/22/26 20:20, Myeonghun Pak wrote:
> Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition") deletes the transfer timer before
> vudc_remove() frees the vudc, but the receive thread keeps running
> until usb_del_gadget_udc().  v_kick_timer() calls mod_timer() for
> CMD_SUBMIT and CMD_UNLINK even in VUDC_TR_STOPPED, so a packet after
> timer_delete_sync() rearms the timer and v_timer() uses the freed
> vudc.
> 
> Use timer_shutdown_sync(), which ignores later arming.  The transfer
> state is freed with the timer.  Annotate v_start_timer() and
> v_kick_timer() with __must_hold(&udc->lock).

Can you explain why __must_hold(&udc->lock) necessary?

> 
> A KASAN and DEBUG_OBJECTS x86_64 QEMU guest reproduced the free-active
> timer warning and the use-after-free in v_timer().  The same harness
> completed 4000 iterations with this change.__must_hold(&udc->lock)

What does the harness do? You mentioned it in a response, but I would
like to see this in the change log.

> 
> Fixes: d96209626a29 ("usbip: vudc: Fix use after free bug in vudc_remove due to race condition")
> Link: https://lore.kernel.org/all/20230316180940.1601515-1-zyytlz.wz@163.com/
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
> Changes in v2:
> - Drop the lock-held comment and mark v_start_timer() and
>    v_kick_timer() with __must_hold(&udc->lock), on the prototypes and
>    the definitions (Greg Kroah-Hartman).
> 
>   drivers/usb/usbip/vudc.h          | 6 ++++--
>   drivers/usb/usbip/vudc_transfer.c | 8 +++-----
>   2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vudc.h b/drivers/usb/usbip/vudc.h
> index 5ef0e7d9b23a..c129f4c56e8a 100644
> --- a/drivers/usb/usbip/vudc.h
> +++ b/drivers/usb/usbip/vudc.h
> @@ -157,8 +157,10 @@ int v_rx_loop(void *data);
>   /* vudc_transfer.c */
>   
>   void v_init_timer(struct vudc *udc);
> -void v_start_timer(struct vudc *udc);
> -void v_kick_timer(struct vudc *udc, unsigned long time);
> +void v_start_timer(struct vudc *udc)
> +	__must_hold(&udc->lock);
> +void v_kick_timer(struct vudc *udc, unsigned long time)
> +	__must_hold(&udc->lock);
>   void v_stop_timer(struct vudc *udc);
>   
>   /* vudc_dev.c */
> diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
> index d4ce85c4c6a2..ba625622975e 100644
> --- a/drivers/usb/usbip/vudc_transfer.c
> +++ b/drivers/usb/usbip/vudc_transfer.c
> @@ -441,8 +441,6 @@ static void v_timer(struct timer_list *t)
>   	spin_unlock_irqrestore(&udc->lock, flags);
>   }
>   
> -/* All timer functions are run with udc->lock held */
> -
>   void v_init_timer(struct vudc *udc)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
> @@ -452,6 +450,7 @@ void v_init_timer(struct vudc *udc)
>   }
>   
>   void v_start_timer(struct vudc *udc)
> +	__must_hold(&udc->lock)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
>   
> @@ -470,6 +469,7 @@ void v_start_timer(struct vudc *udc)
>   }
>   
>   void v_kick_timer(struct vudc *udc, unsigned long time)
> +	__must_hold(&udc->lock)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
>   
> @@ -490,8 +490,6 @@ void v_stop_timer(struct vudc *udc)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
>   
> -	/* Delete the timer synchronously before teardown frees udc. */
>   	dev_dbg(&udc->pdev->dev, "timer stop");
> -	timer_delete_sync(&t->timer);
> -	t->state = VUDC_TR_STOPPED;
> +	timer_shutdown_sync(&t->timer);
>   }
> 
> base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5

Have you tested this with real devices bound to a host and exported
to a client?

thanks,
-- Shuah

Re: [PATCH v2] usbip: vudc: Prevent transfer timer rearm during teardown

[Myeonghun Pak]

Hi Shuah,

__must_hold records the existing locking requirement for
v_start_timer() and v_kick_timer(). The old comment was inaccurate
because v_stop_timer() runs without the lock. timer_shutdown_sync() is
the actual fix.

The QEMU harness sends CMD_SUBMIT packets while repeatedly unbinding
and rebinding vudc. It reproduced the bug before the fix and ran 4,000
iterations without a report afterward. I’ll add this to the v3 commit
message.

I have not tested with physical devices and a remote client.

Thanks,
Myeonghun

2026년 9월 23일 (수) 오전 5:11, Shuah Khan <skhan@linuxfoundation.org>님이 작성:
>
> On 9/22/26 20:20, Myeonghun Pak wrote:
> > Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> > vudc_remove due to race condition") deletes the transfer timer before
> > vudc_remove() frees the vudc, but the receive thread keeps running
> > until usb_del_gadget_udc().  v_kick_timer() calls mod_timer() for
> > CMD_SUBMIT and CMD_UNLINK even in VUDC_TR_STOPPED, so a packet after
> > timer_delete_sync() rearms the timer and v_timer() uses the freed
> > vudc.
> >
> > Use timer_shutdown_sync(), which ignores later arming.  The transfer
> > state is freed with the timer.  Annotate v_start_timer() and
> > v_kick_timer() with __must_hold(&udc->lock).
>
> Can you explain why __must_hold(&udc->lock) necessary?
>
> >
> > A KASAN and DEBUG_OBJECTS x86_64 QEMU guest reproduced the free-active
> > timer warning and the use-after-free in v_timer().  The same harness
> > completed 4000 iterations with this change.__must_hold(&udc->lock)
>
> What does the harness do? You mentioned it in a response, but I would
> like to see this in the change log.
>
> >
> > Fixes: d96209626a29 ("usbip: vudc: Fix use after free bug in vudc_remove due to race condition")
> > Link: https://lore.kernel.org/all/20230316180940.1601515-1-zyytlz.wz@163.com/
> > Cc: stable@vger.kernel.org
> > Assisted-by: LLM
> > Co-developed-by: Ijae Kim <ae878000@gmail.com>
> > Signed-off-by: Ijae Kim <ae878000@gmail.com>
> > Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> > ---
> > Changes in v2:
> > - Drop the lock-held comment and mark v_start_timer() and
> >    v_kick_timer() with __must_hold(&udc->lock), on the prototypes and
> >    the definitions (Greg Kroah-Hartman).
> >
> >   drivers/usb/usbip/vudc.h          | 6 ++++--
> >   drivers/usb/usbip/vudc_transfer.c | 8 +++-----
> >   2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/usbip/vudc.h b/drivers/usb/usbip/vudc.h
> > index 5ef0e7d9b23a..c129f4c56e8a 100644
> > --- a/drivers/usb/usbip/vudc.h
> > +++ b/drivers/usb/usbip/vudc.h
> > @@ -157,8 +157,10 @@ int v_rx_loop(void *data);
> >   /* vudc_transfer.c */
> >
> >   void v_init_timer(struct vudc *udc);
> > -void v_start_timer(struct vudc *udc);
> > -void v_kick_timer(struct vudc *udc, unsigned long time);
> > +void v_start_timer(struct vudc *udc)
> > +     __must_hold(&udc->lock);
> > +void v_kick_timer(struct vudc *udc, unsigned long time)
> > +     __must_hold(&udc->lock);
> >   void v_stop_timer(struct vudc *udc);
> >
> >   /* vudc_dev.c */
> > diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
> > index d4ce85c4c6a2..ba625622975e 100644
> > --- a/drivers/usb/usbip/vudc_transfer.c
> > +++ b/drivers/usb/usbip/vudc_transfer.c
> > @@ -441,8 +441,6 @@ static void v_timer(struct timer_list *t)
> >       spin_unlock_irqrestore(&udc->lock, flags);
> >   }
> >
> > -/* All timer functions are run with udc->lock held */
> > -
> >   void v_init_timer(struct vudc *udc)
> >   {
> >       struct transfer_timer *t = &udc->tr_timer;
> > @@ -452,6 +450,7 @@ void v_init_timer(struct vudc *udc)
> >   }
> >
> >   void v_start_timer(struct vudc *udc)
> > +     __must_hold(&udc->lock)
> >   {
> >       struct transfer_timer *t = &udc->tr_timer;
> >
> > @@ -470,6 +469,7 @@ void v_start_timer(struct vudc *udc)
> >   }
> >
> >   void v_kick_timer(struct vudc *udc, unsigned long time)
> > +     __must_hold(&udc->lock)
> >   {
> >       struct transfer_timer *t = &udc->tr_timer;
> >
> > @@ -490,8 +490,6 @@ void v_stop_timer(struct vudc *udc)
> >   {
> >       struct transfer_timer *t = &udc->tr_timer;
> >
> > -     /* Delete the timer synchronously before teardown frees udc. */
> >       dev_dbg(&udc->pdev->dev, "timer stop");
> > -     timer_delete_sync(&t->timer);
> > -     t->state = VUDC_TR_STOPPED;
> > +     timer_shutdown_sync(&t->timer);
> >   }
> >
> > base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
>
> Have you tested this with real devices bound to a host and exported
> to a client?
>
> thanks,
> -- Shuah
>