On Tue, 26 Nov 2024 13:04:20 +0100,
Kirill A. Korinsky <kirill@korins.ky> wrote:
> 
> On Tue, 26 Nov 2024 12:56:10 +0100,
> Martin Pieuchot <mpi@grenadille.net> wrote:
> > 
> > On 26/11/24(Tue) 12:35, Kirill A. Korinsky wrote:
> > > On Tue, 26 Nov 2024 11:21:14 +0100,
> > > Martin Pieuchot <mpi@grenadille.net> wrote:
> > > > 
> > > > 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?
> > > >
> > > 
> > > Seems my wording wasn't good. I mean that I use usbd_ref_wait as lock to
> > > block the second call of close.
> > 
> > I don't understand what you mean.
> > 
> > > I also was wrong that it goes to non-bulk code path, and you're right. The
> > > code is blocked at usbd_transfer inside uvideo_vs_start_bulk_thread.
> > 
> > That's interesting.  So the bulk thread is waiting for the transfer to
> > timeout before it can check `bulk_running' and finally decrement the
> > refcounter?  Is it so?
> > 
> > In that case, does calling usbd_abort_pipe() before usbd_ref_wait()
> > help?
> > 
> > > When I add a very short timeout (1s), I can't crash system because anymore,
> > > but for longer timeout (10s) I still can do it.
> > 
> > Are you talking about the usbd_ref_wait() timeout?  If so, that confirms
> > the double-free due to a double close.
> > 
> > I wonder if we should not have a mechanism a layer above to prevent
> > double close()...
> > 
> > 
> 
> I had reverted all my changes and tested with 1000 (1s) and 10000 (10s):
> 

This seems quite better solution: to abort pipe when it is blocked inside
usbd_transfer.

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	26 Nov 2024 13:40:00 -0000
@@ -1935,6 +1935,11 @@ uvideo_vs_close(struct uvideo_softc *sc)
 {
 	if (sc->sc_vs_cur->bulk_running == 1) {
 		sc->sc_vs_cur->bulk_running = 0;
+
+		/* Bulk thread may sleep in usbd_transfer, abort it */
+		if (sc->sc_vs_cur->pipeh)
+			usbd_abort_pipe(sc->sc_vs_cur->pipeh);
+
 		usbd_ref_wait(sc->sc_udev);
 	}
 

-- 
wbr, Kirill