linux-kernel - Re: [PATCH v2] usb: hub: add retry routine after intr URB sumbit error

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e4e9c9ec8eee3978f2c88aef22b3d042d5695f75.camel@suse.de>
Date:   Mon, 19 Nov 2018 16:52:10 +0100
From:   Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
To:     Oliver Neukum <oneukum@...e.com>, stern@...land.harvard.edu
Cc:     gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH v2] usb: hub: add retry routine after intr URB sumbit
 error

Hi Oliver,
thanks for the reviews :).

On Mon, 2018-11-19 at 15:04 +0100, Oliver Neukum wrote:
> On Mo, 2018-11-19 at 15:02 +0100, Nicolas Saenz Julienne wrote:
> > +static void hub_retry_irq_urb(struct timer_list *t)
> > +{
> > +	struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
> > +	int status;
> > +
> > +	if (hub->disconnected || hub->quiescing)
> > +		return;
> > +
> > +	dev_err(hub->intfdev, "retrying int urb\n");
> > +	status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> > +	if (status && status != -ENODEV && status != -EPERM &&
> > +	    status != -ESHUTDOWN)
> > +		mod_timer(&hub->irq_urb_retry,
> > +			  jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> > +}
> > +
> >  static void kick_hub_wq(struct usb_hub *hub)
> >  {
> >  	struct usb_interface *intf;
> > @@ -713,8 +729,12 @@ static void hub_irq(struct urb *urb)
> >  		return;
> >  
> >  	status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> > -	if (status != 0 && status != -ENODEV && status != -EPERM)
> > +	if (status != 0 && status != -ENODEV && status != -EPERM &&
> > +	    status != -ESHUTDOWN) {
> >  		dev_err(hub->intfdev, "resubmit --> %d\n", status);
> > +		mod_timer(&hub->irq_urb_retry,
> > +			  jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> > +	}
> >  }
> >  
> >  /* USB 2.0 spec Section 11.24.2.3 */
> > @@ -1268,6 +1288,7 @@ static void hub_quiesce(struct usb_hub *hub,
> > enum hub_quiescing_type type)
> >  	}
> >  
> >  	/* Stop hub_wq and related activity */
> > +	del_timer_sync(&hub->irq_urb_retry);
> 
> That is a race condition. You kill the timer here, but the URB may
> still be in flight. And if it fails, it will restart the error
> handler. You have to introduce a flag or poison the URB.

I see, wouldn't checking "hub->quiescing" in hub_irq()'s submit error
path do the work? Something the likes of this:

@@ -713,8 +729,12 @@ static void hub_irq(struct urb *urb)
  		return;

 	status = usb_submit_urb(hub->urb, GFP_ATOMIC);
-	if (status != 0 && status != -ENODEV && status != -EPERM)
+	if (status != 0 && status != -ENODEV && status != -EPERM &&
+	    status != -ESHUTDOWN && !hub->quiescing) {
		dev_err(hub->intfdev, "resubmit --> %d\n", status);
+		mod_timer(&hub->irq_urb_retry,
+			  jiffies + msecs_to_jiffies(MSEC_PER_SEC));
+	}

> 
> >  	usb_kill_urb(hub->urb);
> >  	if (hub->has_indicators)
> >  		cancel_delayed_work_sync(&hub->leds);
> > 
> 
> 	Regards
> 		Oliver
> 

Also, looking at this made me realize that there is no real need to
check "hub->disconnected" in the timer's function, since "hub-
>quiescing" is also set in that code path before any actual cleanup.
I'll update it in the next revision.

Regards,
Nicolas


Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)