[PATCH] neigh: error out pending skbs if netlink invalidates incomplete neigh

[PATCH] neigh: error out pending skbs if netlink invalidates incomplete neigh

[Timo Teras]

The state transition code from incomplete to invalid via neigh_update()
is missing the proper clean up of skb queue. Separate the clean up
code from neigh_timer_handler() to a new function and make neigh_update()
is it also.

Signed-off-by: Timo Teras <timo.teras@iki.fi>
---
 net/core/neighbour.c |   46 ++++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index a1cbce7..a17cb9d 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -771,6 +771,28 @@ static __inline__ int neigh_max_probes(struct neighbour *n)
 		p->ucast_probes + p->app_probes + p->mcast_probes);
 }
 
+static void neigh_invalidate(struct neighbour *neigh)
+{
+	struct sk_buff *skb;
+
+	NEIGH_CACHE_STAT_INC(neigh->tbl, res_failed);
+	NEIGH_PRINTK2("neigh %p is failed.\n", neigh);
+	neigh->updated = jiffies;
+
+	/* It is very thin place. report_unreachable is very complicated
+	   routine. Particularly, it can hit the same neighbour entry!
+
+	   So that, we try to be accurate and avoid dead loop. --ANK
+	 */
+	while (neigh->nud_state == NUD_FAILED &&
+	       (skb = __skb_dequeue(&neigh->arp_queue)) != NULL) {
+		write_unlock(&neigh->lock);
+		neigh->ops->error_report(neigh, skb);
+		write_lock(&neigh->lock);
+	}
+	skb_queue_purge(&neigh->arp_queue);
+}
+
 /* Called when a timer expires for a neighbour entry. */
 
 static void neigh_timer_handler(unsigned long arg)
@@ -835,26 +857,9 @@ static void neigh_timer_handler(unsigned long arg)
 
 	if ((neigh->nud_state & (NUD_INCOMPLETE | NUD_PROBE)) &&
 	    atomic_read(&neigh->probes) >= neigh_max_probes(neigh)) {
-		struct sk_buff *skb;
-
 		neigh->nud_state = NUD_FAILED;
-		neigh->updated = jiffies;
 		notify = 1;
-		NEIGH_CACHE_STAT_INC(neigh->tbl, res_failed);
-		NEIGH_PRINTK2("neigh %p is failed.\n", neigh);
-
-		/* It is very thin place. report_unreachable is very complicated
-		   routine. Particularly, it can hit the same neighbour entry!
-
-		   So that, we try to be accurate and avoid dead loop. --ANK
-		 */
-		while (neigh->nud_state == NUD_FAILED &&
-		       (skb = __skb_dequeue(&neigh->arp_queue)) != NULL) {
-			write_unlock(&neigh->lock);
-			neigh->ops->error_report(neigh, skb);
-			write_lock(&neigh->lock);
-		}
-		skb_queue_purge(&neigh->arp_queue);
+		neigh_invalidate(neigh);
 	}
 
 	if (neigh->nud_state & NUD_IN_TIMER) {
@@ -1001,6 +1006,11 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 		neigh->nud_state = new;
 		err = 0;
 		notify = old & NUD_VALID;
+		if ((old & (NUD_INCOMPLETE | NUD_PROBE)) &&
+		    (new & NUD_FAILED)) {
+			neigh_invalidate(neigh);
+			notify = 1;
+		}
 		goto out;
 	}
 
-- 
1.6.0.4

Re: [PATCH] neigh: error out pending skbs if netlink invalidates incomplete neigh

[David Miller]

From: Timo Teras <timo.teras@iki.fi>
Date: Thu,  4 Jun 2009 11:40:55 +0300

> The state transition code from incomplete to invalid via neigh_update()
> is missing the proper clean up of skb queue. Separate the clean up
> code from neigh_timer_handler() to a new function and make neigh_update()
> is it also.
> 
> Signed-off-by: Timo Teras <timo.teras@iki.fi>

We don't error out pending SKBs on the state transition alone, we do
it when we have the state transition _AND_ the number of probes
exceeds the limit.

And it seems that behavior is very much intentional.

I'm not going to apply this.

Re: [PATCH] neigh: error out pending skbs if netlink invalidates incomplete neigh

[Timo Teräs]

David Miller wrote:
> From: Timo Teras <timo.teras@iki.fi>
> Date: Thu,  4 Jun 2009 11:40:55 +0300
> 
>> The state transition code from incomplete to invalid via neigh_update()
>> is missing the proper clean up of skb queue. Separate the clean up
>> code from neigh_timer_handler() to a new function and make neigh_update()
>> is it also.
>>
>> Signed-off-by: Timo Teras <timo.teras@iki.fi>
> 
> We don't error out pending SKBs on the state transition alone, we do
> it when we have the state transition _AND_ the number of probes
> exceeds the limit.
> 
> And it seems that behavior is very much intentional.
> 
> I'm not going to apply this.

When the state is changed to FAILED by netlink, the timer is stopped
and further probes are not tried anymore. This results in that the
skb's are not errored. The neigh entry is left stale.

The behaviour is flawed currently. Either we need to prevent state
change from INCOMPLETE -> FAILED via netlink and keep on sending probes,
or flush the queue when userland replies with FAILED. Otherwise the
neigh entry state is left bad.

- Timo

