Commit 5b6e9bcd authored by Ming Lei's avatar Ming Lei Committed by David S. Miller
Browse files

usbnet: fix skb traversing races during unlink(v2) 



Commit 4231d47e(net/usbnet: avoid
recursive locking in usbnet_stop()) fixes the recursive locking
problem by releasing the skb queue lock before unlink, but may
cause skb traversing races:
	- after URB is unlinked and the queue lock is released,
	the refered skb and skb->next may be moved to done queue,
	even be released
	- in skb_queue_walk_safe, the next skb is still obtained
	by next pointer of the last skb
	- so maybe trigger oops or other problems

This patch extends the usage of entry->state to describe 'start_unlink'
state, so always holding the queue(rx/tx) lock to change the state if
the referd skb is in rx or tx queue because we need to know if the
refered urb has been started unlinking in unlink_urbs.

The other part of this patch is based on Huajun's patch:
always traverse from head of the tx/rx queue to get skb which is
to be unlinked but not been started unlinking.

Signed-off-by: default avatarHuajun Li <huajun.li.lee@gmail.com>
Signed-off-by: default avatarMing Lei <tom.leiming@gmail.com>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: stable@kernel.org
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 8aa51d64

drivers/net/usb/usbnet.c

+38 −16
Original line number Diff line number Diff line
@@ -282,17 +282,32 @@ int usbnet_change_mtu (struct net_device *net, int new_mtu) 
}

EXPORT_SYMBOL_GPL(usbnet_change_mtu);



/* The caller must hold list->lock */

static void __usbnet_queue_skb(struct sk_buff_head *list,

			struct sk_buff *newsk, enum skb_state state)

{

	struct skb_data *entry = (struct skb_data *) newsk->cb;



	__skb_queue_tail(list, newsk);

	entry->state = state;

}



/*-------------------------------------------------------------------------*/



/* some LK 2.4 HCDs oopsed if we freed or resubmitted urbs from

 * completion callbacks.  2.5 should have fixed those bugs...

 */



static void defer_bh(struct usbnet *dev, struct sk_buff *skb, struct sk_buff_head *list)

static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb,

		struct sk_buff_head *list, enum skb_state state)

{

	unsigned long		flags;

	enum skb_state 		old_state;

	struct skb_data *entry = (struct skb_data *) skb->cb;



	spin_lock_irqsave(&list->lock, flags);

	old_state = entry->state;

	entry->state = state;

	__skb_unlink(skb, list);

	spin_unlock(&list->lock);

	spin_lock(&dev->done.lock);
@@ -300,6 +315,7 @@ static void defer_bh(struct usbnet *dev, struct sk_buff *skb, struct sk_buff_hea 
	if (dev->done.qlen == 1)

		tasklet_schedule(&dev->bh);

	spin_unlock_irqrestore(&dev->done.lock, flags);

	return old_state;

}



/* some work can't be done in tasklets, so we use keventd
@@ -340,7 +356,6 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) 
	entry = (struct skb_data *) skb->cb;

	entry->urb = urb;

	entry->dev = dev;

	entry->state = rx_start;

	entry->length = 0;



	usb_fill_bulk_urb (urb, dev->udev, dev->in,
@@ -372,7 +387,7 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) 
			tasklet_schedule (&dev->bh);

			break;

		case 0:

			__skb_queue_tail (&dev->rxq, skb);

			__usbnet_queue_skb(&dev->rxq, skb, rx_start);

		}

	} else {

		netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
@@ -423,16 +438,17 @@ static void rx_complete (struct urb *urb) 
	struct skb_data		*entry = (struct skb_data *) skb->cb;

	struct usbnet		*dev = entry->dev;

	int			urb_status = urb->status;

	enum skb_state		state;



	skb_put (skb, urb->actual_length);

	entry->state = rx_done;

	state = rx_done;

	entry->urb = NULL;



	switch (urb_status) {

	/* success */

	case 0:

		if (skb->len < dev->net->hard_header_len) {

			entry->state = rx_cleanup;

			state = rx_cleanup;

			dev->net->stats.rx_errors++;

			dev->net->stats.rx_length_errors++;

			netif_dbg(dev, rx_err, dev->net,
@@ -471,7 +487,7 @@ static void rx_complete (struct urb *urb) 
				  "rx throttle %d\n", urb_status);

		}

block:

		entry->state = rx_cleanup;

		state = rx_cleanup;

		entry->urb = urb;

		urb = NULL;

		break;
@@ -482,17 +498,18 @@ block: 
		// FALLTHROUGH



	default:

		entry->state = rx_cleanup;

		state = rx_cleanup;

		dev->net->stats.rx_errors++;

		netif_dbg(dev, rx_err, dev->net, "rx status %d\n", urb_status);

		break;

	}



	defer_bh(dev, skb, &dev->rxq);

	state = defer_bh(dev, skb, &dev->rxq, state);



	if (urb) {

		if (netif_running (dev->net) &&

		    !test_bit (EVENT_RX_HALT, &dev->flags)) {

		    !test_bit (EVENT_RX_HALT, &dev->flags) &&

		    state != unlink_start) {

			rx_submit (dev, urb, GFP_ATOMIC);

			usb_mark_last_busy(dev->udev);

			return;
@@ -579,16 +596,23 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq); 
static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)

{

	unsigned long		flags;

	struct sk_buff		*skb, *skbnext;

	struct sk_buff		*skb;

	int			count = 0;



	spin_lock_irqsave (&q->lock, flags);

	skb_queue_walk_safe(q, skb, skbnext) {

	while (!skb_queue_empty(q)) {

		struct skb_data		*entry;

		struct urb		*urb;

		int			retval;



		skb_queue_walk(q, skb) {

			entry = (struct skb_data *) skb->cb;

			if (entry->state != unlink_start)

				goto found;

		}

		break;

found:

		entry->state = unlink_start;

		urb = entry->urb;



		/*
@@ -1039,8 +1063,7 @@ static void tx_complete (struct urb *urb) 
	}



	usb_autopm_put_interface_async(dev->intf);

	entry->state = tx_done;

	defer_bh(dev, skb, &dev->txq);

	(void) defer_bh(dev, skb, &dev->txq, tx_done);

}



/*-------------------------------------------------------------------------*/
@@ -1096,7 +1119,6 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, 
	entry = (struct skb_data *) skb->cb;

	entry->urb = urb;

	entry->dev = dev;

	entry->state = tx_start;

	entry->length = length;



	usb_fill_bulk_urb (urb, dev->udev, dev->out,
@@ -1155,7 +1177,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, 
		break;

	case 0:

		net->trans_start = jiffies;

		__skb_queue_tail (&dev->txq, skb);

		__usbnet_queue_skb(&dev->txq, skb, tx_start);

		if (dev->txq.qlen >= TX_QLEN (dev))

			netif_stop_queue (net);

	}

include/linux/usb/usbnet.h

+2 −1
Original line number Diff line number Diff line
@@ -191,7 +191,8 @@ extern void usbnet_cdc_status(struct usbnet *, struct urb *); 
enum skb_state {

	illegal = 0,

	tx_start, tx_done,

	rx_start, rx_done, rx_cleanup

	rx_start, rx_done, rx_cleanup,

	unlink_start

};



struct skb_data {	/* skb->cb is one of these */

CRA Git | Maintained and supported by SUSTech CRA and CCSE