Re: [PATCH] usb-gadget-ether: Prevent oops caused by error interrupt race -V2 (comments update)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> Sigh. I need a real deep look inside that code to understand, why
> tx_reqs is not a requestlist but a freelist. Very intuitive naming :)

It *is* a list of requests:  free ones -- the only kind this level of
driver is allowed to remember!  ;)

Yeah, I had to go back and read the driver again before I understood
just what problem this patch was trying to fix.  Which is why I wanted
to make sure the mismatch between comments and contents was resolved.

- Dave

==========	CUT HERE
From: Benedikt Spranger <[email protected]>

This patch fixes a longstanding race in the Ethernet gadget driver,
which can cause an oops on device disconnect.  The fix is just to
make the TX path check whether its freelist is empty.  That check
is otherwise not necessary, since the queue is always stopped when
that list empties (and restarted when request completion puts an
entry back on that freelist).

The race window starts when the network code decides to transmit a
packet, and ends when hard_start_xmit() grabs the freelist lock.
When disconnect() is called inside that window, it shuts down the
TX queue and breaks the otherwise-solid assumption that packets are
never sent through a TX queue that's stopped.

Signed-off-by: Benedikt Spranger <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: David Brownell <[email protected]>

--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -1989,8 +1989,20 @@ static int eth_start_xmit (struct sk_buff *skb, struct net_device *net)
 	}
 
 	spin_lock_irqsave(&dev->req_lock, flags);
+	/*
+	 * this freelist can be empty if an interrupt triggered disconnect()
+	 * and reconfigured the gadget (shutting down this queue) after the
+	 * network stack decided to xmit but before we got the spinlock.
+	 */
+	if (list_empty(&dev->tx_reqs)) {
+		spin_unlock_irqrestore(&dev->req_lock, flags);
+		return 1;
+	}
+
 	req = container_of (dev->tx_reqs.next, struct usb_request, list);
 	list_del (&req->list);
+
+	/* temporarily stop TX queue when the freelist empties */
 	if (list_empty (&dev->tx_reqs))
 		netif_stop_queue (net);
 	spin_unlock_irqrestore(&dev->req_lock, flags);


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux