* [PATCH net 1/2] ipv6: Fix route replacement with dev-only route
@ 2020-02-12 1:41 Benjamin Poirier
2020-02-12 1:41 ` [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route Benjamin Poirier
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Benjamin Poirier @ 2020-02-12 1:41 UTC (permalink / raw)
To: netdev; +Cc: Michal Kubeček, Nicolas Dichtel, David Ahern
After commit 27596472473a ("ipv6: fix ECMP route replacement") it is no
longer possible to replace an ECMP-able route by a non ECMP-able route.
For example,
ip route add 2001:db8::1/128 via fe80::1 dev dummy0
ip route replace 2001:db8::1/128 dev dummy0
does not work as expected.
Tweak the replacement logic so that point 3 in the log of the above commit
becomes:
3. If the new route is not ECMP-able, and no matching non-ECMP-able route
exists, replace matching ECMP-able route (if any) or add the new route.
We can now summarize the entire replace semantics to:
When doing a replace, prefer replacing a matching route of the same
"ECMP-able-ness" as the replace argument. If there is no such candidate,
fallback to the first route found.
Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
net/ipv6/ip6_fib.c | 7 ++++---
tools/testing/selftests/net/fib_tests.sh | 6 ++++++
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 58fbde244381..72abf892302f 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1102,8 +1102,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
found++;
break;
}
- if (rt_can_ecmp)
- fallback_ins = fallback_ins ?: ins;
+ fallback_ins = fallback_ins ?: ins;
goto next_iter;
}
@@ -1146,7 +1145,9 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
}
if (fallback_ins && !found) {
- /* No ECMP-able route found, replace first non-ECMP one */
+ /* No matching route with same ecmp-able-ness found, replace
+ * first matching route
+ */
ins = fallback_ins;
iter = rcu_dereference_protected(*ins,
lockdep_is_held(&rt->fib6_table->tb6_lock));
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 6dd403103800..60273f1bc7d9 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -910,6 +910,12 @@ ipv6_rt_replace_mpath()
check_route6 "2001:db8:104::/64 via 2001:db8:101::3 dev veth1 metric 1024"
log_test $? 0 "Multipath with single path via multipath attribute"
+ # multipath with dev-only
+ add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 dev veth1"
+ check_route6 "2001:db8:104::/64 dev veth1 metric 1024"
+ log_test $? 0 "Multipath with dev-only"
+
# route replace fails - invalid nexthop 1
add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:111::3 nexthop via 2001:db8:103::3"
--
2.25.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route
2020-02-12 1:41 [PATCH net 1/2] ipv6: Fix route replacement with dev-only route Benjamin Poirier
@ 2020-02-12 1:41 ` Benjamin Poirier
2020-02-12 21:58 ` Michal Kubecek
` (2 more replies)
2020-02-12 21:57 ` [PATCH net 1/2] ipv6: Fix route replacement with dev-only route Michal Kubecek
` (2 subsequent siblings)
3 siblings, 3 replies; 10+ messages in thread
From: Benjamin Poirier @ 2020-02-12 1:41 UTC (permalink / raw)
To: netdev; +Cc: Michal Kubeček, Nicolas Dichtel, David Ahern
When splitting an RTA_MULTIPATH request into multiple routes and adding the
second and later components, we must not simply remove NLM_F_REPLACE but
instead replace it by NLM_F_CREATE. Otherwise, it may look like the netlink
message was malformed.
For example,
ip route add 2001:db8::1/128 dev dummy0
ip route change 2001:db8::1/128 nexthop via fe80::30:1 dev dummy0 \
nexthop via fe80::30:2 dev dummy0
results in the following warnings:
[ 1035.057019] IPv6: RTM_NEWROUTE with no NLM_F_CREATE or NLM_F_REPLACE
[ 1035.057517] IPv6: NLM_F_CREATE should be set when creating new route
This patch makes the nlmsg sequence look equivalent for __ip6_ins_rt() to
what it would get if the multipath route had been added in multiple netlink
operations:
ip route add 2001:db8::1/128 dev dummy0
ip route change 2001:db8::1/128 nexthop via fe80::30:1 dev dummy0
ip route append 2001:db8::1/128 nexthop via fe80::30:2 dev dummy0
Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
net/ipv6/route.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4fbdc60b4e07..2931224b674e 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -5198,6 +5198,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
*/
cfg->fc_nlinfo.nlh->nlmsg_flags &= ~(NLM_F_EXCL |
NLM_F_REPLACE);
+ cfg->fc_nlinfo.nlh->nlmsg_flags |= NLM_F_CREATE;
nhn++;
}
--
2.25.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] ipv6: Fix route replacement with dev-only route
2020-02-12 1:41 [PATCH net 1/2] ipv6: Fix route replacement with dev-only route Benjamin Poirier
2020-02-12 1:41 ` [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route Benjamin Poirier
@ 2020-02-12 21:57 ` Michal Kubecek
2020-02-15 18:00 ` David Ahern
2020-02-17 2:35 ` David Miller
3 siblings, 0 replies; 10+ messages in thread
From: Michal Kubecek @ 2020-02-12 21:57 UTC (permalink / raw)
To: netdev; +Cc: Benjamin Poirier, Nicolas Dichtel, David Ahern
On Wed, Feb 12, 2020 at 10:41:06AM +0900, Benjamin Poirier wrote:
> After commit 27596472473a ("ipv6: fix ECMP route replacement") it is no
> longer possible to replace an ECMP-able route by a non ECMP-able route.
> For example,
> ip route add 2001:db8::1/128 via fe80::1 dev dummy0
> ip route replace 2001:db8::1/128 dev dummy0
> does not work as expected.
>
> Tweak the replacement logic so that point 3 in the log of the above commit
> becomes:
> 3. If the new route is not ECMP-able, and no matching non-ECMP-able route
> exists, replace matching ECMP-able route (if any) or add the new route.
>
> We can now summarize the entire replace semantics to:
> When doing a replace, prefer replacing a matching route of the same
> "ECMP-able-ness" as the replace argument. If there is no such candidate,
> fallback to the first route found.
>
> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
> Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> net/ipv6/ip6_fib.c | 7 ++++---
> tools/testing/selftests/net/fib_tests.sh | 6 ++++++
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 58fbde244381..72abf892302f 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -1102,8 +1102,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
> found++;
> break;
> }
> - if (rt_can_ecmp)
> - fallback_ins = fallback_ins ?: ins;
> + fallback_ins = fallback_ins ?: ins;
> goto next_iter;
> }
>
> @@ -1146,7 +1145,9 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
> }
>
> if (fallback_ins && !found) {
> - /* No ECMP-able route found, replace first non-ECMP one */
> + /* No matching route with same ecmp-able-ness found, replace
> + * first matching route
> + */
> ins = fallback_ins;
> iter = rcu_dereference_protected(*ins,
> lockdep_is_held(&rt->fib6_table->tb6_lock));
> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> index 6dd403103800..60273f1bc7d9 100755
> --- a/tools/testing/selftests/net/fib_tests.sh
> +++ b/tools/testing/selftests/net/fib_tests.sh
> @@ -910,6 +910,12 @@ ipv6_rt_replace_mpath()
> check_route6 "2001:db8:104::/64 via 2001:db8:101::3 dev veth1 metric 1024"
> log_test $? 0 "Multipath with single path via multipath attribute"
>
> + # multipath with dev-only
> + add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
> + run_cmd "$IP -6 ro replace 2001:db8:104::/64 dev veth1"
> + check_route6 "2001:db8:104::/64 dev veth1 metric 1024"
> + log_test $? 0 "Multipath with dev-only"
> +
> # route replace fails - invalid nexthop 1
> add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
> run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:111::3 nexthop via 2001:db8:103::3"
> --
> 2.25.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route
2020-02-12 1:41 ` [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route Benjamin Poirier
@ 2020-02-12 21:58 ` Michal Kubecek
2020-02-15 17:58 ` David Ahern
2020-02-17 2:35 ` David Miller
2 siblings, 0 replies; 10+ messages in thread
From: Michal Kubecek @ 2020-02-12 21:58 UTC (permalink / raw)
To: netdev; +Cc: Benjamin Poirier, Nicolas Dichtel, David Ahern
On Wed, Feb 12, 2020 at 10:41:07AM +0900, Benjamin Poirier wrote:
> When splitting an RTA_MULTIPATH request into multiple routes and adding the
> second and later components, we must not simply remove NLM_F_REPLACE but
> instead replace it by NLM_F_CREATE. Otherwise, it may look like the netlink
> message was malformed.
>
> For example,
> ip route add 2001:db8::1/128 dev dummy0
> ip route change 2001:db8::1/128 nexthop via fe80::30:1 dev dummy0 \
> nexthop via fe80::30:2 dev dummy0
> results in the following warnings:
> [ 1035.057019] IPv6: RTM_NEWROUTE with no NLM_F_CREATE or NLM_F_REPLACE
> [ 1035.057517] IPv6: NLM_F_CREATE should be set when creating new route
>
> This patch makes the nlmsg sequence look equivalent for __ip6_ins_rt() to
> what it would get if the multipath route had been added in multiple netlink
> operations:
> ip route add 2001:db8::1/128 dev dummy0
> ip route change 2001:db8::1/128 nexthop via fe80::30:1 dev dummy0
> ip route append 2001:db8::1/128 nexthop via fe80::30:2 dev dummy0
>
> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
> Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> net/ipv6/route.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4fbdc60b4e07..2931224b674e 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -5198,6 +5198,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
> */
> cfg->fc_nlinfo.nlh->nlmsg_flags &= ~(NLM_F_EXCL |
> NLM_F_REPLACE);
> + cfg->fc_nlinfo.nlh->nlmsg_flags |= NLM_F_CREATE;
> nhn++;
> }
>
> --
> 2.25.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route
2020-02-12 1:41 ` [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route Benjamin Poirier
2020-02-12 21:58 ` Michal Kubecek
@ 2020-02-15 17:58 ` David Ahern
2020-02-17 2:35 ` David Miller
2 siblings, 0 replies; 10+ messages in thread
From: David Ahern @ 2020-02-15 17:58 UTC (permalink / raw)
To: Benjamin Poirier, netdev; +Cc: Michal Kubeček, Nicolas Dichtel
On 2/11/20 6:41 PM, Benjamin Poirier wrote:
> When splitting an RTA_MULTIPATH request into multiple routes and adding the
> second and later components, we must not simply remove NLM_F_REPLACE but
> instead replace it by NLM_F_CREATE. Otherwise, it may look like the netlink
> message was malformed.
>
> For example,
> ip route add 2001:db8::1/128 dev dummy0
> ip route change 2001:db8::1/128 nexthop via fe80::30:1 dev dummy0 \
> nexthop via fe80::30:2 dev dummy0
> results in the following warnings:
> [ 1035.057019] IPv6: RTM_NEWROUTE with no NLM_F_CREATE or NLM_F_REPLACE
> [ 1035.057517] IPv6: NLM_F_CREATE should be set when creating new route
>
> This patch makes the nlmsg sequence look equivalent for __ip6_ins_rt() to
> what it would get if the multipath route had been added in multiple netlink
> operations:
> ip route add 2001:db8::1/128 dev dummy0
> ip route change 2001:db8::1/128 nexthop via fe80::30:1 dev dummy0
> ip route append 2001:db8::1/128 nexthop via fe80::30:2 dev dummy0
>
> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
> Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
> ---
> net/ipv6/route.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4fbdc60b4e07..2931224b674e 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -5198,6 +5198,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
> */
> cfg->fc_nlinfo.nlh->nlmsg_flags &= ~(NLM_F_EXCL |
> NLM_F_REPLACE);
> + cfg->fc_nlinfo.nlh->nlmsg_flags |= NLM_F_CREATE;
> nhn++;
> }
>
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] ipv6: Fix route replacement with dev-only route
2020-02-12 1:41 [PATCH net 1/2] ipv6: Fix route replacement with dev-only route Benjamin Poirier
2020-02-12 1:41 ` [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route Benjamin Poirier
2020-02-12 21:57 ` [PATCH net 1/2] ipv6: Fix route replacement with dev-only route Michal Kubecek
@ 2020-02-15 18:00 ` David Ahern
2020-02-17 3:12 ` Benjamin Poirier
2020-02-17 2:35 ` David Miller
3 siblings, 1 reply; 10+ messages in thread
From: David Ahern @ 2020-02-15 18:00 UTC (permalink / raw)
To: Benjamin Poirier, netdev
Cc: Michal Kubeček, Nicolas Dichtel, Ido Schimmel
On 2/11/20 6:41 PM, Benjamin Poirier wrote:
> After commit 27596472473a ("ipv6: fix ECMP route replacement") it is no
> longer possible to replace an ECMP-able route by a non ECMP-able route.
> For example,
> ip route add 2001:db8::1/128 via fe80::1 dev dummy0
> ip route replace 2001:db8::1/128 dev dummy0
> does not work as expected.
>
> Tweak the replacement logic so that point 3 in the log of the above commit
> becomes:
> 3. If the new route is not ECMP-able, and no matching non-ECMP-able route
> exists, replace matching ECMP-able route (if any) or add the new route.
>
> We can now summarize the entire replace semantics to:
> When doing a replace, prefer replacing a matching route of the same
> "ECMP-able-ness" as the replace argument. If there is no such candidate,
> fallback to the first route found.
>
> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
> Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
> ---
> net/ipv6/ip6_fib.c | 7 ++++---
> tools/testing/selftests/net/fib_tests.sh | 6 ++++++
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 58fbde244381..72abf892302f 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -1102,8 +1102,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
> found++;
> break;
> }
> - if (rt_can_ecmp)
> - fallback_ins = fallback_ins ?: ins;
> + fallback_ins = fallback_ins ?: ins;
> goto next_iter;
> }
>
> @@ -1146,7 +1145,9 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
> }
>
> if (fallback_ins && !found) {
> - /* No ECMP-able route found, replace first non-ECMP one */
> + /* No matching route with same ecmp-able-ness found, replace
> + * first matching route
> + */
> ins = fallback_ins;
> iter = rcu_dereference_protected(*ins,
> lockdep_is_held(&rt->fib6_table->tb6_lock));
> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> index 6dd403103800..60273f1bc7d9 100755
> --- a/tools/testing/selftests/net/fib_tests.sh
> +++ b/tools/testing/selftests/net/fib_tests.sh
> @@ -910,6 +910,12 @@ ipv6_rt_replace_mpath()
> check_route6 "2001:db8:104::/64 via 2001:db8:101::3 dev veth1 metric 1024"
> log_test $? 0 "Multipath with single path via multipath attribute"
>
> + # multipath with dev-only
> + add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
> + run_cmd "$IP -6 ro replace 2001:db8:104::/64 dev veth1"
> + check_route6 "2001:db8:104::/64 dev veth1 metric 1024"
> + log_test $? 0 "Multipath with dev-only"
> +
> # route replace fails - invalid nexthop 1
> add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
> run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:111::3 nexthop via 2001:db8:103::3"
>
Thanks for adding a test case. I take this to mean that all existing
tests pass with this change. We have found this code to be extremely
sensitive to seemingly obvious changes.
Added Ido.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] ipv6: Fix route replacement with dev-only route
2020-02-12 1:41 [PATCH net 1/2] ipv6: Fix route replacement with dev-only route Benjamin Poirier
` (2 preceding siblings ...)
2020-02-15 18:00 ` David Ahern
@ 2020-02-17 2:35 ` David Miller
3 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2020-02-17 2:35 UTC (permalink / raw)
To: bpoirier; +Cc: netdev, mkubecek, nicolas.dichtel, dsahern
From: Benjamin Poirier <bpoirier@cumulusnetworks.com>
Date: Wed, 12 Feb 2020 10:41:06 +0900
> After commit 27596472473a ("ipv6: fix ECMP route replacement") it is no
> longer possible to replace an ECMP-able route by a non ECMP-able route.
> For example,
> ip route add 2001:db8::1/128 via fe80::1 dev dummy0
> ip route replace 2001:db8::1/128 dev dummy0
> does not work as expected.
>
> Tweak the replacement logic so that point 3 in the log of the above commit
> becomes:
> 3. If the new route is not ECMP-able, and no matching non-ECMP-able route
> exists, replace matching ECMP-able route (if any) or add the new route.
>
> We can now summarize the entire replace semantics to:
> When doing a replace, prefer replacing a matching route of the same
> "ECMP-able-ness" as the replace argument. If there is no such candidate,
> fallback to the first route found.
>
> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
> Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route
2020-02-12 1:41 ` [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route Benjamin Poirier
2020-02-12 21:58 ` Michal Kubecek
2020-02-15 17:58 ` David Ahern
@ 2020-02-17 2:35 ` David Miller
2 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2020-02-17 2:35 UTC (permalink / raw)
To: bpoirier; +Cc: netdev, mkubecek, nicolas.dichtel, dsahern
From: Benjamin Poirier <bpoirier@cumulusnetworks.com>
Date: Wed, 12 Feb 2020 10:41:07 +0900
> When splitting an RTA_MULTIPATH request into multiple routes and adding the
> second and later components, we must not simply remove NLM_F_REPLACE but
> instead replace it by NLM_F_CREATE. Otherwise, it may look like the netlink
> message was malformed.
>
> For example,
> ip route add 2001:db8::1/128 dev dummy0
> ip route change 2001:db8::1/128 nexthop via fe80::30:1 dev dummy0 \
> nexthop via fe80::30:2 dev dummy0
> results in the following warnings:
> [ 1035.057019] IPv6: RTM_NEWROUTE with no NLM_F_CREATE or NLM_F_REPLACE
> [ 1035.057517] IPv6: NLM_F_CREATE should be set when creating new route
>
> This patch makes the nlmsg sequence look equivalent for __ip6_ins_rt() to
> what it would get if the multipath route had been added in multiple netlink
> operations:
> ip route add 2001:db8::1/128 dev dummy0
> ip route change 2001:db8::1/128 nexthop via fe80::30:1 dev dummy0
> ip route append 2001:db8::1/128 nexthop via fe80::30:2 dev dummy0
>
> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
> Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] ipv6: Fix route replacement with dev-only route
2020-02-15 18:00 ` David Ahern
@ 2020-02-17 3:12 ` Benjamin Poirier
2020-02-20 3:20 ` Cong Wang
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Poirier @ 2020-02-17 3:12 UTC (permalink / raw)
To: David Ahern, Cong Wang
Cc: netdev, Michal Kubeček, Nicolas Dichtel, Ido Schimmel
On 2020/02/15 11:00 -0700, David Ahern wrote:
[...]
>
> Thanks for adding a test case. I take this to mean that all existing
> tests pass with this change. We have found this code to be extremely
> sensitive to seemingly obvious changes.
I saw two failures from that test suite, regardless of this change:
IPv4 rp_filter tests
TEST: rp_filter passes local packets [FAIL]
TEST: rp_filter passes loopback packets [FAIL]
The other tests, including the ipv6_rt group of tests, are OK.
The rp_filter tests fail for me even if I build a kernel from commit
adb701d6cfa4 ("selftests: add a test case for rp_filter")
Running the first ping manually, tcpdump shows:
root@vsid:~# tcpdump -nepi lo
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on lo, link-type EN10MB (Ethernet), capture size 262144 bytes
12:01:12.618623 52:54:00:6a:c7:5e > 52:54:00:6a:c7:5e, ethertype IPv4 (0x0800), length 98: 198.51.100.1 > 198.51.100.1: ICMP echo request, id 616, seq 435, length 64
12:01:12.618650 52:54:00:6a:c7:5e > 52:54:00:6a:c7:5e, ethertype IPv4 (0x0800), length 98: 198.51.100.1 > 198.51.100.1: ICMP echo reply, id 616, seq 435, length 64
`ping` doesn't show any replies (since it's bound to dummy1...?).
Cong?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] ipv6: Fix route replacement with dev-only route
2020-02-17 3:12 ` Benjamin Poirier
@ 2020-02-20 3:20 ` Cong Wang
0 siblings, 0 replies; 10+ messages in thread
From: Cong Wang @ 2020-02-20 3:20 UTC (permalink / raw)
To: Benjamin Poirier
Cc: David Ahern, Linux Kernel Network Developers,
Michal Kubeček, Nicolas Dichtel, Ido Schimmel
On Sun, Feb 16, 2020 at 7:12 PM Benjamin Poirier
<bpoirier@cumulusnetworks.com> wrote:
>
> On 2020/02/15 11:00 -0700, David Ahern wrote:
> [...]
> >
> > Thanks for adding a test case. I take this to mean that all existing
> > tests pass with this change. We have found this code to be extremely
> > sensitive to seemingly obvious changes.
>
> I saw two failures from that test suite, regardless of this change:
>
> IPv4 rp_filter tests
> TEST: rp_filter passes local packets [FAIL]
> TEST: rp_filter passes loopback packets [FAIL]
>
> The other tests, including the ipv6_rt group of tests, are OK.
>
>
> The rp_filter tests fail for me even if I build a kernel from commit
> adb701d6cfa4 ("selftests: add a test case for rp_filter")
>
> Running the first ping manually, tcpdump shows:
> root@vsid:~# tcpdump -nepi lo
> tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
> listening on lo, link-type EN10MB (Ethernet), capture size 262144 bytes
> 12:01:12.618623 52:54:00:6a:c7:5e > 52:54:00:6a:c7:5e, ethertype IPv4 (0x0800), length 98: 198.51.100.1 > 198.51.100.1: ICMP echo request, id 616, seq 435, length 64
> 12:01:12.618650 52:54:00:6a:c7:5e > 52:54:00:6a:c7:5e, ethertype IPv4 (0x0800), length 98: 198.51.100.1 > 198.51.100.1: ICMP echo reply, id 616, seq 435, length 64
>
> `ping` doesn't show any replies (since it's bound to dummy1...?).
I suspect this is related to the version of ping, as it works perfectly
for me on an old Fedora:
IPv4 rp_filter tests
ping: Warning: source address might be selected on device other than dummy1.
PING 198.51.100.1 (198.51.100.1) from 198.51.100.1 dummy1: 56(84) bytes of data.
64 bytes from 198.51.100.1: icmp_seq=1 ttl=64 time=0.452 ms
64 bytes from 198.51.100.1: icmp_seq=2 ttl=64 time=0.418 ms
64 bytes from 198.51.100.1: icmp_seq=3 ttl=64 time=0.485 ms
64 bytes from 198.51.100.1: icmp_seq=4 ttl=64 time=0.446 ms
64 bytes from 198.51.100.1: icmp_seq=5 ttl=64 time=0.388 ms
64 bytes from 198.51.100.1: icmp_seq=6 ttl=64 time=0.362 ms
64 bytes from 198.51.100.1: icmp_seq=7 ttl=64 time=0.373 ms
64 bytes from 198.51.100.1: icmp_seq=8 ttl=64 time=0.515 ms
64 bytes from 198.51.100.1: icmp_seq=9 ttl=64 time=0.641 ms
--- 198.51.100.1 ping statistics ---
9 packets transmitted, 9 received, 0% packet loss, time 8012ms
rtt min/avg/max/mdev = 0.362/0.453/0.641/0.083 ms
TEST: rp_filter passes local packets [ OK ]
I do see the same failure on a new Fedora. So, I will take a look
at this tomorrow.
Thanks!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-02-20 3:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 1:41 [PATCH net 1/2] ipv6: Fix route replacement with dev-only route Benjamin Poirier
2020-02-12 1:41 ` [PATCH net 2/2] ipv6: Fix nlmsg_flags when splitting a multipath route Benjamin Poirier
2020-02-12 21:58 ` Michal Kubecek
2020-02-15 17:58 ` David Ahern
2020-02-17 2:35 ` David Miller
2020-02-12 21:57 ` [PATCH net 1/2] ipv6: Fix route replacement with dev-only route Michal Kubecek
2020-02-15 18:00 ` David Ahern
2020-02-17 3:12 ` Benjamin Poirier
2020-02-20 3:20 ` Cong Wang
2020-02-17 2:35 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.