Skip to contentSkip to conversation

Deadlock in af_packet/packet_rcv

PATCH In review Olaf Kirch 3 participants 9 messages Nov 25, 2004 – Nov 30, 2004

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Olaf Kirch

author
#0 lore raw

PATCH 1/1 Deadlock in af_packet/packet_rcv

Before introduction of lock-less loopback, all of netdev_xmit_nit was running with bottom halves disabled, and some code seems to rely on this. One of them is af_packet's packet_rcv handler, which is called from the receive path via netdev_xmit_nit. It takes a spin lock, and if an interrupt occurs and calls netdev_xmit during this time, the CPU deadlocks.

The patch below disables BHs while in the TX path for loopback and similar devices.

An alternative, less conservative fix would be to just use spin_lock_bh in af_packet.c.

Signed-off-by Olaf Kirch <[email protected]>
net/core/dev.c +2 -0
Index: linux-2.6.9/net/core/dev.c
===================================================================
@@ -1222,6 +1222,7 @@ int __skb_linearize(struct sk_buff *skb,
 }
 
 #define HARD_TX_LOCK(dev, cpu) {			\
+	local_disable_bh();				\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
 		spin_lock(&dev->xmit_lock);		\
 		dev->xmit_lock_owner = cpu;		\

@@ -1233,6 +1234,7 @@ int __skb_linearize(struct sk_buff *skb,
 		dev->xmit_lock_owner = -1;		\
 		spin_unlock(&dev->xmit_lock);		\
 	}						\
+	local_enable_bh();				\
 }
 
 static inline void qdisc_run(struct net_device *dev)
Signature · 3 lines
Olaf Kirch     | Things that make Monday morning interesting, #2:
[email protected]   |        "We have 8,000 NFS mount points, why do we keep
---------------+ 	 running out of privileged ports?"
maintainer
#1 lore raw
Quoted 7 lines · On Thu, 25 Nov 2004 21:55:03 +0100 Olaf Kirch <[email protected]> wrote:

Before introduction of lock-less loopback, all of netdev_xmit_nit was running with bottom halves disabled, and some code seems to rely on this. One of them is af_packet's packet_rcv handler, which is called from the receive path via netdev_xmit_nit. It takes a spin lock, and if an interrupt occurs and calls netdev_xmit during this time, the CPU deadlocks.

Quoted 5 lines

The patch below disables BHs while in the TX path for loopback and similar devices.

An alternative, less conservative fix would be to just use spin_lock_bh in af_packet.c.

Good spotting.

If it's only the dev_queue_xmit_nit() handlers which all want BHs disabled, why don't we just disable BHs in that function?

#3 lore raw
In reply to Olaf Kirch
Quoted 6 lines · Olaf Kirch

Before introduction of lock-less loopback, all of netdev_xmit_nit was running with bottom halves disabled, and some code seems to rely on this. One of them is af_packet's packet_rcv handler, which is called from the receive path via netdev_xmit_nit. It takes a spin lock, and if an interrupt occurs and calls netdev_xmit during this time, the CPU deadlocks.

An interrupt handler shouldn't call dev_queue_xmit() directly. If this indeed happens, it needs to be fixed. Which handler is this?

Quoted 2 lines

The patch below disables BHs while in the TX path for loopback and similar devices.

dev_queue_xmit() already have BHs disabled, so this won't do anything. And weren't you talking about interrupts ??

Protocol handlers are called with bottom halves disabled, but IRQs are enabled. This hasn't changed - and is assumed in lots of places.

-Tommy

author
#2 lore raw
In reply to David S. Miller
Quoted 2 lines · David S. Miller

If it's only the dev_queue_xmit_nit() handlers which all want BHs disabled, why don't we just disable BHs in that function?

That was my original fix (see attachment), but Andi Kleen suggested to actually disable BHs at that level; I think as a matter of caution.

Olaf

Signature · 3 lines
Olaf Kirch     | Things that make Monday morning interesting, #2:
[email protected]   |        "We have 8,000 NFS mount points, why do we keep
---------------+ 	 running out of privileged ports?"
net/core/dev.c +6 -0
Index: linux-2.6.9/net/core/dev.c
===================================================================
@@ -1094,9 +1094,15 @@ void dev_queue_xmit_nit(struct sk_buff *
 				skb2->nh.raw = skb2->data;
 			}
 
+			/* The packet handler may expect BHs to be disabled,
+			 * so don't let them down */
+			local_bh_disable();
+
 			skb2->h.raw = skb2->nh.raw;
 			skb2->pkt_type = PACKET_OUTGOING;
 			ptype->func(skb2, skb->dev, ptype);
+
+			local_bh_enable();
 		}
 	}
 	rcu_read_unlock();
author
#4 lore raw
In reply to Tommy Christensen
Quoted 2 lines · Tommy Christensen

An interrupt handler shouldn't call dev_queue_xmit() directly. If this indeed happens, it needs to be fixed. Which handler is this?

