On 19/03/25(Wed) 14:18, Kirill A. Korinsky wrote:
> On Wed, 19 Mar 2025 11:35:09 +0100,
> Martin Pieuchot <mpi@grenadille.net> wrote:
> > 
> > There are two issues here.  The first one is that uvideo_detach()
> > doesn't "close" the USB endpoints which should abort the transfers.  I'd
> > suggest uvideo_detach() calls uvideo_close().
> > 
> > The second issue is that uvideo_close() doesn't close the pipe due to
> > the wrong isdying() check.  This is what aborts transfers and stop the
> > callbacks.
> > 
> > Here's an untested diff that illustrates all of this.  Note that the
> > kernel might now call uvideo_close() twice and some pointers might have
> > to be cleaned.
> > 
> > The "dying" check should be avoided as much as possible.  I don't think
> > it is necessary in the callback if the pipe is aborted during the close
> > operation.  Btw usbd_close_pipe() implies usbd_abort_pipe().
> > 
> >
> 
> Thanks for the diff, I just tested it.
> 
> > @@ -2171,8 +2172,7 @@ uvideo_vs_close(struct uvideo_softc *sc)
> >  		usbd_delay_ms(sc->sc_udev, 100);
> >  
> >  		/* switch back to default interface (turns off cam LED) */
> > -		if (!usbd_is_dying(sc->sc_udev))
> > -			(void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0);
> > +		(void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0);
> >  	}
> >  }
> >  
> 
> this hunk is required, because kernel may free memora which is pointed by
> sc_vs_cur for 100 ms wait, and system crashed.

Which implies two threads are entering the close/free.

That's the problem with the various usbd_delay_ms() inside these detach
functions which lead to context switches while the descriptor is almost
"dead".

The "close" path should not be entered twice.  Ideally this should be
prevented by calling vdevgone(9) like bpf does in bpfsdetach().  In
uvideo(4) this could be fixed by moving the usbd_delay_ms(9) *after*
calling config_detach(9) or even removing it.

Another way to prevent that, is to set a flag like tun(4) does with
TUN_DEAD in net/if_tun.c.  But this approach applied to uvideo(4) means
we don't understand what's happening.