Re: [PATCH] neigh: error out pending skbs if netlink invalidates incomplete neigh

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Thu, 11 Jun 2009 13:08:01 +0300

> When the state is changed to FAILED by netlink, the timer is stopped
> and further probes are not tried anymore. This results in that the
> skb's are not errored. The neigh entry is left stale.
> 
> The behaviour is flawed currently. Either we need to prevent state
> change from INCOMPLETE -> FAILED via netlink and keep on sending
> probes,
> or flush the queue when userland replies with FAILED. Otherwise the
> neigh entry state is left bad.

Thanks for the explanation, yes this is a problem.

Can you add a little bit more verbosity to your commit message
so that this issue is clearly explained and resubmit your patch?

Thank you!

[PATCH] neigh: fix state transition INCOMPLETE->FAILED via Netlink request

[Timo Teras]

The current code errors out the INCOMPLETE neigh entry skb queue only from
the timer if maximum probes have been attempted and there has been no reply.
This also causes the transtion to FAILED state.

However, the neigh entry can be also updated via Netlink to inform that the
address is unavailable.  Currently, neigh_update() just stops the timers and
leaves the pending skb's unreleased. This results that the clean up code in
the timer callback is never called, preventing also proper garbage collection.

This fixes neigh_update() to process the pending skb queue immediately if
INCOMPLETE -> FAILED state transtion occurs due to a Netlink request.

Signed-off-by: Timo Teras <timo.teras@iki.fi>
---
 net/core/neighbour.c |   46 ++++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index a1cbce7..a17cb9d 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -771,6 +771,28 @@ static __inline__ int neigh_max_probes(struct neighbour *n)
 		p->ucast_probes + p->app_probes + p->mcast_probes);
 }
 
+static void neigh_invalidate(struct neighbour *neigh)
+{
+	struct sk_buff *skb;
+
+	NEIGH_CACHE_STAT_INC(neigh->tbl, res_failed);
+	NEIGH_PRINTK2("neigh %p is failed.\n", neigh);
+	neigh->updated = jiffies;
+
+	/* It is very thin place. report_unreachable is very complicated
+	   routine. Particularly, it can hit the same neighbour entry!
+
+	   So that, we try to be accurate and avoid dead loop. --ANK
+	 */
+	while (neigh->nud_state == NUD_FAILED &&
+	       (skb = __skb_dequeue(&neigh->arp_queue)) != NULL) {
+		write_unlock(&neigh->lock);
+		neigh->ops->error_report(neigh, skb);
+		write_lock(&neigh->lock);
+	}
+	skb_queue_purge(&neigh->arp_queue);
+}
+
 /* Called when a timer expires for a neighbour entry. */
 
 static void neigh_timer_handler(unsigned long arg)
@@ -835,26 +857,9 @@ static void neigh_timer_handler(unsigned long arg)
 
 	if ((neigh->nud_state & (NUD_INCOMPLETE | NUD_PROBE)) &&
 	    atomic_read(&neigh->probes) >= neigh_max_probes(neigh)) {
-		struct sk_buff *skb;
-
 		neigh->nud_state = NUD_FAILED;
-		neigh->updated = jiffies;
 		notify = 1;
-		NEIGH_CACHE_STAT_INC(neigh->tbl, res_failed);
-		NEIGH_PRINTK2("neigh %p is failed.\n", neigh);
-
-		/* It is very thin place. report_unreachable is very complicated
-		   routine. Particularly, it can hit the same neighbour entry!
-
-		   So that, we try to be accurate and avoid dead loop. --ANK
-		 */
-		while (neigh->nud_state == NUD_FAILED &&
-		       (skb = __skb_dequeue(&neigh->arp_queue)) != NULL) {
-			write_unlock(&neigh->lock);
-			neigh->ops->error_report(neigh, skb);
-			write_lock(&neigh->lock);
-		}
-		skb_queue_purge(&neigh->arp_queue);
+		neigh_invalidate(neigh);
 	}
 
 	if (neigh->nud_state & NUD_IN_TIMER) {
@@ -1001,6 +1006,11 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 		neigh->nud_state = new;
 		err = 0;
 		notify = old & NUD_VALID;
+		if ((old & (NUD_INCOMPLETE | NUD_PROBE)) &&
+		    (new & NUD_FAILED)) {
+			neigh_invalidate(neigh);
+			notify = 1;
+		}
 		goto out;
 	}
 
-- 
1.6.0.4

Re: [PATCH] neigh: fix state transition INCOMPLETE->FAILED via Netlink request

[David Miller]

From: Timo Teras <timo.teras@iki.fi>
Date: Thu, 11 Jun 2009 13:32:25 +0300

> The current code errors out the INCOMPLETE neigh entry skb queue only from
> the timer if maximum probes have been attempted and there has been no reply.
> This also causes the transtion to FAILED state.
> 
> However, the neigh entry can be also updated via Netlink to inform that the
> address is unavailable.  Currently, neigh_update() just stops the timers and
> leaves the pending skb's unreleased. This results that the clean up code in
> the timer callback is never called, preventing also proper garbage collection.
> 
> This fixes neigh_update() to process the pending skb queue immediately if
> INCOMPLETE -> FAILED state transtion occurs due to a Netlink request.
> 
> Signed-off-by: Timo Teras <timo.teras@iki.fi>

Looks great, applied, thanks!