From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: sys/uvideo: fix crash on close isochronous endpoint's webcam
To: Marcus Glocker <marcus@nazgul.ch>
Cc: tech@openbsd.org
Date: Sun, 09 Mar 2025 22:13:47 +0100

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?

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;