neigh: set nud_state to NUD_INCOMPLETE when probing router reachability

PATCH In review Duan Jiong 3 participants 9 messages May 9, 2014 – May 13, 2014

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Duan Jiong

author

2014-05-09 05:19 UTC #0 lore raw

PATCH v2 1/1 neigh: set nud_state to NUD_INCOMPLETE when probing router reachability

Since commit 7e98056964("ipv6: router reachability probing"), a router falls into NUD_FAILED will be probed.

Now if function rt6_select() selects a router which neighbour state is NUD_FAILED, and at the same time function rt6_probe() changes the neighbour state to NUD_PROBE, then function dst_neigh_output() can directly send packets, but actually the neighbour still is unreachable. If we set nud_state to NUD_INCOMPLETE instead NUD_PROBE, packets will not be sent out until the neihbour is reachable.

In addition, because the route should be probes with a single NS, so we must set neigh->probes to neigh_max_probes(), then the neigh timer timeout and function neigh_timer_handler() will not send other NS Messages.

Signed-off-by Duan Jiong <[email protected]>

---
Changes from v1:
*modify changelog to explain in detail why use neigh_max_probes().

 net/core/neighbour.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/core/neighbour.c +2 -2

index 8f8a96e..32d872e 100644
@@ -1248,8 +1248,8 @@ void __neigh_set_probe_once(struct neighbour *neigh)
 	neigh->updated = jiffies;
 	if (!(neigh->nud_state & NUD_FAILED))
 		return;
-	neigh->nud_state = NUD_PROBE;
-	atomic_set(&neigh->probes, NEIGH_VAR(neigh->parms, UCAST_PROBES));
+	neigh->nud_state = NUD_INCOMPLETE;
+	atomic_set(&neigh->probes, neigh_max_probes(neigh));
 	neigh_add_timer(neigh,
 			jiffies + NEIGH_VAR(neigh->parms, RETRANS_TIME));
 }

Signature · 1 line

1.8.3.1

Hannes Frederic Sowa

2014-05-11 22:04 UTC #1 lore raw

Quoted 27 lines · Duan Jiong

Since commit 7e98056964("ipv6: router reachability probing"), a router falls into NUD_FAILED will be probed.
Now if function rt6_select() selects a router which neighbour state is NUD_FAILED, and at the same time function rt6_probe() changes the neighbour state to NUD_PROBE, then function dst_neigh_output() can directly send packets, but actually the neighbour still is unreachable. If we set nud_state to NUD_INCOMPLETE instead NUD_PROBE, packets will not be sent out until the neihbour is reachable.
In addition, because the route should be probes with a single NS, so we must set neigh->probes to neigh_max_probes(), then the neigh timer timeout and function neigh_timer_handler() will not send other NS Messages.
Signed-off-by Duan Jiong <[email protected]>
---
Changes from v1:
*modify changelog to explain in detail why use neigh_max_probes().
 net/core/neighbour.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/core/neighbour.c +0 -0
index 8f8a96e..32d872e 100644

net/core/neighbour.c void __neigh_set_probe_once(struct neighbour *neigh) · line 1252

Quoted 7 lines

net/core/neighbour.c +2 -2

@@ -1248,8 +1248,8 @@ void __neigh_set_probe_once(struct neighbour *neigh)
 	neigh->updated = jiffies;
 	if (!(neigh->nud_state & NUD_FAILED))
 		return;
-	neigh->nud_state = NUD_PROBE;
-	atomic_set(&neigh->probes, NEIGH_VAR(neigh->parms, UCAST_PROBES));
+	neigh->nud_state = NUD_INCOMPLETE;
+	atomic_set(&neigh->probes, neigh_max_probes(neigh));

Wouldn't it be better if we neigh_suspect the neighbour and leaving the state in NUD_PROBE? We call down to ->output in case neighbour is in NUD_PROBE state, so we must just disable connected 'fast-path'.

Greetings,

Hannes

Duan Jiong

author

2014-05-12 03:28 UTC #2 lore raw

Quoted 23 lines · Hannes Frederic Sowa

Quoted 22 lines · Duan Jiong
Since commit 7e98056964("ipv6: router reachability probing"), a router falls into NUD_FAILED will be probed.
Now if function rt6_select() selects a router which neighbour state is NUD_FAILED, and at the same time function rt6_probe() changes the neighbour state to NUD_PROBE, then function dst_neigh_output() can directly send packets, but actually the neighbour still is unreachable. If we set nud_state to NUD_INCOMPLETE instead NUD_PROBE, packets will not be sent out until the neihbour is reachable.
In addition, because the route should be probes with a single NS, so we must set neigh->probes to neigh_max_probes(), then the neigh timer timeout and function neigh_timer_handler() will not send other NS Messages.
Signed-off-by Duan Jiong <[email protected]>
---
Changes from v1:
*modify changelog to explain in detail why use neigh_max_probes().
 net/core/neighbour.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Quoted 15 lines

