[PATCH] fix potential wild pointer when NIC is dying

[PATCH] fix potential wild pointer when NIC is dying

[Changli Gao]

fix potential wild pointer when NIC is dying.

flush_backlog() works with the assumption: the NIC doesn't enqueue packets to
linux kernel, so there are two places, which packets are in, softnet queue or
being processed in net-rx softirq. flush_backlog() is used to drop the first
kind of packets, and for the later, a grace period is used to wait the
finishing of the packets processing.

It always works without RPS. If RPS is used, although the NIC doesn't enqueue
packets to linux kernel, RPS may do. There may be condition, a grace period has
passed due to softirq running time limit, there are still packets, which refer
to the died NIC, and are enqueued by RPS after flush_backlog() returns.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 net/core/dev.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index a10a216..fe4a821 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -131,6 +131,7 @@
 #include <linux/random.h>
 #include <trace/events/napi.h>
 #include <linux/pci.h>
+#include <linux/stop_machine.h>
 
 #include "net-sysfs.h"
 
@@ -2791,19 +2792,24 @@ int netif_receive_skb(struct sk_buff *skb)
 EXPORT_SYMBOL(netif_receive_skb);
 
 /* Network device is going away, flush any packets still pending  */
-static void flush_backlog(void *arg)
+static int flush_backlog(void *arg)
 {
 	struct net_device *dev = arg;
-	struct softnet_data *queue = &__get_cpu_var(softnet_data);
 	struct sk_buff *skb, *tmp;
+	struct softnet_data *queue;
+	int cpu;
 
-	rps_lock(queue);
-	skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp)
-		if (skb->dev == dev) {
-			__skb_unlink(skb, &queue->input_pkt_queue);
-			kfree_skb(skb);
+	for_each_online_cpu(cpu) {
+		queue = &per_cpu(softnet_data, cpu);
+		skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) {
+			if (skb->dev == dev) {
+				__skb_unlink(skb, &queue->input_pkt_queue);
+				kfree_skb(skb);
+			}
 		}
-	rps_unlock(queue);
+	}
+
+	return 0;
 }
 
 static int napi_gro_complete(struct sk_buff *skb)
@@ -5027,7 +5033,7 @@ void netdev_run_todo(void)
 
 		dev->reg_state = NETREG_UNREGISTERED;
 
-		on_each_cpu(flush_backlog, dev, 1);
+		stop_machine(flush_backlog, dev, NULL);
 
 		netdev_wait_allrefs(dev);
 

Re: [PATCH] fix potential wild pointer when NIC is dying

[Joe Perches]

On Wed, 2010-04-14 at 20:18 +0800, Changli Gao wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a10a216..fe4a821 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[]
> -static void flush_backlog(void *arg)
> +static int flush_backlog(void *arg)

Why change this to return int?

> +	return 0;

It seems to always return 0.

Re: [PATCH] fix potential wild pointer when NIC is dying

[Changli Gao]

On Wed, Apr 14, 2010 at 12:23 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2010-04-14 at 20:18 +0800, Changli Gao wrote:
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index a10a216..fe4a821 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
> []
>> -static void flush_backlog(void *arg)
>> +static int flush_backlog(void *arg)
>
> Why change this to return int?
>
>> +     return 0;
>
> It seems to always return 0.
>
>

Keep stop_machine() happy.
int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);

-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] fix potential wild pointer when NIC is dying

[Eric Dumazet]

Le mercredi 14 avril 2010 à 20:18 +0800, Changli Gao a écrit :
> fix potential wild pointer when NIC is dying.
> 
> flush_backlog() works with the assumption: the NIC doesn't enqueue packets to
> linux kernel, so there are two places, which packets are in, softnet queue or
> being processed in net-rx softirq. flush_backlog() is used to drop the first
> kind of packets, and for the later, a grace period is used to wait the
> finishing of the packets processing.
> 
> It always works without RPS. If RPS is used, although the NIC doesn't enqueue
> packets to linux kernel, RPS may do. There may be condition, a grace period has
> passed due to softirq running time limit, there are still packets, which refer
> to the died NIC, and are enqueued by RPS after flush_backlog() returns.
> 

