On 25/11/24(Mon) 20:58, Kirill A. Korinsky wrote:
> tech@,
> 
> uvideo supports two modes of webcam reading, bulk and non-bulk. In bulk
> mode it wraps by locks usbd_transfer to prevent call usbd_close_pipe on
> used pipe, and in case of non-bulk, no lock here.

What do you mean by locks?

> When close had happened, it may lead to crash if non-bulk decided to
> re-setup usbd_transfer in parallel thread.

I don't understand what you say.  To me the issue seem to come from the
fact that uvideo_vs_close() is called twice, possibly because the first
one sleeps.

> Here is the thread https://marc.info/?t=173228582400007&r=1&w=2 where I
> encountered this crash, and this patch fixes it.

The diff below prevent the crash by clearing pointers before going to
sleep.  Which means the "second" close won't trigger a user-after-free.

I'm not sure the order of operations below is correct.  usbd_ref_wait()
is possibly where the thread being killed is waiting.  You can confirm
that by killing ffplay and looking on which channel it hangs.

Then comes the question, why does the bulk thread doesn't stop?  Is it
stuck?  Where is it sleeping?

> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.222 uvideo.c
> --- sys/dev/usb/uvideo.c	1 Sep 2024 03:09:00 -0000	1.222
> +++ sys/dev/usb/uvideo.c	25 Nov 2024 19:13:11 -0000
> @@ -1933,15 +1933,18 @@ uvideo_vs_open(struct uvideo_softc *sc)
>  void
>  uvideo_vs_close(struct uvideo_softc *sc)
>  {
> -	if (sc->sc_vs_cur->bulk_running == 1) {
> +	struct usbd_pipe	*pipeh;
> +
> +	if (sc->sc_vs_cur->bulk_running == 1)
>  		sc->sc_vs_cur->bulk_running = 0;
> -		usbd_ref_wait(sc->sc_udev);
> -	}
>  
> -	if (sc->sc_vs_cur->pipeh) {
> -		usbd_close_pipe(sc->sc_vs_cur->pipeh);
> -		sc->sc_vs_cur->pipeh = NULL;
> -	}
> +	pipeh = sc->sc_vs_cur->pipeh;
> +	sc->sc_vs_cur->pipeh = NULL;
> +
> +	usbd_ref_wait(sc->sc_udev);
> +
> +	if (pipeh)
> +		usbd_close_pipe(pipeh);
>  
>  	/*
>  	 * Some devices need time to shutdown before we switch back to
> @@ -2063,6 +2066,11 @@ uvideo_vs_start_isoc_ixfer(struct uvideo
>  	for (i = 0; i < sc->sc_nframes; i++)
>  		ixfer->size[i] = sc->sc_vs_cur->psize;
>  
> +	usbd_ref_incr(sc->sc_udev);
> +
> +	if (sc->sc_vs_cur->pipeh == NULL)
> +		goto skip;
> +
>  	usbd_setup_isoc_xfer(
>  	    ixfer->xfer,
>  	    sc->sc_vs_cur->pipeh,
> @@ -2077,6 +2085,9 @@ uvideo_vs_start_isoc_ixfer(struct uvideo
>  		DPRINTF(1, "%s: usbd_transfer error=%s!\n",
>  		    DEVNAME(sc), usbd_errstr(error));
>  	}
> +
> +skip:
> +	usbd_ref_decr(sc->sc_udev);
>  }
>  
>  void
> 
> 
> -- 
> wbr, Kirill
>