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()?
> > 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.
It did not clear this problem because of _timeout(), I guess.
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?
Now you changed this code to return after some time, despite that flag.
Are you sure this is the better suited way to fix this bug?
>
> 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() :-)
Thanks,
Fabio
> --
> You received this message because you are subscribed to the Google Groups
"syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to syzkaller-bugs+unsubscribe@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/
msgid/syzkaller-bugs/2bcd5385-2423-2e8f-be01-9db93afaba43%40I-
love.SAKURA.ne.jp.
>
