Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1450399 > unrolled thread
| Started by | Chunhui He <hchunhui@mail.ustc.edu.cn> |
|---|---|
| First post | 2016-07-26 08:20 +0200 |
| Last post | 2016-07-26 23:30 +0200 |
| Articles | 4 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v2] net: neigh: disallow transition to NUD_STALE if lladdr is unchanged in neigh_update() Chunhui He <hchunhui@mail.ustc.edu.cn> - 2016-07-26 08:20 +0200
Re: [PATCH v2] net: neigh: disallow transition to NUD_STALE if lladdr is unchanged in neigh_update() Julian Anastasov <ja@ssi.bg> - 2016-07-26 09:20 +0200
Re: [PATCH v2] net: neigh: disallow transition to NUD_STALE if lladdr is unchanged in neigh_update() Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-07-26 11:10 +0200
Re: [PATCH v2] net: neigh: disallow transition to NUD_STALE if lladdr is unchanged in neigh_update() David Miller <davem@davemloft.net> - 2016-07-26 23:30 +0200
| From | Chunhui He <hchunhui@mail.ustc.edu.cn> |
|---|---|
| Date | 2016-07-26 08:20 +0200 |
| Subject | [PATCH v2] net: neigh: disallow transition to NUD_STALE if lladdr is unchanged in neigh_update() |
| Message-ID | <rZ3SF-6Uk-21@gated-at.bofh.it> |
NUD_STALE is used when the caller(e.g. arp_process()) can't guarantee
neighbour reachability. If the entry was NUD_VALID and lladdr is unchanged,
the entry state should not be changed.
Currently the code puts an extra "NUD_CONNECTED" condition. So if old state
was NUD_DELAY or NUD_PROBE (they are NUD_VALID but not NUD_CONNECTED), the
state can be changed to NUD_STALE.
This may cause problem. Because NUD_STALE lladdr doesn't guarantee
reachability, when we send traffic, the state will be changed to
NUD_DELAY. In normal case, if we get no confirmation (by dst_confirm()),
we will change the state to NUD_PROBE and send probe traffic. But now the
state may be reset to NUD_STALE again(e.g. by broadcast ARP packets),
so the probe traffic will not be sent. This situation may happen again and
again, and packets will be sent to an non-reachable lladdr forever.
The fix is to remove the "NUD_CONNECTED" condition. After that the
"NEIGH_UPDATE_F_WEAK_OVERRIDE" condition (used by IPv6) in that branch will
be redundant, so remove it.
This change may increase probe traffic, but it's essential since NUD_STALE
lladdr is unreliable. To ensure correctness, we prefer to resolve lladdr,
when we can't get confirmation, even while remote packets try to set
NUD_STALE state.
Signed-off-by: Chunhui He <hchunhui@mail.ustc.edu.cn>
---
v2:
- change title from "net: neigh: disallow state transition DELAY->STALE in
neigh_update()"
- remove "NUD_CONNECTED" condition instead of "NUD_CONNECTED | NUD_DELAY"
- remove "NEIGH_UPDATE_F_WEAK_OVERRIDE" condition
---
net/core/neighbour.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 510cd62..ed8c317e 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1060,8 +1060,6 @@ static void neigh_update_hhs(struct neighbour *neigh)
NEIGH_UPDATE_F_WEAK_OVERRIDE will suspect existing "connected"
lladdr instead of overriding it
if it is different.
- It also allows to retain current state
- if lladdr is unchanged.
NEIGH_UPDATE_F_ADMIN means that the change is administrative.
NEIGH_UPDATE_F_OVERRIDE_ISROUTER allows to override existing
@@ -1150,10 +1148,7 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
} else
goto out;
} else {
- if (lladdr == neigh->ha && new == NUD_STALE &&
- ((flags & NEIGH_UPDATE_F_WEAK_OVERRIDE) ||
- (old & NUD_CONNECTED))
- )
+ if (lladdr == neigh->ha && new == NUD_STALE)
new = old;
}
}
--
2.1.4
[toc] | [next] | [standalone]
| From | Julian Anastasov <ja@ssi.bg> |
|---|---|
| Date | 2016-07-26 09:20 +0200 |
| Subject | Re: [PATCH v2] net: neigh: disallow transition to NUD_STALE if lladdr is unchanged in neigh_update() |
| Message-ID | <rZ4OJ-7t2-1@gated-at.bofh.it> |
| In reply to | #1450399 |
Hello,
On Tue, 26 Jul 2016, Chunhui He wrote:
> NUD_STALE is used when the caller(e.g. arp_process()) can't guarantee
> neighbour reachability. If the entry was NUD_VALID and lladdr is unchanged,
> the entry state should not be changed.
>
> Currently the code puts an extra "NUD_CONNECTED" condition. So if old state
> was NUD_DELAY or NUD_PROBE (they are NUD_VALID but not NUD_CONNECTED), the
> state can be changed to NUD_STALE.
>
> This may cause problem. Because NUD_STALE lladdr doesn't guarantee
> reachability, when we send traffic, the state will be changed to
> NUD_DELAY. In normal case, if we get no confirmation (by dst_confirm()),
> we will change the state to NUD_PROBE and send probe traffic. But now the
> state may be reset to NUD_STALE again(e.g. by broadcast ARP packets),
> so the probe traffic will not be sent. This situation may happen again and
> again, and packets will be sent to an non-reachable lladdr forever.
>
> The fix is to remove the "NUD_CONNECTED" condition. After that the
> "NEIGH_UPDATE_F_WEAK_OVERRIDE" condition (used by IPv6) in that branch will
> be redundant, so remove it.
>
> This change may increase probe traffic, but it's essential since NUD_STALE
> lladdr is unreliable. To ensure correctness, we prefer to resolve lladdr,
> when we can't get confirmation, even while remote packets try to set
> NUD_STALE state.
>
> Signed-off-by: Chunhui He <hchunhui@mail.ustc.edu.cn>
Looks good to me,
Signed-off-by: Julian Anastasov <ja@ssi.bg>
> ---
> v2:
> - change title from "net: neigh: disallow state transition DELAY->STALE in
> neigh_update()"
> - remove "NUD_CONNECTED" condition instead of "NUD_CONNECTED | NUD_DELAY"
> - remove "NEIGH_UPDATE_F_WEAK_OVERRIDE" condition
>
> ---
> net/core/neighbour.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 510cd62..ed8c317e 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1060,8 +1060,6 @@ static void neigh_update_hhs(struct neighbour *neigh)
> NEIGH_UPDATE_F_WEAK_OVERRIDE will suspect existing "connected"
> lladdr instead of overriding it
> if it is different.
> - It also allows to retain current state
> - if lladdr is unchanged.
> NEIGH_UPDATE_F_ADMIN means that the change is administrative.
>
> NEIGH_UPDATE_F_OVERRIDE_ISROUTER allows to override existing
> @@ -1150,10 +1148,7 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
> } else
> goto out;
> } else {
> - if (lladdr == neigh->ha && new == NUD_STALE &&
> - ((flags & NEIGH_UPDATE_F_WEAK_OVERRIDE) ||
> - (old & NUD_CONNECTED))
> - )
> + if (lladdr == neigh->ha && new == NUD_STALE)
> new = old;
> }
> }
> --
> 2.1.4
Regards
--
Julian Anastasov <ja@ssi.bg>
[toc] | [prev] | [next] | [standalone]
| From | Hannes Frederic Sowa <hannes@stressinduktion.org> |
|---|---|
| Date | 2016-07-26 11:10 +0200 |
| Subject | Re: [PATCH v2] net: neigh: disallow transition to NUD_STALE if lladdr is unchanged in neigh_update() |
| Message-ID | <rZ6xb-5m-15@gated-at.bofh.it> |
| In reply to | #1450399 |
On 26.07.2016 08:16, Chunhui He wrote: > NUD_STALE is used when the caller(e.g. arp_process()) can't guarantee > neighbour reachability. If the entry was NUD_VALID and lladdr is unchanged, > the entry state should not be changed. > > Currently the code puts an extra "NUD_CONNECTED" condition. So if old state > was NUD_DELAY or NUD_PROBE (they are NUD_VALID but not NUD_CONNECTED), the > state can be changed to NUD_STALE. > > This may cause problem. Because NUD_STALE lladdr doesn't guarantee > reachability, when we send traffic, the state will be changed to > NUD_DELAY. In normal case, if we get no confirmation (by dst_confirm()), > we will change the state to NUD_PROBE and send probe traffic. But now the > state may be reset to NUD_STALE again(e.g. by broadcast ARP packets), > so the probe traffic will not be sent. This situation may happen again and > again, and packets will be sent to an non-reachable lladdr forever. > > The fix is to remove the "NUD_CONNECTED" condition. After that the > "NEIGH_UPDATE_F_WEAK_OVERRIDE" condition (used by IPv6) in that branch will > be redundant, so remove it. > > This change may increase probe traffic, but it's essential since NUD_STALE > lladdr is unreliable. To ensure correctness, we prefer to resolve lladdr, > when we can't get confirmation, even while remote packets try to set > NUD_STALE state. > > Signed-off-by: Chunhui He <hchunhui@mail.ustc.edu.cn> > --- > v2: > - change title from "net: neigh: disallow state transition DELAY->STALE in > neigh_update()" > - remove "NUD_CONNECTED" condition instead of "NUD_CONNECTED | NUD_DELAY" > - remove "NEIGH_UPDATE_F_WEAK_OVERRIDE" condition Reviewed-by: Hannes Frederic Sowa <hannes@stressinduktion.org> Thanks!
[toc] | [prev] | [next] | [standalone]
| From | David Miller <davem@davemloft.net> |
|---|---|
| Date | 2016-07-26 23:30 +0200 |
| Subject | Re: [PATCH v2] net: neigh: disallow transition to NUD_STALE if lladdr is unchanged in neigh_update() |
| Message-ID | <rZi5j-7eZ-1@gated-at.bofh.it> |
| In reply to | #1450399 |
From: Chunhui He <hchunhui@mail.ustc.edu.cn> Date: Tue, 26 Jul 2016 06:16:52 +0000 > NUD_STALE is used when the caller(e.g. arp_process()) can't guarantee > neighbour reachability. If the entry was NUD_VALID and lladdr is unchanged, > the entry state should not be changed. > > Currently the code puts an extra "NUD_CONNECTED" condition. So if old state > was NUD_DELAY or NUD_PROBE (they are NUD_VALID but not NUD_CONNECTED), the > state can be changed to NUD_STALE. > > This may cause problem. Because NUD_STALE lladdr doesn't guarantee > reachability, when we send traffic, the state will be changed to > NUD_DELAY. In normal case, if we get no confirmation (by dst_confirm()), > we will change the state to NUD_PROBE and send probe traffic. But now the > state may be reset to NUD_STALE again(e.g. by broadcast ARP packets), > so the probe traffic will not be sent. This situation may happen again and > again, and packets will be sent to an non-reachable lladdr forever. > > The fix is to remove the "NUD_CONNECTED" condition. After that the > "NEIGH_UPDATE_F_WEAK_OVERRIDE" condition (used by IPv6) in that branch will > be redundant, so remove it. > > This change may increase probe traffic, but it's essential since NUD_STALE > lladdr is unreliable. To ensure correctness, we prefer to resolve lladdr, > when we can't get confirmation, even while remote packets try to set > NUD_STALE state. > > Signed-off-by: Chunhui He <hchunhui@mail.ustc.edu.cn> Applied, thanks.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web