Re: [PATCH] usb: gadget: dummy_hcd: execute hrtimer callback in softirq context

[Marcello Sylvester Bauer]

Hi Andrey,

On Mon, 2024-07-29 at 04:23 +0200, andrey.konovalov@xxxxxxxxx wrote:
> From: Andrey Konovalov <andreyknvl@xxxxxxxxx>
> 
> Commit a7f3813e589f ("usb: gadget: dummy_hcd: Switch to hrtimer
> transfer
> scheduler") switched dummy_hcd to use hrtimer and made the timer's
> callback be executed in the hardirq context.
> 
> With that change, __usb_hcd_giveback_urb now gets executed in the
> hardirq
> context, which causes problems for KCOV and KMSAN.
> 
> One problem is that KCOV now is unable to collect coverage from
> the USB code that gets executed from the dummy_hcd's timer callback,
> as KCOV cannot collect coverage in the hardirq context.
> 
> Another problem is that the dummy_hcd hrtimer might get triggered in
> the
> middle of a softirq with KCOV remote coverage collection enabled, and
> that
> causes a WARNING in KCOV, as reported by syzbot. (I sent a separate
> patch
> to shut down this WARNING, but that doesn't fix the other two
> issues.)
> 
> Finally, KMSAN appears to ignore tracking memory copying operations
> that happen in the hardirq context, which causes false positive
> kernel-infoleaks, as reported by syzbot.
> 
> Change the hrtimer in dummy_hcd to execute the callback in the
> softirq
> context.
> 
> Reported-by: syzbot+2388cdaeb6b10f0c13ac@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=2388cdaeb6b10f0c13ac
> Reported-by: syzbot+17ca2339e34a1d863aad@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=17ca2339e34a1d863aad
> Fixes: a7f3813e589f ("usb: gadget: dummy_hcd: Switch to hrtimer
> transfer scheduler")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxx>
> 
> ---
> 
> Marcello, would this change be acceptable for your use case?

Thanks for investigating and finding the cause of this problem. I have
already submitted an identical patch to change the hrtimer to softirq:
https://lkml.org/lkml/2024/6/26/969

However, your commit messages contain more useful information about the
problem at hand. So I'm happy to drop my patch in favor of yours.

Btw, the same problem has also been reported by the intel kernel test
robot. So we should add additional tags to mark this patch as the fix.


Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
Closes:
https://lore.kernel.org/oe-lkp/202406141323.413a90d2-lkp@xxxxxxxxx
Acked-by: Marcello Sylvester Bauer <sylv@xxxxxxx>

Thanks,
Marcello

> If we wanted to keep the hardirq hrtimer, we would need teach KCOV to
> collect coverage in the hardirq context (or disable it, which would
> be
> unfortunate) and also fix whatever is wrong with KMSAN, but all that
> requires some work.
> ---
>  drivers/usb/gadget/udc/dummy_hcd.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c
> b/drivers/usb/gadget/udc/dummy_hcd.c
> index f37b0d8386c1a..ff7bee78bcc49 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -1304,7 +1304,8 @@ static int dummy_urb_enqueue(
>  
>   /* kick the scheduler, it'll do the rest */
>   if (!hrtimer_active(&dum_hcd->timer))
> - hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS),
> HRTIMER_MODE_REL);
> + hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS),
> + HRTIMER_MODE_REL_SOFT);
>  
>   done:
>   spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
> @@ -1325,7 +1326,7 @@ static int dummy_urb_dequeue(struct usb_hcd
> *hcd, struct urb *urb, int status)
>   rc = usb_hcd_check_unlink_urb(hcd, urb, status);
>   if (!rc && dum_hcd->rh_state != DUMMY_RH_RUNNING &&
>   !list_empty(&dum_hcd->urbp_list))
> - hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL);
> + hrtimer_start(&dum_hcd->timer, ns_to_ktime(0),
> HRTIMER_MODE_REL_SOFT);
>  
>   spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
>   return rc;
> @@ -1995,7 +1996,8 @@ static enum hrtimer_restart dummy_timer(struct
> hrtimer *t)
>   dum_hcd->udev = NULL;
>   } else if (dum_hcd->rh_state == DUMMY_RH_RUNNING) {
>   /* want a 1 msec delay here */
> - hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS),
> HRTIMER_MODE_REL);
> + hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS),
> + HRTIMER_MODE_REL_SOFT);
>   }
>  
>   spin_unlock_irqrestore(&dum->lock, flags);
> @@ -2389,7 +2391,7 @@ static int dummy_bus_resume(struct usb_hcd
> *hcd)
>   dum_hcd->rh_state = DUMMY_RH_RUNNING;
>   set_link_state(dum_hcd);
>   if (!list_empty(&dum_hcd->urbp_list))
> - hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL);
> + hrtimer_start(&dum_hcd->timer, ns_to_ktime(0),
> HRTIMER_MODE_REL_SOFT);
>   hcd->state = HC_STATE_RUNNING;
>   }
>   spin_unlock_irq(&dum_hcd->dum->lock);
> @@ -2467,7 +2469,7 @@ static DEVICE_ATTR_RO(urbs);
>  
>  static int dummy_start_ss(struct dummy_hcd *dum_hcd)
>  {
> - hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC,
> HRTIMER_MODE_REL_SOFT);
>   dum_hcd->timer.function = dummy_timer;
>   dum_hcd->rh_state = DUMMY_RH_RUNNING;
>   dum_hcd->stream_en_ep = 0;
> @@ -2497,7 +2499,7 @@ static int dummy_start(struct usb_hcd *hcd)
>   return dummy_start_ss(dum_hcd);
>  
>   spin_lock_init(&dum_hcd->dum->lock);
> - hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC,
> HRTIMER_MODE_REL_SOFT);
>   dum_hcd->timer.function = dummy_timer;
>   dum_hcd->rh_state = DUMMY_RH_RUNNING;
>

Attachment: signature.asc
Description: This is a digitally signed message part