The call path according to KDB goes like this:

	application does sendmsg()
	udp_push_pending_frames 
	ip_push_pending_frames 
	ip_output 
	dev_queue_xmit 
	dev_queue_xmit_nit 
		calls ptype->func(skb2, skb->dev, ptype),
		where func=packet_rcv 
	packet_rcv (and this runs with BHs enabled)
		take the &sk->sk_receive_queue spinlock 
*** timer interrupt
	net_tx_action
		take the dev->queue_lock spin lock
	qdisc_run
	qdisc_restart
	dev_queue_xmit_nit
		as above
	packet_rcv
		blocks on the &sk->sk_receive_queue spinlock

Before lockless-loopback this never triggered because we did a spin_lock_bh(&dev->xmit_lock) around the call to dev_queue_xmit_nit.

Olaf

Signature · 3 lines
Olaf Kirch     | Things that make Monday morning interesting, #2:
[email protected]   |        "We have 8,000 NFS mount points, why do we keep
---------------+ 	 running out of privileged ports?"
#5 lore raw
Quoted 28 lines · Olaf Kirch
Quoted 2 lines · Tommy Christensen

An interrupt handler shouldn't call dev_queue_xmit() directly. If this indeed happens, it needs to be fixed. Which handler is this?

The call path according to KDB goes like this:

	application does sendmsg()
	udp_push_pending_frames 
	ip_push_pending_frames 
	ip_output 
	dev_queue_xmit 
	dev_queue_xmit_nit 
		calls ptype->func(skb2, skb->dev, ptype),
		where func=packet_rcv 
	packet_rcv (and this runs with BHs enabled)
		take the &sk->sk_receive_queue spinlock 
*** timer interrupt
	net_tx_action
		take the dev->queue_lock spin lock
	qdisc_run
	qdisc_restart
	dev_queue_xmit_nit
		as above
	packet_rcv
		blocks on the &sk->sk_receive_queue spinlock
Quoted 4 lines

Before lockless-loopback this never triggered because we did a spin_lock_bh(&dev->xmit_lock) around the call to dev_queue_xmit_nit.

Olaf

Ahh, back-traces are *so* nice to have.

I still don't agree with the conclusion, though. The spin_lock_bh() is changed to a local_bh_disable() and an optional spin_lock(). That should not lead to what you are seeing!

I think perhaps your 'BH disabled count' has been corrupted.

There's a fix for that in 2.6.10-rc2.

-Tommy

author
#6 lore raw
Quoted 3 lines · Tommy Christensen

I still don't agree with the conclusion, though. The spin_lock_bh() is changed to a local_bh_disable() and an optional spin_lock(). That should not lead to what you are seeing!

Well, the code in 2.6.9 has

#define HARD_TX_LOCK(dev, cpu) {                        \
	if ((dev->features & NETIF_F_LLTX) == 0) {      \
		spin_lock(&dev->xmit_lock);             \
		dev->xmit_lock_owner = cpu;             \
	}                                               \
}

i.e. there's no local_bh_disable at all - adding the local_bh_disable was the whole point of my patch. Or did you refer to a different spinlock?

Olaf

Signature · 3 lines
Olaf Kirch     | Things that make Monday morning interesting, #2:
[email protected]   |        "We have 8,000 NFS mount points, why do we keep
---------------+ 	 running out of privileged ports?"
#7 lore raw
Quoted 16 lines · Olaf Kirch
Quoted 3 lines · Tommy Christensen

I still don't agree with the conclusion, though. The spin_lock_bh() is changed to a local_bh_disable() and an optional spin_lock(). That should not lead to what you are seeing!

Well, the code in 2.6.9 has

#define HARD_TX_LOCK(dev, cpu) {                        \
	if ((dev->features & NETIF_F_LLTX) == 0) {      \
		spin_lock(&dev->xmit_lock);             \
		dev->xmit_lock_owner = cpu;             \
	}                                               \
}
Quoted 2 lines

i.e. there's no local_bh_disable at all - adding the local_bh_disable was the whole point of my patch. Or did you refer to a different spinlock?

The local_bh_disable() is called earlier in dev_queue_xmit(), and is held across the whole HARD_TX_LOCK/dev_queue_xmit_nit/ hard_start_xmit/HARD_TX_UNLOCK sequence.

-Tommy

maintainer
#8 lore raw
Quoted 6 lines · On Tue, 30 Nov 2004 12:56:30 +0100 Tommy Christensen…
Quoted 2 lines

i.e. there's no local_bh_disable at all - adding the local_bh_disable was the whole point of my patch. Or did you refer to a different spinlock?

The local_bh_disable() is called earlier in dev_queue_xmit(), and is held across the whole HARD_TX_LOCK/dev_queue_xmit_nit/ hard_start_xmit/HARD_TX_UNLOCK sequence.

He's right. There must be something else happening in your tree Olaf.