netfront: Lockdep fixes

PATCH In review Jeremy Fitzhardinge 4 participants 6 messages Apr 12, 2007 – Apr 13, 2007

lore

Share & tools

lore

Jeremy Fitzhardinge

author

2007-04-12 22:58 UTC #0 lore raw

PATCH 1/1 netfront: Lockdep fixes

netfront contains two locking problems found by lockdep:

1. rx_lock is a normal spinlock, and tx_lock is an irq spinlock. This means that in normal use, tx_lock may be taken by an interrupt routine while rx_lock is held. However, netif_disconnect_backend takes them in the order tx_lock->rx_lock, which could lead to a deadlock. Reverse them.
2. rx_lock can also be used in softirq context, so it should be taken/released with spin_(un)lock_bh.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Chris Wright <[email protected]>

Cc Christian Limpach <[email protected]>

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +15 ~~-15~~

diff -r a69029562c74 linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
@@ -622,14 +622,14 @@ static int network_open(struct net_devic
 
 	memset(&np->stats, 0, sizeof(np->stats));
 
-	spin_lock(&np->rx_lock);
+	spin_lock_bh(&np->rx_lock);
 	if (netfront_carrier_ok(np)) {
 		network_alloc_rx_buffers(dev);
 		np->rx.sring->rsp_event = np->rx.rsp_cons + 1;
 		if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx))
 			netif_rx_schedule(dev);
 	}
-	spin_unlock(&np->rx_lock);
+	spin_unlock_bh(&np->rx_lock);
 
 	network_maybe_wake_tx(dev);

@@ -1307,10 +1307,10 @@ static int netif_poll(struct net_device 
 	int pages_flipped = 0;
 	int err;
 
-	spin_lock(&np->rx_lock);
+	spin_lock_bh(&np->rx_lock);
 
 	if (unlikely(!netfront_carrier_ok(np))) {
-		spin_unlock(&np->rx_lock);
+		spin_unlock_bh(&np->rx_lock);
 		return 0;
 	}

@@ -1478,7 +1478,7 @@ err:
 		local_irq_restore(flags);
 	}
 
-	spin_unlock(&np->rx_lock);
+	spin_unlock_bh(&np->rx_lock);
 
 	return more_to_do;
 }

