Skip to contentSkip to conversation

net: avoid race between netpoll and network fast path

PATCH In review Tina Yang 3 participants 5 messages Oct 17, 2007 – Oct 30, 2007

Subsystems (2)
lore
Share & tools
lore

Tina Yang

author
#0 lore raw

PATCH 1/1 net: avoid race between netpoll and network fast path

The current netpoll design and implementation has serveral race issues with the network fast path that panics/hangs the system or causes interface timeout/reset but the fix is likely to have impact on the overall system performance and could involve a large number of drivers. The proposal is to disable the problem code for normal operations but only to enable it at the time of crash in case polling is necessary. Tests that have been done included the bug fix verification as well as regression check on the netlog results in various crash modes.

---

include/linux/kernel.h renamed from include/linux/kernel.h.orig +2 -0
@@ -184,6 +184,8 @@ static inline void console_verbose(void)
 		console_loglevel = 15;
 }
 
+extern int netpoll_enable;
+
 extern void bust_spinlocks(int yes);
 extern void wake_up_klogd(void);
 extern int oops_in_progress;		/* If set, an oops, panic(), BUG() or die() is in progress */
net/core/netpoll.c renamed from net/core/netpoll.c.orig +7 -3
@@ -150,15 +150,19 @@ static void service_arp_queue(struct net
 	}
 }
 
