From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: fix crash on close isochronous endpoint's webcam
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org
Date: Mon, 10 Mar 2025 06:55:54 +0100

On Sun, Mar 09, 2025 at 10:13:47PM GMT, Kirill A. Korinsky wrote:

> On Sat, 08 Mar 2025 09:09:55 +0100,
> Marcus Glocker <marcus@nazgul.ch> wrote:
> > 
> > On Sat, Mar 08, 2025 at 08:53:27AM GMT, Kirill A. Korinsky wrote:
> > 
> > > On Sat, 08 Mar 2025 08:26:29 +0100,
> > > Marcus Glocker <marcus@nazgul.ch> wrote:
> > > > 
> > > > On Sat, Mar 08, 2025 at 12:57:22AM GMT, Kirill A. Korinsky wrote:
> > > > 
> > > > > On Fri, 07 Mar 2025 21:39:30 +0100,
> > > > > Marcus Glocker <marcus@nazgul.ch> wrote:
> > > > > >
> > > > > > On Thu, Mar 06, 2025 at 09:12:55AM GMT, Kirill A. Korinsky wrote:
> > > > > >
> > > > > > > tech@,
> > > > > > >
> > > > > > > I'd like to fix a crash in uvide when I detach my Elgato webcam when
> > > > > > > it's streamming.
> > > > > > >
> > > > > > > uvideo_vs_close is called from videoclose and by VIDIOC_STREAMOFF. When
> > > > > > > it's happened on detached webcam with isochronous endpoint, the system
> > > > > > > crashes inside usbd_set_interface.
> > > > > > >
> > > > > > > Ok?
> > > > > >
> > > > > > Usually we're using usbd_is_dying() to return from a function at the
> > > > > > very beginning.  Would it also work in your case to return from
> > > > > > uvideo_vs_close() at the very beginning instead of only wrapping it
> > > > > > around usbd_set_interface()?
> > > > > >
> > > > > 
> > > > > Well, I assume that usually driver doesn't sleep before call set interface.
> > > > > 
> > > > > I had tried to move usbd_is_dying(sc->sc_udev) to the first line of
> > > > > uvideo_vs_close, and it crashed inside usbd_set_interface().
> > > > > 
> > > > > And when I had moved it back, after sleep, it works.
> > > > 
> > > > I see.  Which indicates that at the beginning of uvideo_vs_close()
> > > > usbd_is_dying() isn't set.  Probably the usb state converts to dying
> > > > after the delay in your case.
> > > > 
> > > > I'm basically OK with that then, and it shouldn't have regression to
> > > > other devices.
> > > > 
> > > > Just one style thing;  Can you please move the comment above the 'if'
> > > > to avoid line break?
> > > 
> > > Sure, like this?
> > 
> > ok mglocker@
> >  
> > > Index: sys/dev/usb/uvideo.c
> > > ===================================================================
> > > RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> > > diff -u -p -r1.249 uvideo.c
> > > --- sys/dev/usb/uvideo.c	4 Mar 2025 22:59:01 -0000	1.249
> > > +++ sys/dev/usb/uvideo.c	8 Mar 2025 07:52:31 -0000
> > > @@ -2170,7 +2170,8 @@ uvideo_vs_close(struct uvideo_softc *sc)
> > >  		usbd_delay_ms(sc->sc_udev, 100);
> > >  
> > >  		/* switch back to default interface (turns off cam LED) */
> > > -		(void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0);
> > > +		if (!usbd_is_dying(sc->sc_udev))
> > > +			(void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0);
> > >  	}
> > >  }
> > >  
> > > 
> 
> I had continue to detach this webcam from test laptop... and I had crashede
> system one more time.
> 
> You was right, I need usbd_is_dying at begining because usbd_close_pipe on
> dead device lead to crash.
> 
> So, here diff with usbd_is_dying at the begining which protects from a crash
> by ioctl(VIDIOC_STREAMOFF) on long dead / detached device.
> 
> Ok?

ok mglocker@
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.250 uvideo.c
> --- sys/dev/usb/uvideo.c	8 Mar 2025 08:27:32 -0000	1.250
> +++ sys/dev/usb/uvideo.c	9 Mar 2025 21:13:22 -0000
> @@ -2136,6 +2136,9 @@ skip_set_alt:
>  void
>  uvideo_vs_close(struct uvideo_softc *sc)
>  {
> +	if (usbd_is_dying(sc->sc_udev))
> +		return;
> +
>  	if (sc->sc_vs_cur->bulk_running == 1) {
>  		sc->sc_vs_cur->bulk_running = 0;
>  
>