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 <jeremy@xensource.com>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Christian Limpach <Christian.Limpach@cl.cam.ac.uk>
diff -r a69029562c74 linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
--- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c Thu Apr 12 14:13:33
2007 -0700
+++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c Thu Apr 12 14:19:42
2007 -0700
@@ -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)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On 12/4/07 23:58, "Jeremy Fitzhardinge" <jeremy@xensource.com> wrote:> netfront contains two locking problems found by lockdep:I want to port lockdep down into Xen at some point. It''s sweet! -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Jeremy: Jeremy Fitzhardinge <jeremy@xensource.com> wrote:> > @@ -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.> @@ -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.> 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);You can''t enable BH with IRQs disabled. Besides, for the sake of symmetry these two should be reversed.> @@ -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, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 13/4/07 13:10, "Herbert Xu" <herbert@gondor.apana.org.au> wrote:> The ->poll method is guaranteed to be called with BH disabled so > this isn''t necessary.Thanks.>> @@ -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()!> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Herbert Xu wrote:> 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.>> @@ -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. >OK.>> 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); >> > > 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. J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, Apr 13, 2007 at 03:22:41PM +0100, Keir Fraser wrote:> > >> @@ -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, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel