Skip to contentSkip to conversation

net: fix race in process_backlog

PATCH In review Peter Zijlstra 3 participants 5 messages Oct 3, 2007

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

Peter Zijlstra

author
#0 lore raw

PATCH 1/1 net: fix race in process_backlog

Subject: net: fix race in process_backlog

The recent NAPI rework (4fa57c9ea9f36f9ca852f3a88ca5d2f1aebbc960) introduced a race between netif_rx() and process_backlog() which resulted in softirq processing to drop dead.

netif_rx() process_backlog()

			irq_disable();
			skb = __skb_dequeue();
			irq_enable();

irq_disable();
__skb_queue_tail();
napi_schedule();
irq_enable();

			if (!skb)
			  napi_complete();  <-- oops!

we cleared the napi bit, even though there is data to process.

Signed-off-by Peter Zijlstra <[email protected]>
---
 net/core/dev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/core/dev.c +2 -2
Index: linux-2.6/net/core/dev.c
===================================================================
@@ -2095,11 +2095,11 @@ static int process_backlog(struct napi_s
 
 		local_irq_disable();
 		skb = __skb_dequeue(&queue->input_pkt_queue);
-		local_irq_enable();
 		if (!skb) {
-			napi_complete(napi);
+			__napi_complete(napi);
 			break;
 		}
+		local_irq_enable();
 
 		dev = skb->dev;
Acked-by on v1 1/1
#1 lore raw
Quoted 17 lines · On Wed, 03 Oct 2007 17:44:53 +0200 Peter Zijlstra <[email protected]>…

Subject: net: fix race in process_backlog

The recent NAPI rework (4fa57c9ea9f36f9ca852f3a88ca5d2f1aebbc960) introduced a race between netif_rx() and process_backlog() which resulted in softirq processing to drop dead.

netif_rx() process_backlog()

			irq_disable();
			skb = __skb_dequeue();
			irq_enable();

irq_disable();
__skb_queue_tail();
napi_schedule();
irq_enable();

Quoted 6 lines
			if (!skb)
			  napi_complete();  <-- oops!

we cleared the napi bit, even though there is data to process.

Signed-off-by Peter Zijlstra <[email protected]>
Acked-by Stephen Hemminger <[email protected]>
Signature · 1 line
Stephen Hemminger <[email protected]>
maintainer
#2 lore raw
In reply to Peter Zijlstra

From: Peter Zijlstra <[email protected]>
Date: Wed, 03 Oct 2007 17:44:53 +0200

Quoted 5 lines

Index: linux-2.6/net/core/dev.c
===================================================================

net/core/dev.c static int process_backlog(struct napi_s · line 2102

Quoted 10 lines
net/core/dev.c +2 -2
@@ -2095,11 +2095,11 @@ static int process_backlog(struct napi_s
 
 		local_irq_disable();
 		skb = __skb_dequeue(&queue->input_pkt_queue);
-		local_irq_enable();
 		if (!skb) {
-			napi_complete(napi);
+			__napi_complete(napi);
 			break;
 		}
+		local_irq_enable();

What re-enables interrupts in the !skb path?

#3 lore raw
Quoted 9 lines · On Wed, 03 Oct 2007 14:58:07 -0700 (PDT) David Miller <[email protected]>…

From: Peter Zijlstra <[email protected]>
Date: Wed, 03 Oct 2007 17:44:53 +0200

Quoted 6 lines

Index: linux-2.6/net/core/dev.c
===================================================================

net/core/dev.c +0 -0
@@ -2095,11 +2095,11 @@ static int process_backlog(struct napi_s
Quoted 11 lines

net/core/dev.c static int process_backlog(struct napi_s · line 2102

Quoted 9 lines
net/core/dev.c +2 -2
 		local_irq_disable();
 		skb = __skb_dequeue(&queue->input_pkt_queue);
-		local_irq_enable();
 		if (!skb) {
-			napi_complete(napi);
+			__napi_complete(napi);
 			break;
 		}
+		local_irq_enable();

What re-enables interrupts in the !skb path?

This looks like a better fix. the irq_enable is needed in both cases.

net/core/dev.c +4 -2
@@ -2077,12 +2077,14 @@ static int process_backlog(struct napi_s
 
 		local_irq_disable();
 		skb = __skb_dequeue(&queue->input_pkt_queue);
-		local_irq_enable();
 		if (!skb) {
-			napi_complete(napi);
+			__napi_complete(napi);
+			local_irq_enable();
 			break;
 		}
 
+		local_irq_enable();
+
 		dev = skb->dev;
 
 		netif_receive_skb(skb);
Signature · 1 line
Stephen Hemminger <[email protected]>
maintainer
#4 lore raw

From: Stephen Hemminger <[email protected]>
Date: Wed, 3 Oct 2007 15:05:19 -0700

Quoted 24 lines
Quoted 9 lines · On Wed, 03 Oct 2007 14:58:07 -0700 (PDT) David Miller <[email protected]>…

From: Peter Zijlstra <[email protected]>
Date: Wed, 03 Oct 2007 17:44:53 +0200

Quoted 6 lines

Index: linux-2.6/net/core/dev.c
===================================================================

net/core/dev.c +0 -0
@@ -2095,11 +2095,11 @@ static int process_backlog(struct napi_s
Quoted 11 lines

net/core/dev.c static int process_backlog(struct napi_s · line 2086

Quoted 9 lines
net/core/dev.c +2 -2
 		local_irq_disable();
 		skb = __skb_dequeue(&queue->input_pkt_queue);
-		local_irq_enable();
 		if (!skb) {
-			napi_complete(napi);
+			__napi_complete(napi);
 			break;
 		}
+		local_irq_enable();

What re-enables interrupts in the !skb path?

Quoted 1 line

This looks like a better fix. the irq_enable is needed in both cases.

Yep, applied, thanks Peter and Stephen.