On venerdì 22 luglio 2022 16:39:09 CEST Tetsuo Handa wrote: > On 2022/07/22 22:53, syzbot wrote: > > patch: https://syzkaller.appspot.com/x/patch.diff? x=1141355e080000 > > This patch helps only if iforce_usb_disconnect() is called while waiting at > wait_event_interruptible(iforce->wait, !test_bit(IFORCE_XMIT_RUNNING, iforce->xmit_flags)). > > It is possible that iforce_usb_disconnect() is called before > iforce_send_packet(iforce, FF_CMD_ENABLE, "\001") sets IFORCE_XMIT_RUNNING bit. I haven't spent time looking closely at this driver, I'm also reacting at what you said about to signal the waiter that the flag changed. First of all, I want to thank you because (1) I see how much time you use to spend fixing tons of bugs reported by Syzbot and (2) _you_ made the analysis which easily lead me to this "proof of concept" diff (acknowledgment is due!). I sent this patch for two different reasons: 1) If it passes, and it actually passes tests, I probably go deeper and see if it is enough or other things must be considered. You mentioned another case where it cannot work, but I have had no time to see it yet. 2) Actually I didn't like that you made a timeout wait. I wanted to "prove" that Syzbot tests _can_ pass for a myriad reasons, but this is not a guarantee that a patch is "good". > > On 2022/07/22 1:53, Fabio M. De Francesco wrote: > > On giovedì 21 luglio 2022 17:06:26 CEST Tetsuo Handa wrote: > >> On 2022/07/21 23:45, Fabio M. De Francesco wrote: > >>> If it can be fixed, as you said, by a simple notification to > >>> wait_event_interruptible(), why not changing iforce_usb_disconnect() the > >>> following way? > >>> > >>> static void iforce_usb_disconnect(struct usb_interface *intf) > >>> { > >>> struct iforce_usb *iforce_usb = usb_get_intfdata(intf); > >>> > >>> usb_set_intfdata(intf, NULL); > >>> > >>> __set_bit(IFORCE_XMIT_RUNNING, iforce_usb- >iforce.xmit_flags); > >> > >> I assume you meant clear_bit() here, for > >> > >> wait_event_interruptible(iforce->wait, > >> !test_bit(IFORCE_XMIT_RUNNING, iforce->xmit_flags)); > >> > >> waits until IFORCE_XMIT_RUNNING bit is cleared. > >> > > > > Sorry, yes you are correct. I didn't note that negation of test_bit(). > > However, you understood what I was trying to convey :-) > > > >> However, clear_bit() is racy, for IFORCE_XMIT_RUNNING bit is set by > >> iforce_send_packet() at the previous line. > > > > Why not protecting with a mutex, I mean both in iforce_usb_disconnect() and > > soon before calling iforce_send_packet() in iforce_close()? > > Protecting with a mutex does not help. It is possible that clear_bit(IFORCE_XMIT_RUNNING) > is called before iforce_send_packet() is called. I'm sorry, you are right. No mutex. In fact you see no mutexes in my patch. I had misunderstood easily what you said because I had no context. I have not yet all the necessary context to prepare a "real" patch. As said, it was only a "proof of concept". > > > > It did not trigger this problem because of _timeout(), I guess. > > Right. This is not something you should do, since you have much more experience to figure out how to fix it properly :-) > > > > If I recall correctly, this task hanged in wait_event_interruptible() and > > your problem was how to clear that bit and make the task return from > > wait_event_interruptible(). Correct? > > Not limited to clearing IFORCE_XMIT_RUNNING bit. We could introduce a new > bit for disconnect event and check both bits at wait_event_interruptible(). It sounds reasonable. > >> Since wait_event_interruptible() was used here, I think we can expect that > >> it is tolerable to continue without waiting for the command to complete... > > > > Ah, yes. Maybe you are right here but I wouldn't bet on what authors > > thought when they called wait_event_interruptible() :-) > > The author who added this wait_event_interruptible() call is Dmitry Torokhov. I didn't check. For what I saw in other cases, he knows what he does ;) > > commit c2b27ef672992a206e5b221b8676972dd840ffa5 > Author: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> > Date: Wed Dec 30 12:18:24 2009 -0800 > > Input: iforce - wait for command completion when closing the device > > We need to wait for the command to disable FF effects to complete before > continuing with closing the device. > > Tested-by: Johannes Ebke <johannes.ebke@xxxxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx> > > Dmitry, what do you think? Even without iforce_usb_disconnect() race, > a joystick device not responding for many seconds would be annoying. Thanks, Fabio
