Re: WARNING in wdm_write/usb_submit_urb

From: Oliver Neukum <oneukum@suse.com>
To: "Bjørn Mork" <bjorn@mork.no>
Cc: gustavo@embeddedor.com, andreyknvl@google.com,
	syzkaller-bugs@googlegroups.com, gregkh@linuxfoundation.org,
	syzbot <syzbot+d232cca6ec42c2edb3fc@syzkaller.appspotmail.com>,
	linux-usb@vger.kernel.org
Subject: Re: WARNING in wdm_write/usb_submit_urb
Date: Tue, 20 Aug 2019 14:27:01 +0200	[thread overview]
Message-ID: <1566304021.11678.13.camel@suse.com> (raw)
In-Reply-To: <87sgpw5d6j.fsf@miraculix.mork.no>

Am Dienstag, den 20.08.2019, 12:44 +0200 schrieb Bjørn Mork :
> Oliver Neukum <oneukum@suse.com> writes:
> 
> > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> > index 1656f5155ab8..a341081a5f47 100644
> > --- a/drivers/usb/class/cdc-wdm.c
> > +++ b/drivers/usb/class/cdc-wdm.c
> > @@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
> >  {
> >  	struct wdm_device *desc = file->private_data;
> >  
> > -	wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
> > +	wait_event(desc->wait,
> > +			/*
> > +			 * needs both flags. We cannot do with one
> > +			 * because resetting it would cause a race
> > +			 * with write() yet we need to signal
> > +			 * a disconnect
> > +			 */
> > +			!test_bit(WDM_IN_USE, &desc->flags) &&
> > +			!test_bit(WDM_DISCONNECTING, &desc->flags));
> 
> 
> Makes sense.  But isn't the WDM_DISCONNECTING test inverted?

You are right. I am making V3.

> >  	/* cannot dereference desc->intf if WDM_DISCONNECTING */
> >  	if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
> >  		dev_err(&desc->intf->dev, "Error in flush path: %d\n",
> >  			desc->werr);
> >  
> > -	return usb_translate_errors(desc->werr);
> > +	return test_bit(WDM_DISCONNECTING, &desc->flags) ? 
> > +			-ENODEV : 
> > +			usb_translate_errors(desc->werr);
> >  }
> 
> Minor detail, but there's an awful lot of test_bit(WDM_DISCONNECTING)
> here.  How about
> 
>   if (test_bit(WDM_DISCONNECTING, &desc->flags))
>     return -ENODEV;
>   if (desc->werr < 0)
>     dev_err(&desc->intf->dev, "Error in flush path: %d\n", desc->werr);
>   return usb_translate_errors(desc->werr);

Much better.

	Regards
		Oliver