I dont see how the problem can happens, and how RPS is involved.

Did you got a single panic, could you provide us a stack trace ?

Maybe are you referring to NAPI ?

NAPI process packets delivered by NIC, and through RPS deliver it to a
(possibly) remote CPU queue.

But at device dismantle time, we should stop NAPI on this device and
packet delivery machinery. RPS being on or not, NAPI wont deliver new
packets. The fact that NAPI can be throtled doesnt change the napi
instance being disabled at this point. No more packet will be delivered
(RPS or not)

Only after this point we call flush_backlog() to make sure we dont have
any queued packet in each cpu input_pkt_queue pointing to the device we
dismantle.

RPS doesnt change this at all.

Hmm ???



> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
>  net/core/dev.c |   24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a10a216..fe4a821 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -131,6 +131,7 @@
>  #include <linux/random.h>
>  #include <trace/events/napi.h>
>  #include <linux/pci.h>
> +#include <linux/stop_machine.h>
>  
>  #include "net-sysfs.h"
>  
> @@ -2791,19 +2792,24 @@ int netif_receive_skb(struct sk_buff *skb)
>  EXPORT_SYMBOL(netif_receive_skb);
>  
>  /* Network device is going away, flush any packets still pending  */
> -static void flush_backlog(void *arg)
> +static int flush_backlog(void *arg)
>  {
>  	struct net_device *dev = arg;
> -	struct softnet_data *queue = &__get_cpu_var(softnet_data);
>  	struct sk_buff *skb, *tmp;
> +	struct softnet_data *queue;
> +	int cpu;
>  
> -	rps_lock(queue);
> -	skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp)
> -		if (skb->dev == dev) {
> -			__skb_unlink(skb, &queue->input_pkt_queue);
> -			kfree_skb(skb);
> +	for_each_online_cpu(cpu) {
> +		queue = &per_cpu(softnet_data, cpu);
> +		skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) {
> +			if (skb->dev == dev) {
> +				__skb_unlink(skb, &queue->input_pkt_queue);
> +				kfree_skb(skb);
> +			}
>  		}
> -	rps_unlock(queue);
> +	}
> +
> +	return 0;
>  }
>  
>  static int napi_gro_complete(struct sk_buff *skb)
> @@ -5027,7 +5033,7 @@ void netdev_run_todo(void)
>  
>  		dev->reg_state = NETREG_UNREGISTERED;
>  
> -		on_each_cpu(flush_backlog, dev, 1);
> +		stop_machine(flush_backlog, dev, NULL);
>  
>  		netdev_wait_allrefs(dev);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] fix potential wild pointer when NIC is dying

[Changli Gao]

On Wed, Apr 14, 2010 at 1:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mercredi 14 avril 2010 à 20:18 +0800, Changli Gao a écrit :
>
> I dont see how the problem can happens, and how RPS is involved.
>
> Did you got a single panic, could you provide us a stack trace ?
>
> Maybe are you referring to NAPI ?
>
> NAPI process packets delivered by NIC, and through RPS deliver it to a
> (possibly) remote CPU queue.
>
> But at device dismantle time, we should stop NAPI on this device and
> packet delivery machinery. RPS being on or not, NAPI wont deliver new
> packets. The fact that NAPI can be throtled doesnt change the napi
> instance being disabled at this point. No more packet will be delivered
> (RPS or not)
>
> Only after this point we call flush_backlog() to make sure we dont have
> any queued packet in each cpu input_pkt_queue pointing to the device we
> dismantle.
>
> RPS doesnt change this at all.
>
> Hmm ???
>

Thanks, I got it.

-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] fix potential wild pointer when NIC is dying

[Eric Dumazet]

Le mercredi 14 avril 2010 à 15:25 +0800, Changli Gao a écrit :

> 
> Thanks, I got it.
> 

No problem, its better to double check anyway :)