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.
However, clear_bit() is racy, for IFORCE_XMIT_RUNNING bit is set by
iforce_send_packet() at the previous line.
> wake_up(&iforce_usb->iforce.wait);
>
> input_unregister_device(iforce_usb->iforce.dev);
>
> usb_free_urb(iforce_usb->irq);
> usb_free_urb(iforce_usb->out);
>
> kfree(iforce_usb);
> }
>
> I am sorry if I'm overlooking anything, especially because I'm entering
> this thread without reading the other messages and so without knowing the
> whole context. Furthermore I haven't even test-compiled these changes :-(
So far, I asked syzbot to test
--- a/drivers/input/joystick/iforce/iforce-usb.c
+++ b/drivers/input/joystick/iforce/iforce-usb.c
@@ -258,6 +258,9 @@ static void iforce_usb_disconnect(struct usb_interface *intf)
usb_set_intfdata(intf, NULL);
+ usb_poison_urb(iforce_usb->irq);
+ usb_poison_urb(iforce_usb->out);
+
input_unregister_device(iforce_usb->iforce.dev);
usb_free_urb(iforce_usb->irq);
which still triggered this problem, and
--- a/drivers/input/joystick/iforce/iforce-main.c
+++ b/drivers/input/joystick/iforce/iforce-main.c
@@ -200,8 +200,10 @@ static void iforce_close(struct input_dev *dev)
/* Disable force feedback playback */
iforce_send_packet(iforce, FF_CMD_ENABLE, "\001");
/* Wait for the command to complete */
- wait_event_interruptible(iforce->wait,
- !test_bit(IFORCE_XMIT_RUNNING, iforce->xmit_flags));
+ wait_event_interruptible_timeout
+ (iforce->wait,
+ !test_bit(IFORCE_XMIT_RUNNING, iforce->xmit_flags),
+ 5 * HZ);
}
iforce->xport_ops->stop_io(iforce);
which did not trigger this problem.
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...