net/core/neighbour.c void __neigh_set_probe_once(struct neighbour *neigh) · line 1252
Quoted 12 lines
net/core/neighbour.c +2 -2
index 8f8a96e..32d872e 100644
@@ -1248,8 +1248,8 @@ void __neigh_set_probe_once(struct neighbour *neigh)
 	neigh->updated = jiffies;
 	if (!(neigh->nud_state & NUD_FAILED))
 		return;
-	neigh->nud_state = NUD_PROBE;
-	atomic_set(&neigh->probes, NEIGH_VAR(neigh->parms, UCAST_PROBES));
+	neigh->nud_state = NUD_INCOMPLETE;
+	atomic_set(&neigh->probes, neigh_max_probes(neigh));
Wouldn't it be better if we neigh_suspect the neighbour and leaving the state in NUD_PROBE? We call down to ->output in case neighbour is in NUD_PROBE state, so we must just disable connected 'fast-path'.

You can look into neigh_event_send() called in neigh_resolve_output(), and if neigh->nud_state still is NUD_PROBE, the neigh_event_send() will return 0, so the packet will still be sent out without probe.

So, using neigh_suspect is not a good idea.

Thanks,
Duan

Quoted 5 lines

Greetings,
Hannes
.

David Miller

maintainer

2014-05-12 18:37 UTC #3 lore raw

From: Duan Jiong <[email protected]>
Date: Mon, 12 May 2014 11:26:10 +0800

Quoted 40 lines

Quoted 23 lines · Hannes Frederic Sowa
Quoted 22 lines · Duan Jiong
Since commit 7e98056964("ipv6: router reachability probing"), a router falls into NUD_FAILED will be probed.
Now if function rt6_select() selects a router which neighbour state is NUD_FAILED, and at the same time function rt6_probe() changes the neighbour state to NUD_PROBE, then function dst_neigh_output() can directly send packets, but actually the neighbour still is unreachable. If we set nud_state to NUD_INCOMPLETE instead NUD_PROBE, packets will not be sent out until the neihbour is reachable.
In addition, because the route should be probes with a single NS, so we must set neigh->probes to neigh_max_probes(), then the neigh timer timeout and function neigh_timer_handler() will not send other NS Messages.
Signed-off-by Duan Jiong <[email protected]>
---
Changes from v1:
*modify changelog to explain in detail why use neigh_max_probes().
 net/core/neighbour.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 15 lines
net/core/neighbour.c void __neigh_set_probe_once(struct neighbour *neigh) · line 1252
Quoted 12 lines
net/core/neighbour.c +2 -2
index 8f8a96e..32d872e 100644
@@ -1248,8 +1248,8 @@ void __neigh_set_probe_once(struct neighbour *neigh)
 	neigh->updated = jiffies;
 	if (!(neigh->nud_state & NUD_FAILED))
 		return;
-	neigh->nud_state = NUD_PROBE;
-	atomic_set(&neigh->probes, NEIGH_VAR(neigh->parms, UCAST_PROBES));
+	neigh->nud_state = NUD_INCOMPLETE;
+	atomic_set(&neigh->probes, neigh_max_probes(neigh));
Wouldn't it be better if we neigh_suspect the neighbour and leaving the state in NUD_PROBE? We call down to ->output in case neighbour is in NUD_PROBE state, so we must just disable connected 'fast-path'.

Quoted 5 lines

You can look into neigh_event_send() called in neigh_resolve_output(), and if neigh->nud_state still is NUD_PROBE, the neigh_event_send() will return 0, so the packet will still be sent out without probe.
So, using neigh_suspect is not a good idea.

If you set it to NUD_INCOMPLETE however, neigh_event_send() is going to add the packet to the neigh's ARP queue and return '1'.

Is that really what you want to happen in this case?

Duan Jiong

author

2014-05-13 01:09 UTC #4 lore raw

Quoted 47 lines · David Miller

