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@xxxxxxxxxxxxxxxxxxx>님이 작성:
>
> 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@xxxxxxx/
> > Cc: stable@xxxxxxxxxxxxxxx
> > Assisted-by: LLM
> > Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> > ---
> > 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
>