Related to that, we should check for `sc_open' in videoclose() in the
video(4) driver and only call video_stop() if it isn't set.   Here again
the flag is set *after* calling uvideo_close() which sleeps...

> Yes, I had tested it, and crash inside usbd_set_interface is back.
> 
> Anyway, I had reused your diff in that I'm working on right now and would
> like to point that I can't crash the system anymore with inlined diff and
> https://marc.info/?l=openbsd-tech&m=174180759929893&w=2

Note that atomic functions are not necessary in this case because all
the code is running under KERNEL_LOCK().

> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.253 uvideo.c
> --- sys/dev/usb/uvideo.c	18 Mar 2025 13:38:15 -0000	1.253
> +++ sys/dev/usb/uvideo.c	19 Mar 2025 13:14:46 -0000
> @@ -24,6 +24,7 @@
>  #include <sys/stat.h>
>  #include <sys/kthread.h>
>  #include <sys/stdint.h>
> +#include <sys/atomic.h>
>  
>  #include <uvm/uvm_extern.h>
>  
> @@ -50,12 +51,18 @@ int uvideo_debug = 1;
>  #define byteof(x) ((x) >> 3)
>  #define bitof(x)  (1L << ((x) & 0x7))
>  
> +#define UVIDEO_FLAG_ATTACHED		0x00000001
> +#define UVIDEO_FLAG_CLOSED		0x00000002
> +#define UVIDEO_FLAG_PARSING_FRAME	0x00000004
> +
>  struct uvideo_softc {
>  	struct device				 sc_dev;
>  	struct usbd_device			*sc_udev;
>  	int					 sc_iface;
>  	int					 sc_nifaces;
>  
> +	int					 sc_flags;
> +
>  	struct device				*sc_videodev;
>  
>  	int					 sc_max_ctrl_size;
> @@ -465,7 +472,7 @@ uvideo_open(void *addr, int flags, int *
>  
>  	DPRINTF(1, "%s: uvideo_open: sc=%p\n", DEVNAME(sc), sc);
>  
> -	if (usbd_is_dying(sc->sc_udev))
> +	if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
>  		return (EIO);
>  
>  	/* pointers to upper video layer */
> @@ -487,6 +494,9 @@ uvideo_close(void *addr)
>  
>  	DPRINTF(1, "%s: uvideo_close: sc=%p\n", DEVNAME(sc), sc);
>  
> +	if (sc->sc_flags & UVIDEO_FLAG_CLOSED)
> +		return (EIO);
> +
>  #ifdef UVIDEO_DUMP
>  	usb_rem_task(sc->sc_udev, &sc->sc_task_write);
>  #endif
> @@ -501,6 +511,9 @@ uvideo_close(void *addr)
>  
>  	/* free video stream frame buffer */
>  	uvideo_vs_free_frame(sc);
> +
> +	atomic_setbits_int(&sc->sc_flags, UVIDEO_FLAG_CLOSED);
> +
>  	return (0);
>  }
>  
> @@ -550,6 +563,8 @@ uvideo_attach(struct device *parent, str
>  
>  	sc->sc_udev = uaa->device;
>  
> +	sc->sc_flags = UVIDEO_FLAG_ATTACHED;
> +
>  	/* Find the first unclaimed video interface. */
>  	for (i = 0; i < uaa->nifaces; i++) {
>  		if (usbd_iface_claimed(sc->sc_udev, i))
> @@ -685,13 +700,16 @@ uvideo_detach(struct device *self, int f
>  	struct uvideo_softc *sc = (struct uvideo_softc *)self;
>  	int rv = 0;
>  
> +	atomic_clearbits_int(&sc->sc_flags, UVIDEO_FLAG_ATTACHED);
> +
>  	/* Wait for outstanding requests to complete */
> -	usbd_delay_ms(sc->sc_udev, UVIDEO_NFRAMES_MAX);
> +	if (sc->sc_flags & UVIDEO_FLAG_PARSING_FRAME)
> +		usbd_delay_ms(sc->sc_udev, 1);
>  
>  	if (sc->sc_videodev != NULL)
>  		rv = config_detach(sc->sc_videodev, flags);
>  
> -	uvideo_vs_free_frame(sc);
> +	uvideo_close(sc);
>  
>  	return (rv);
>  }
> @@ -2136,7 +2154,7 @@ skip_set_alt:
>  void
>  uvideo_vs_close(struct uvideo_softc *sc)
>  {
> -	if (usbd_is_dying(sc->sc_udev))
> +	if (sc->sc_flags & UVIDEO_FLAG_CLOSED)
>  		return;
>  
>  	if (sc->sc_vs_cur->bulk_running == 1) {
> @@ -2258,7 +2276,12 @@ uvideo_vs_start_bulk_thread(void *arg)
>  
>  		DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size);
>  
> +		if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
> +			break;
> +
> +		atomic_setbits_int(&sc->sc_flags, UVIDEO_FLAG_PARSING_FRAME);
>  		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size);
> +		atomic_clearbits_int(&sc->sc_flags, UVIDEO_FLAG_PARSING_FRAME);
>  	}
>  	usbd_ref_decr(sc->sc_udev);
>  
> @@ -2283,9 +2306,6 @@ uvideo_vs_start_isoc_ixfer(struct uvideo
>  
>  	DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
>  
> -	if (usbd_is_dying(sc->sc_udev))
> -		return;
> -
>  	for (i = 0; i < sc->sc_nframes; i++)
>  		ixfer->size[i] = sc->sc_vs_cur->psize;
>  
> @@ -2335,7 +2355,12 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  			/* frame is empty */
>  			continue;
>  
> +		if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
> +			return;
> +
> +		atomic_setbits_int(&sc->sc_flags, UVIDEO_FLAG_PARSING_FRAME);
>  		sc->sc_decode_stream_header(sc, frame, frame_size);
> +		atomic_clearbits_int(&sc->sc_flags, UVIDEO_FLAG_PARSING_FRAME);
>  	}
>  
>  skip:	/* setup new transfer */
> @@ -3596,6 +3621,9 @@ uvideo_querybuf(void *v, struct v4l2_buf
>  	    qb->index >= sc->sc_mmap_count)
>  		return (EINVAL);
>  
> +	if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
> +		return (EINVAL);
> +
>  	bcopy(&sc->sc_mmap[qb->index].v4l2_buf, qb,
>  	    sizeof(struct v4l2_buffer));
>  
> @@ -3644,6 +3672,9 @@ uvideo_dqbuf(void *v, struct v4l2_buffer
>  		if (error)
>  			return (EINVAL);
>  	}
> +
> +	if (!(sc->sc_flags & UVIDEO_FLAG_ATTACHED))
> +		return (EINVAL);
>  
>  	mmap = SIMPLEQ_FIRST(&sc->sc_mmap_q);
>  	if (mmap == NULL)
>