From: Duan Jiong <[email protected]>
Date: Mon, 12 May 2014 11:26:10 +0800
Quoted 44 lines
Quoted 23 lines · Hannes Frederic Sowa
Quoted 22 lines · Duan Jiong
Since commit 7e98056964("ipv6: router reachability probing"), a router falls into NUD_FAILED will be probed.
Now if function rt6_select() selects a router which neighbour state is NUD_FAILED, and at the same time function rt6_probe() changes the neighbour state to NUD_PROBE, then function dst_neigh_output() can directly send packets, but actually the neighbour still is unreachable. If we set nud_state to NUD_INCOMPLETE instead NUD_PROBE, packets will not be sent out until the neihbour is reachable.
In addition, because the route should be probes with a single NS, so we must set neigh->probes to neigh_max_probes(), then the neigh timer timeout and function neigh_timer_handler() will not send other NS Messages.
Signed-off-by Duan Jiong <[email protected]>
---
Changes from v1:
*modify changelog to explain in detail why use neigh_max_probes().
 net/core/neighbour.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 15 lines
net/core/neighbour.c void __neigh_set_probe_once(struct neighbour *neigh) · line 1252
Quoted 12 lines
net/core/neighbour.c +2 -2
index 8f8a96e..32d872e 100644
@@ -1248,8 +1248,8 @@ void __neigh_set_probe_once(struct neighbour *neigh)
 	neigh->updated = jiffies;
 	if (!(neigh->nud_state & NUD_FAILED))
 		return;
-	neigh->nud_state = NUD_PROBE;
-	atomic_set(&neigh->probes, NEIGH_VAR(neigh->parms, UCAST_PROBES));
+	neigh->nud_state = NUD_INCOMPLETE;
+	atomic_set(&neigh->probes, neigh_max_probes(neigh));
Wouldn't it be better if we neigh_suspect the neighbour and leaving the state in NUD_PROBE? We call down to ->output in case neighbour is in NUD_PROBE state, so we must just disable connected 'fast-path'.
You can look into neigh_event_send() called in neigh_resolve_output(), and if neigh->nud_state still is NUD_PROBE, the neigh_event_send() will return 0, so the packet will still be sent out without probe.

Quoted 6 lines

Quoted 1 line
So, using neigh_suspect is not a good idea.
If you set it to NUD_INCOMPLETE however, neigh_event_send() is going to add the packet to the neigh's ARP queue and return '1'.
Is that really what you want to happen in this case?

Yes, packets should not be sent out until the neihbour is reachable.

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
.

David Miller

maintainer

2014-05-13 03:25 UTC #5 lore raw

From: Duan Jiong <[email protected]>
Date: Tue, 13 May 2014 09:07:12 +0800

Quoted 48 lines

Quoted 47 lines · David Miller
From: Duan Jiong <[email protected]>
Date: Mon, 12 May 2014 11:26:10 +0800
Quoted 44 lines
Quoted 23 lines · Hannes Frederic Sowa
Quoted 22 lines · Duan Jiong
Since commit 7e98056964("ipv6: router reachability probing"), a router falls into NUD_FAILED will be probed.
Now if function rt6_select() selects a router which neighbour state is NUD_FAILED, and at the same time function rt6_probe() changes the neighbour state to NUD_PROBE, then function dst_neigh_output() can directly send packets, but actually the neighbour still is unreachable. If we set nud_state to NUD_INCOMPLETE instead NUD_PROBE, packets will not be sent out until the neihbour is reachable.
In addition, because the route should be probes with a single NS, so we must set neigh->probes to neigh_max_probes(), then the neigh timer timeout and function neigh_timer_handler() will not send other NS Messages.
Signed-off-by Duan Jiong <[email protected]>
---
Changes from v1:
*modify changelog to explain in detail why use neigh_max_probes().
 net/core/neighbour.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 15 lines
net/core/neighbour.c void __neigh_set_probe_once(struct neighbour *neigh) · line 1252
Quoted 12 lines
net/core/neighbour.c +2 -2
index 8f8a96e..32d872e 100644
@@ -1248,8 +1248,8 @@ void __neigh_set_probe_once(struct neighbour *neigh)
 	neigh->updated = jiffies;
 	if (!(neigh->nud_state & NUD_FAILED))
 		return;
-	neigh->nud_state = NUD_PROBE;
-	atomic_set(&neigh->probes, NEIGH_VAR(neigh->parms, UCAST_PROBES));
+	neigh->nud_state = NUD_INCOMPLETE;
+	atomic_set(&neigh->probes, neigh_max_probes(neigh));
Wouldn't it be better if we neigh_suspect the neighbour and leaving the state in NUD_PROBE? We call down to ->output in case neighbour is in NUD_PROBE state, so we must just disable connected 'fast-path'.
You can look into neigh_event_send() called in neigh_resolve_output(), and if neigh->nud_state still is NUD_PROBE, the neigh_event_send() will return 0, so the packet will still be sent out without probe.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
So, using neigh_suspect is not a good idea.
If you set it to NUD_INCOMPLETE however, neigh_event_send() is going to add the packet to the neigh's ARP queue and return '1'.
Is that really what you want to happen in this case?
Yes, packets should not be sent out until the neihbour is reachable.

