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. 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. > > It did not trigger this problem because of _timeout(), I guess. Right. > > 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(). >> 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. 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.
