lkml.org 
[lkml]   [2022]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [syzbot] INFO: task hung in __input_unregister_device (4)
    Date
    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@googlegroups.com.
    > > 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.
    > >
    >
    >
    >
    >
    >




    \
     
     \ /
      Last update: 2022-07-21 20:17    [W:3.811 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site