+int netpoll_enable;
+EXPORT_SYMBOL(netpoll_enable);
 void netpoll_poll(struct netpoll *np)
 {
 	if (!np->dev || !netif_running(np->dev) || !np->dev->poll_controller)
 		return;
 
 	/* Process pending work on NIC */
-	np->dev->poll_controller(np->dev);
-	if (np->dev->poll)
-		poll_napi(np);
+	if (unlikely(netpoll_enable)) {
+		np->dev->poll_controller(np->dev);
+		if (np->dev->poll)
+			poll_napi(np);
+	}
 
 	service_arp_queue(np->dev->npinfo);
 
kernel/panic.c renamed from kernel/panic.c.orig +1 -0
@@ -66,6 +66,7 @@ NORET_TYPE void panic(const char * fmt,
         unsigned long caller = (unsigned long) __builtin_return_address(0);
 #endif
 
+	netpoll_enable = 1;
 	/*
 	 * It's possible to come here directly from a panic-assertion and not
 	 * have preempt disabled. Some functions called from here want
arch/x86_64/kernel/traps.c renamed from arch/x86_64/kernel/traps.c.orig +2 -0
@@ -522,6 +522,8 @@ void __kprobes __die(const char * str, s
 #endif
 	printk("\n");
 	notify_die(DIE_OOPS, str, regs, err, current->thread.trap_no, SIGSEGV);
+	if (kexec_should_crash(current))
+		netpoll_enable = 1;
 	show_registers(regs);
 	add_taint(TAINT_DIE);
 	/* Executive summary in case the oops scrolled away */
arch/i386/kernel/traps.c renamed from arch/i386/kernel/traps.c.orig +2 -0
@@ -428,6 +428,8 @@ void die(const char * str, struct pt_reg
 		if (notify_die(DIE_OOPS, str, regs, err,
 					current->thread.trap_no, SIGSEGV) !=
 				NOTIFY_STOP) {
+			if (kexec_should_crash(current))
+				netpoll_enable = 1;
 			show_registers(regs);
 			/* Executive summary in case the oops scrolled away */
 			esp = (unsigned long) (&regs->esp);
maintainer
#1 lore raw

From: Tina Yang <[email protected]>
Date: Tue, 16 Oct 2007 20:45:04 -0700

Quoted 9 lines

The current netpoll design and implementation has serveral race issues with the network fast path that panics/hangs the system or causes interface timeout/reset but the fix is likely to have impact on the overall system performance and could involve a large number of drivers. The proposal is to disable the problem code for normal operations but only to enable it at the time of crash in case polling is necessary. Tests that have been done included the bug fix verification as well as regression check on the netlog results in various crash modes.

This is at best a kludge, and it's the wrong way to approach this problem.

Fix the bug, and fix it right.

If you disable that stretch of code, what you've done is make the netpoll code hang and/or drop console messages if the TX queue is full in the driver and the only way to liberate TX space is to call into ->poll().

You haven't shown the precise race that leads to corruption so that someone so motivated can guide you towards a more correct fix if you are not capable of implementing it properly on your own.

author
#2 lore raw
Quoted 15 lines · David Miller

From: Tina Yang <[email protected]>
Date: Tue, 16 Oct 2007 20:45:04 -0700

Quoted 9 lines

The current netpoll design and implementation has serveral race issues with the network fast path that panics/hangs the system or causes interface timeout/reset but the fix is likely to have impact on the overall system performance and could involve a large number of drivers. The proposal is to disable the problem code for normal operations but only to enable it at the time of crash in case polling is necessary. Tests that have been done included the bug fix verification as well as regression check on the netlog results in various crash modes.

This is at best a kludge, and it's the wrong way to approach this problem.

Quoted 7 lines

Fix the bug, and fix it right.

If you disable that stretch of code, what you've done is make the netpoll code hang and/or drop console messages if the TX queue is full in the driver and the only way to liberate TX space is to call into ->poll().

Isn't net_rx_action() calling ->poll() to free the TX space ? TX queue full can only be emptied when the device is done transmitting not because of netpoll ->poll() it. The softirq (net_rx_action) is the purpose for such an event. Netconsole messages will be dropped if the device can't keep up with it regardless of netpoll ->poll() or not. If no dropping can be tolerated, then the netpoll upper layer probably should be redesigned to buffer the data.

The poll_list currently is in a per_cpu structure, not being protected globally that netpoll thread from any cpu can trash it.

Quoted 4 lines

You haven't shown the precise race that leads to corruption so that someone so motivated can guide you towards a more correct fix if you are not capable of implementing it properly on your own.

The precise race is
1) net_rx_action get the dev from poll_list
2) at the same time, netpoll poll_napi() get a hold of the poll lock and calls ->poll(), remove dev from the poll list
3) after it finishes, net_rx_action get the poll lock, and calls ->poll() the second time, and panic when trying to remove (again) the dev from the poll list.
and I had logged all the crash info from the crash scenes into the bug database.

As Matt Mackall had acknowledged, the network fast path went to great length to reduce locking overhead, should that be undone because of netpoll if that's what it takes to fix it more correctly ?

maintainer
#3 lore raw

From: Tina Yang <[email protected]>
Date: Tue, 16 Oct 2007 22:46:30 -0700

Quoted 7 lines

The precise race is
1) net_rx_action get the dev from poll_list
2) at the same time, netpoll poll_napi() get a hold of the poll lock and calls ->poll(), remove dev from the poll list
3) after it finishes, net_rx_action get the poll lock, and calls ->poll() the second time, and panic when trying to remove (again) the dev from the poll list.

This is trivial to fix.

I'll check the following into 2.6.14 and backport it to the -stable trees.

[NET]: Fix race between poll_napi() and net_rx_action()

netpoll_poll_lock() synchronizes the ->poll() invocation code paths, but once we have the lock we have to make sure that NAPI_STATE_SCHED is still set. Otherwise we get:

	cpu 0			cpu 1

	net_rx_action()		poll_napi()
	netpoll_poll_lock()	... spin on ->poll_lock
	->poll()
	  netif_rx_complete
	netpoll_poll_unlock()	acquire ->poll_lock()
				->poll()
				 netif_rx_complete()
				 CRASH

Based upon a bug report from Tina Yang.

net/core/dev.c +9 -1
index 853c8b5..02e7d83 100644
@@ -2172,7 +2172,15 @@ static void net_rx_action(struct softirq_action *h)
 
 		weight = n->weight;
 