But shouldn't we be using the router in the list which did not enter NUD_FAILED in this situation?

Duan Jiong

author

2014-05-13 05:14 UTC #6 lore raw

Quoted 56 lines · David Miller

From: Duan Jiong <[email protected]>
Date: Tue, 13 May 2014 09:07:12 +0800
Quoted 53 lines
Quoted 52 lines · David Miller
From: Duan Jiong <[email protected]>
Date: Mon, 12 May 2014 11:26:10 +0800
Quoted 40 lines
Quoted 23 lines · Hannes Frederic Sowa
Quoted 22 lines · Duan Jiong
Since commit 7e98056964("ipv6: router reachability probing"), a router falls into NUD_FAILED will be probed.
Now if function rt6_select() selects a router which neighbour state is NUD_FAILED, and at the same time function rt6_probe() changes the neighbour state to NUD_PROBE, then function dst_neigh_output() can directly send packets, but actually the neighbour still is unreachable. If we set nud_state to NUD_INCOMPLETE instead NUD_PROBE, packets will not be sent out until the neihbour is reachable.
In addition, because the route should be probes with a single NS, so we must set neigh->probes to neigh_max_probes(), then the neigh timer timeout and function neigh_timer_handler() will not send other NS Messages.
Signed-off-by Duan Jiong <[email protected]>
---
Changes from v1:
*modify changelog to explain in detail why use neigh_max_probes().
 net/core/neighbour.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 15 lines
net/core/neighbour.c void __neigh_set_probe_once(struct neighbour *neigh) · line 1252
Quoted 12 lines
net/core/neighbour.c +2 -2
index 8f8a96e..32d872e 100644
@@ -1248,8 +1248,8 @@ void __neigh_set_probe_once(struct neighbour *neigh)
 	neigh->updated = jiffies;
 	if (!(neigh->nud_state & NUD_FAILED))
 		return;
-	neigh->nud_state = NUD_PROBE;
-	atomic_set(&neigh->probes, NEIGH_VAR(neigh->parms, UCAST_PROBES));
+	neigh->nud_state = NUD_INCOMPLETE;
+	atomic_set(&neigh->probes, neigh_max_probes(neigh));
Wouldn't it be better if we neigh_suspect the neighbour and leaving the state in NUD_PROBE? We call down to ->output in case neighbour is in NUD_PROBE state, so we must just disable connected 'fast-path'.
Quoted 5 lines
You can look into neigh_event_send() called in neigh_resolve_output(), and if neigh->nud_state still is NUD_PROBE, the neigh_event_send() will return 0, so the packet will still be sent out without probe.
So, using neigh_suspect is not a good idea.
If you set it to NUD_INCOMPLETE however, neigh_event_send() is going to add the packet to the neigh's ARP queue and return '1'.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Is that really what you want to happen in this case?
Yes, packets should not be sent out until the neihbour is reachable.
But shouldn't we be using the router in the list which did not enter NUD_FAILED in this situation?
.

In situation of connected router, the router which enter NUD_FAILED will never be choosed, and we only probe NUD_FAILED router's reachability by sending a single Neighbor Solicitation to that router's address. Finally， we still send packets out through the connected router.

Hannes Frederic Sowa

2014-05-13 16:39 UTC #7 lore raw

Quoted 5 lines · Duan Jiong

In situation of connected router, the router which enter NUD_FAILED will never be choosed, and we only probe NUD_FAILED router's reachability by sending
a single Neighbor Solicitation to that router's address. Finally， we still send packets out through the connected router.

I agree with the patch mostly. The reason why I would liked to see another change without changing nud_state is that we export those states to user space.

Greetings,

Hannes

David Miller

maintainer

2014-05-13 16:43 UTC #8 lore raw

In reply to Duan Jiong

From: Duan Jiong <[email protected]>
Date: Fri, 9 May 2014 13:16:48 +0800

Quoted 9 lines

Since commit 7e98056964("ipv6: router reachability probing"), a router falls into NUD_FAILED will be probed.
Now if function rt6_select() selects a router which neighbour state is NUD_FAILED, and at the same time function rt6_probe() changes the neighbour state to NUD_PROBE, then function dst_neigh_output() can directly send packets, but actually the neighbour still is unreachable. If we set nud_state to NUD_INCOMPLETE instead NUD_PROBE, packets will not be sent out until the neihbour is reachable.

Quoted 5 lines

In addition, because the route should be probes with a single NS, so we must set neigh->probes to neigh_max_probes(), then the neigh timer timeout and function neigh_timer_handler() will not send other NS Messages.
Signed-off-by Duan Jiong <[email protected]>

Applied and queued up for -stable, thanks.