@@ -1520,7 +1520,7 @@ static void netif_release_rx_bufs(struct
 
 	skb_queue_head_init(&free_list);
 
-	spin_lock(&np->rx_lock);
+	spin_lock_bh(&np->rx_lock);
 
 	for (id = 0; id < NET_RX_RING_SIZE; id++) {
 		if ((ref = np->grant_rx_ref[id]) == GRANT_INVALID_REF) {

@@ -1588,7 +1588,7 @@ static void netif_release_rx_bufs(struct
 	while ((skb = __skb_dequeue(&free_list)) != NULL)
 		dev_kfree_skb(skb);
 
-	spin_unlock(&np->rx_lock);
+	spin_unlock_bh(&np->rx_lock);
 }
 
 static int network_close(struct net_device *dev)

@@ -1708,8 +1708,8 @@ static int network_connect(struct net_de
 	IPRINTK("device %s has %sing receive path.\n",
 		dev->name, np->copying_receiver ? "copy" : "flipp");
 
+	spin_lock_bh(&np->rx_lock);
 	spin_lock_irq(&np->tx_lock);
-	spin_lock(&np->rx_lock);
 
 	/*
 	 * Recovery procedure:

@@ -1761,7 +1761,7 @@ static int network_connect(struct net_de
 	network_tx_buf_gc(dev);
 	network_alloc_rx_buffers(dev);
 
-	spin_unlock(&np->rx_lock);
+	spin_unlock_bh(&np->rx_lock);
 	spin_unlock_irq(&np->tx_lock);
 
 	return 0;

@@ -1818,7 +1818,7 @@ static ssize_t store_rxbuf_min(struct cl
 	if (target > RX_MAX_TARGET)
 		target = RX_MAX_TARGET;
 
-	spin_lock(&np->rx_lock);
+	spin_lock_bh(&np->rx_lock);
 	if (target > np->rx_max_target)
 		np->rx_max_target = target;
 	np->rx_min_target = target;

@@ -1827,7 +1827,7 @@ static ssize_t store_rxbuf_min(struct cl
 
 	network_alloc_rx_buffers(netdev);
 
-	spin_unlock(&np->rx_lock);
+	spin_unlock_bh(&np->rx_lock);
 	return len;
 }

@@ -1861,7 +1861,7 @@ static ssize_t store_rxbuf_max(struct cl
 	if (target > RX_MAX_TARGET)
 		target = RX_MAX_TARGET;
 
-	spin_lock(&np->rx_lock);
+	spin_lock_bh(&np->rx_lock);
 	if (target < np->rx_min_target)
 		np->rx_min_target = target;
 	np->rx_max_target = target;

@@ -1870,7 +1870,7 @@ static ssize_t store_rxbuf_max(struct cl
 
 	network_alloc_rx_buffers(netdev);
 
-	spin_unlock(&np->rx_lock);
+	spin_unlock_bh(&np->rx_lock);
 	return len;
 }

@@ -2033,10 +2033,10 @@ static void netif_disconnect_backend(str
 static void netif_disconnect_backend(struct netfront_info *info)
 {
 	/* Stop old i/f to prevent errors whilst we rebuild the state. */
+	spin_lock_bh(&info->rx_lock);
 	spin_lock_irq(&info->tx_lock);
-	spin_lock(&info->rx_lock);
 	netfront_carrier_off(info);
-	spin_unlock(&info->rx_lock);
+	spin_unlock_bh(&info->rx_lock);
 	spin_unlock_irq(&info->tx_lock);
 
 	if (info->irq)

Keir Fraser

2007-04-13 09:50 UTC #1 lore raw

Quoted 1 line · Jeremy Fitzhardinge

netfront contains two locking problems found by lockdep:

I want to port lockdep down into Xen at some point. It's sweet!

-- Keir

Herbert Xu

2007-04-13 12:10 UTC #2 lore raw

In reply to Jeremy Fitzhardinge

Hi Jeremy:

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c static int netif_poll(struct net_device · line 1310

Quoted 7 lines · Jeremy Fitzhardinge

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +1 -1

@@ -1307,10 +1307,10 @@ static int netif_poll(struct net_device 
       int pages_flipped = 0;
       int err;
 
-       spin_lock(&np->rx_lock);
+       spin_lock_bh(&np->rx_lock);

The ->poll method is guaranteed to be called with BH disabled so this isn't necessary.

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c static void netif_release_rx_bufs(struct · line 1592

Quoted 7 lines

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +1 -1

@@ -1588,7 +1588,7 @@ static void netif_release_rx_bufs(struct
       while ((skb = __skb_dequeue(&free_list)) != NULL)
               dev_kfree_skb(skb);
 
-       spin_unlock(&np->rx_lock);
+       spin_unlock_bh(&np->rx_lock);
 }

Just a minor nit. This is normally called with BH disabled, except from uninit so you could put a local_bh_disable there instead.

Quoted 12 lines

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +1 -1

 static int network_close(struct net_device *dev)

@@ -1708,8 +1708,8 @@ static int network_connect(struct net_de
       IPRINTK("device %s has %sing receive path.\n",
               dev->name, np->copying_receiver ? "copy" : "flipp");
 
+       spin_lock_bh(&np->rx_lock);
       spin_lock_irq(&np->tx_lock);
-       spin_lock(&np->rx_lock);
 
       /*
        * Recovery procedure:

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c static int network_connect(struct net_de · line 1765

Quoted 6 lines

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +1 -1

@@ -1761,7 +1761,7 @@ static int network_connect(struct net_de
       network_tx_buf_gc(dev);
       network_alloc_rx_buffers(dev);
 
-       spin_unlock(&np->rx_lock);
+       spin_unlock_bh(&np->rx_lock);
       spin_unlock_irq(&np->tx_lock);

You can't enable BH with IRQs disabled. Besides, for the sake of symmetry these two should be reversed.

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c static void netif_disconnect_backend(str · line 2040

Quoted 11 lines

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +2 -2

@@ -2033,10 +2033,10 @@ static void netif_disconnect_backend(str
 static void netif_disconnect_backend(struct netfront_info *info)
 {
       /* Stop old i/f to prevent errors whilst we rebuild the state. */
+       spin_lock_bh(&info->rx_lock);
       spin_lock_irq(&info->tx_lock);
-       spin_lock(&info->rx_lock);
       netfront_carrier_off(info);
-       spin_unlock(&info->rx_lock);
+       spin_unlock_bh(&info->rx_lock);
       spin_unlock_irq(&info->tx_lock);

Ditto.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Keir Fraser

2007-04-13 14:22 UTC #3 lore raw

Quoted 2 lines · Herbert Xu

The ->poll method is guaranteed to be called with BH disabled so this isn't necessary.

Thanks.

Quoted 5 lines

Review static void netif_release_rx_bufs(struct
Quoted 1 line
(quoted diff) +0 -0
@@ -1588,7 +1588,7 @@ static void netif_release_rx_bufs(struct
Just a minor nit. This is normally called with BH disabled, except from uninit so you could put a local_bh_disable there instead.

That function's *only* called from uninit()!

Quoted 2 lines

You can't enable BH with IRQs disabled. Besides, for the sake of symmetry these two should be reversed.

Ack, I should have spotted this one, and the ditto.

-- Keir

Herbert Xu

2007-04-13 22:34 UTC #4 lore raw

Quoted 8 lines · Keir Fraser

Quoted 6 lines
Review static void netif_release_rx_bufs(struct
Quoted 2 lines
(quoted diff) +0 -0
@@ -1588,7 +1588,7 @@ static void netif_release_rx_bufs(struct
Just a minor nit. This is normally called with BH disabled, except from uninit so you could put a local_bh_disable there instead.
That function's *only* called from uninit()!

Heh, don't know how I saw that one getting called elsewhere :)

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Jeremy Fitzhardinge

author

2007-04-13 15:21 UTC #5 lore raw

In reply to Herbert Xu

Quoted 3 lines · Herbert Xu

The ->poll method is guaranteed to be called with BH disabled so this isn't necessary.

OK. As you can probably tell, I took a fairly blunt search-and-replace approach once I'd worked out what lockdep was complaining about.

Quoted 4 lines

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +0 -0

@@ -1588,7 +1588,7 @@ static void netif_release_rx_bufs(struct
       while ((skb = __skb_dequeue(&free_list)) != NULL)
               dev_kfree_skb(skb);

Quoted 9 lines

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c static int netif_poll(struct net_device · line 1316
Quoted 4 lines
linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +1 -1
-       spin_unlock(&np->rx_lock);
+       spin_unlock_bh(&np->rx_lock);
 }
 
Just a minor nit. This is normally called with BH disabled, except from uninit so you could put a local_bh_disable there instead.

OK.

Quoted 15 lines

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +1 -1

 static int network_close(struct net_device *dev)

@@ -1708,8 +1708,8 @@ static int network_connect(struct net_de
       IPRINTK("device %s has %sing receive path.\n",
               dev->name, np->copying_receiver ? "copy" : "flipp");
 
+       spin_lock_bh(&np->rx_lock);
       spin_lock_irq(&np->tx_lock);
-       spin_lock(&np->rx_lock);
 
       /*
        * Recovery procedure:

@@ -1761,7 +1761,7 @@ static int network_connect(struct net_de
       network_tx_buf_gc(dev);
       network_alloc_rx_buffers(dev);

Quoted 8 lines

linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c static int network_connect(struct net_de · line 1766
Quoted 4 lines
linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +1 -1
-       spin_unlock(&np->rx_lock);
+       spin_unlock_bh(&np->rx_lock);
       spin_unlock_irq(&np->tx_lock);
 
You can't enable BH with IRQs disabled. Besides, for the sake of symmetry these two should be reversed.

Will do.

Thanks for looking over it.