Re: [syzbot] INFO: task hung in __input_unregister_device (4)

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

On venerdì 22 luglio 2022 16:39:09 CEST Tetsuo Handa wrote:
> 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.

I haven't spent time looking closely at this driver, I'm also reacting at 
what you said about to signal the waiter that the flag changed.

First of all, I want to thank you because (1) I see how much time you use 
to spend fixing tons of bugs reported by Syzbot and (2) _you_ made the 
analysis which easily lead me to this "proof of concept" diff 
(acknowledgment is due!).  

I sent this patch for two different reasons:

1) If it passes, and it actually passes tests, I probably go deeper and see 
if it is enough or other things must be considered. You mentioned another 
case where it cannot work, but I have had no time to see it yet. 

2) Actually I didn't like that you made a timeout wait. I wanted to "prove" 
that Syzbot tests _can_ pass for a myriad reasons, but this is not a 
guarantee that a patch is "good".

> 
> 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.

I'm sorry, you are right. No mutex. In fact you see no mutexes in my patch.

I had misunderstood easily what you said because I had no context. I have 
not yet all the necessary context to prepare a "real" patch. As said, it 
was only a "proof of concept".


> > 
> > It did not trigger this problem because of _timeout(), I guess.
> 
> Right.

This is not something you should do, since you have much more experience to 
figure out how to fix it properly :-)

> > 
> > 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().

It sounds reasonable.

> >> 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.

I didn't check. For what I saw in other cases, he knows what he does ;)

> 
>   commit c2b27ef672992a206e5b221b8676972dd840ffa5
>   Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>   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@physik.uni-muenchen.de>
>       Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> 
> Dmitry, what do you think? Even without iforce_usb_disconnect() race,
> a joystick device not responding for many seconds would be annoying.

Thanks,

Fabio