linux-kernel - Re: [syzbot] INFO: task hung in __input_unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:   Thu, 21 Jul 2022 20:16:59 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     syzkaller-bugs@...glegroups.com,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Johan Hovold <johan@...nel.org>,
        syzkaller-bugs@...glegroups.com, rydberg@...math.org,
        syzbot <syzbot+deb6abc36aad4008f407@...kaller.appspotmail.com>,
        linux-input@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>, ira.weiny@...el.com
Subject: Re: [syzbot] INFO: task hung in __input_unregister_device (4)

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