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

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

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