aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNeil Horman <nhorman@tuxdriver.com>2008-12-10 02:22:26 -0500
committerDavid S. Miller <davem@davemloft.net>2008-12-10 02:22:26 -0500
commit7b363e440021a1cf9ed76944b2685f48dacefb3e (patch)
tree973e674ab7bfd29807075316489f357327fcf2e8
parent24fc7b86dc0470616803be2f921c8cd5c459175d (diff)
netpoll: fix race on poll_list resulting in garbage entry
A few months back a race was discused between the netpoll napi service path, and the fast path through net_rx_action: http://kerneltrap.org/mailarchive/linux-netdev/2007/10/16/345470 A patch was submitted for that bug, but I think we missed a case. Consider the following scenario: INITIAL STATE CPU0 has one napi_struct A on its poll_list CPU1 is calling netpoll_send_skb and needs to call poll_napi on the same napi_struct A that CPU0 has on its list CPU0 CPU1 net_rx_action poll_napi !list_empty (returns true) locks poll_lock for A poll_one_napi napi->poll netif_rx_complete __napi_complete (removes A from poll_list) list_entry(list->next) In the above scenario, net_rx_action assumes that the per-cpu poll_list is exclusive to that cpu. netpoll of course violates that, and because the netpoll path can dequeue from the poll list, its possible for CPU0 to detect a non-empty list at the top of the while loop in net_rx_action, but have it become empty by the time it calls list_entry. Since the poll_list isn't surrounded by any other structure, the returned data from that list_entry call in this situation is garbage, and any number of crashes can result based on what exactly that garbage is. Given that its not fasible for performance reasons to place exclusive locks arround each cpus poll list to provide that mutal exclusion, I think the best solution is modify the netpoll path in such a way that we continue to guarantee that the poll_list for a cpu is in fact exclusive to that cpu. To do this I've implemented the patch below. It adds an additional bit to the state field in the napi_struct. When executing napi->poll from the netpoll_path, this bit will be set. When a driver calls netif_rx_complete, if that bit is set, it will not remove the napi_struct from the poll_list. That work will be saved for the next iteration of net_rx_action. I've tested this and it seems to work well. About the biggest drawback I can see to it is the fact that it might result in an extra loop through net_rx_action in the event that the device is actually contended for (i.e. the netpoll path actually preforms all the needed work no the device, and the call to net_rx_action winds up doing nothing, except removing the napi_struct from the poll_list. However I think this is probably a small price to pay, given that the alternative is a crash. Signed-off-by: Neil Horman <nhorman@tuxdriver.com> Signed-off-by: David S. Miller <davem@davemloft.net>
-rw-r--r--include/linux/netdevice.h7
-rw-r--r--net/core/netpoll.c2
2 files changed, 9 insertions, 0 deletions
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9d77b1d7dca8..e26f54952892 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -319,6 +319,7 @@ enum
319{ 319{
320 NAPI_STATE_SCHED, /* Poll is scheduled */ 320 NAPI_STATE_SCHED, /* Poll is scheduled */
321 NAPI_STATE_DISABLE, /* Disable pending */ 321 NAPI_STATE_DISABLE, /* Disable pending */
322 NAPI_STATE_NPSVC, /* Netpoll - don't dequeue from poll_list */
322}; 323};
323 324
324extern void __napi_schedule(struct napi_struct *n); 325extern void __napi_schedule(struct napi_struct *n);
@@ -1497,6 +1498,12 @@ static inline void netif_rx_complete(struct net_device *dev,
1497{ 1498{
1498 unsigned long flags; 1499 unsigned long flags;
1499 1500
1501 /*
1502 * don't let napi dequeue from the cpu poll list
1503 * just in case its running on a different cpu
1504 */
1505 if (unlikely(test_bit(NAPI_STATE_NPSVC, &napi->state)))
1506 return;
1500 local_irq_save(flags); 1507 local_irq_save(flags);
1501 __netif_rx_complete(dev, napi); 1508 __netif_rx_complete(dev, napi);
1502 local_irq_restore(flags); 1509 local_irq_restore(flags);
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 6c7af390be0a..dadac6281f20 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -133,9 +133,11 @@ static int poll_one_napi(struct netpoll_info *npinfo,
133 133
134 npinfo->rx_flags |= NETPOLL_RX_DROP; 134 npinfo->rx_flags |= NETPOLL_RX_DROP;
135 atomic_inc(&trapped); 135 atomic_inc(&trapped);
136 set_bit(NAPI_STATE_NPSVC, &napi->state);
136 137
137 work = napi->poll(napi, budget); 138 work = napi->poll(napi, budget);
138 139
140 clear_bit(NAPI_STATE_NPSVC, &napi->state);
139 atomic_dec(&trapped); 141 atomic_dec(&trapped);
140 npinfo->rx_flags &= ~NETPOLL_RX_DROP; 142 npinfo->rx_flags &= ~NETPOLL_RX_DROP;
141 143