-		work = n->poll(n, weight);
+		/* This NAPI_STATE_SCHED test is for avoiding a race
+		 * with netpoll's poll_napi().  Only the entity which
+		 * obtains the lock and sees NAPI_STATE_SCHED set will
+		 * actually make the ->poll() call.  Therefore we avoid
+		 * accidently calling ->poll() when NAPI is not scheduled.
+		 */
+		work = 0;
+		if (test_bit(NAPI_STATE_SCHED, &n->state))
+			work = n->poll(n, weight);
 
 		WARN_ON_ONCE(work > weight);
 
net/core/netpoll.c +28 -9
index bf8d18f..c499b5c 100644
@@ -116,6 +116,29 @@ static __sum16 checksum_udp(struct sk_buff *skb, struct udphdr *uh,
  * network adapter, forcing superfluous retries and possibly timeouts.
  * Thus, we set our budget to greater than 1.
  */
+static int poll_one_napi(struct netpoll_info *npinfo,
+			 struct napi_struct *napi, int budget)
+{
+	int work;
+
+	/* net_rx_action's ->poll() invocations and our's are
+	 * synchronized by this test which is only made while
+	 * holding the napi->poll_lock.
+	 */
+	if (!test_bit(NAPI_STATE_SCHED, &napi->state))
+		return budget;
+
+	npinfo->rx_flags |= NETPOLL_RX_DROP;
+	atomic_inc(&trapped);
+
+	work = napi->poll(napi, budget);
+
+	atomic_dec(&trapped);
+	npinfo->rx_flags &= ~NETPOLL_RX_DROP;
+
+	return budget - work;
+}
+
 static void poll_napi(struct netpoll *np)
 {
 	struct netpoll_info *npinfo = np->dev->npinfo;
@@ -123,17 +146,13 @@ static void poll_napi(struct netpoll *np)
 	int budget = 16;
 
 	list_for_each_entry(napi, &np->dev->napi_list, dev_list) {
-		if (test_bit(NAPI_STATE_SCHED, &napi->state) &&
-		    napi->poll_owner != smp_processor_id() &&
+		if (napi->poll_owner != smp_processor_id() &&
 		    spin_trylock(&napi->poll_lock)) {
-			npinfo->rx_flags |= NETPOLL_RX_DROP;
-			atomic_inc(&trapped);
-
-			napi->poll(napi, budget);
-
-			atomic_dec(&trapped);
-			npinfo->rx_flags &= ~NETPOLL_RX_DROP;
+			budget = poll_one_napi(npinfo, napi, budget);
 			spin_unlock(&napi->poll_lock);
+
+			if (!budget)
+				break;
 		}
 	}
 }
#4 lore raw
Quoted 34 lines · David Miller

From: Tina Yang <[email protected]>
Date: Tue, 16 Oct 2007 22:46:30 -0700

Quoted 7 lines

The precise race is
1) net_rx_action get the dev from poll_list
2) at the same time, netpoll poll_napi() get a hold of the poll lock and calls ->poll(), remove dev from the poll list
3) after it finishes, net_rx_action get the poll lock, and calls ->poll() the second time, and panic when trying to remove (again) the dev from the poll list.

This is trivial to fix.

I'll check the following into 2.6.14 and backport it to the -stable trees.

[NET]: Fix race between poll_napi() and net_rx_action()

netpoll_poll_lock() synchronizes the ->poll() invocation code paths, but once we have the lock we have to make sure that NAPI_STATE_SCHED is still set. Otherwise we get:

	cpu 0			cpu 1

	net_rx_action()		poll_napi()
	netpoll_poll_lock()	... spin on ->poll_lock
	->poll()
	  netif_rx_complete
	netpoll_poll_unlock()	acquire ->poll_lock()
				->poll()
				 netif_rx_complete()
				 CRASH
Quoted 3 lines

Based upon a bug report from Tina Yang.

Thanks, Dave.

Signature · 1 line
Mathematics is the supreme nostalgia of our time.