On giovedì 21 luglio 2022 18:53:27 CEST 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()?
>
> > > 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.
^^^^^ Sorry, "clear" -> "trigger" ^^^^^
However, I suppose it doesn't matter any longer.
Thanks,
Fabio
>
> 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.
> >
>
>
>
>
>
