Re: [PATCH v2] ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule
On 9/24/19 8:01 AM, Jason A. Donenfeld wrote: > Commit 7d9e5f422150 removed references from certain dsts, but accounting > for this never translated down into the fib6 suppression code. This bug > was triggered by WireGuard users who use wg-quick(8), which uses the > "suppress-prefix" directive to ip-rule(8) for routing all of their > internet traffic without routing loops. The test case added here > causes the reference underflow by causing packets to evaluate a suppress > rule. > > Cc: sta...@vger.kernel.org > Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use > RT6_LOOKUP_F_DST_NOREF") > Signed-off-by: Jason A. Donenfeld > --- > net/ipv6/fib6_rules.c| 3 ++- > tools/testing/selftests/net/fib_tests.sh | 17 - > 2 files changed, 18 insertions(+), 2 deletions(-) Thanks for adding the test case. Reviewed-by: David Ahern
Re: [PATCH v2] ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule
From: "Jason A. Donenfeld" Date: Tue, 24 Sep 2019 16:01:28 +0200 > Commit 7d9e5f422150 removed references from certain dsts, but accounting > for this never translated down into the fib6 suppression code. This bug > was triggered by WireGuard users who use wg-quick(8), which uses the > "suppress-prefix" directive to ip-rule(8) for routing all of their > internet traffic without routing loops. The test case added here > causes the reference underflow by causing packets to evaluate a suppress > rule. > > Cc: sta...@vger.kernel.org Please don't CC: stable for networking fixes, I handle these manually as per the netdev FAQ. > Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use > RT6_LOOKUP_F_DST_NOREF") > Signed-off-by: Jason A. Donenfeld Applied and queued up for -stable, thanks.
Re: [PATCH v2] ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule
On Tue, Sep 24, 2019 at 7:01 AM Jason A. Donenfeld wrote: > > Commit 7d9e5f422150 removed references from certain dsts, but accounting > for this never translated down into the fib6 suppression code. This bug > was triggered by WireGuard users who use wg-quick(8), which uses the > "suppress-prefix" directive to ip-rule(8) for routing all of their > internet traffic without routing loops. The test case added here > causes the reference underflow by causing packets to evaluate a suppress > rule. > > Cc: sta...@vger.kernel.org > Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use > RT6_LOOKUP_F_DST_NOREF") > Signed-off-by: Jason A. Donenfeld > --- Acked-by: Wei Wang Good catch. Thanks for the fix. > net/ipv6/fib6_rules.c| 3 ++- > tools/testing/selftests/net/fib_tests.sh | 17 - > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c > index d22b6c140f23..f9e8fe3ff0c5 100644 > --- a/net/ipv6/fib6_rules.c > +++ b/net/ipv6/fib6_rules.c > @@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, > struct fib_lookup_arg *arg > return false; > > suppress_route: > - ip6_rt_put(rt); > + if (!(arg->flags & FIB_LOOKUP_NOREF)) > + ip6_rt_put(rt); > return true; > } > > diff --git a/tools/testing/selftests/net/fib_tests.sh > b/tools/testing/selftests/net/fib_tests.sh > index 4465fc2dae14..c2c5f2bf0f95 100755 > --- a/tools/testing/selftests/net/fib_tests.sh > +++ b/tools/testing/selftests/net/fib_tests.sh > @@ -9,7 +9,7 @@ ret=0 > ksft_skip=4 > > # all tests in this script. Can be overridden with -t option > -TESTS="unregister down carrier nexthop ipv6_rt ipv4_rt ipv6_addr_metric > ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw > rp_filter" > +TESTS="unregister down carrier nexthop suppress ipv6_rt ipv4_rt > ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics > ipv4_route_v6_gw rp_filter" > > VERBOSE=0 > PAUSE_ON_FAIL=no > @@ -614,6 +614,20 @@ fib_nexthop_test() > cleanup > } > > +fib_suppress_test() > +{ > + $IP link add dummy1 type dummy > + $IP link set dummy1 up > + $IP -6 route add default dev dummy1 > + $IP -6 rule add table main suppress_prefixlength 0 > + ping -f -c 1000 -W 1 1234::1 || true > + $IP -6 rule del table main suppress_prefixlength 0 > + $IP link del dummy1 > + > + # If we got here without crashing, we're good. > + return 0 > +} > + > > > # Tests on route add and replace > > @@ -1591,6 +1605,7 @@ do > fib_carrier_test|carrier) fib_carrier_test;; > fib_rp_filter_test|rp_filter) fib_rp_filter_test;; > fib_nexthop_test|nexthop) fib_nexthop_test;; > + fib_suppress_test|suppress) fib_suppress_test;; > ipv6_route_test|ipv6_rt)ipv6_route_test;; > ipv4_route_test|ipv4_rt)ipv4_route_test;; > ipv6_addr_metric) ipv6_addr_metric_test;; > -- > 2.21.0 >
[PATCH v2] ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule
Commit 7d9e5f422150 removed references from certain dsts, but accounting for this never translated down into the fib6 suppression code. This bug was triggered by WireGuard users who use wg-quick(8), which uses the "suppress-prefix" directive to ip-rule(8) for routing all of their internet traffic without routing loops. The test case added here causes the reference underflow by causing packets to evaluate a suppress rule. Cc: sta...@vger.kernel.org Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF") Signed-off-by: Jason A. Donenfeld --- net/ipv6/fib6_rules.c| 3 ++- tools/testing/selftests/net/fib_tests.sh | 17 - 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c index d22b6c140f23..f9e8fe3ff0c5 100644 --- a/net/ipv6/fib6_rules.c +++ b/net/ipv6/fib6_rules.c @@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg return false; suppress_route: - ip6_rt_put(rt); + if (!(arg->flags & FIB_LOOKUP_NOREF)) + ip6_rt_put(rt); return true; } diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh index 4465fc2dae14..c2c5f2bf0f95 100755 --- a/tools/testing/selftests/net/fib_tests.sh +++ b/tools/testing/selftests/net/fib_tests.sh @@ -9,7 +9,7 @@ ret=0 ksft_skip=4 # all tests in this script. Can be overridden with -t option -TESTS="unregister down carrier nexthop ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter" +TESTS="unregister down carrier nexthop suppress ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter" VERBOSE=0 PAUSE_ON_FAIL=no @@ -614,6 +614,20 @@ fib_nexthop_test() cleanup } +fib_suppress_test() +{ + $IP link add dummy1 type dummy + $IP link set dummy1 up + $IP -6 route add default dev dummy1 + $IP -6 rule add table main suppress_prefixlength 0 + ping -f -c 1000 -W 1 1234::1 || true + $IP -6 rule del table main suppress_prefixlength 0 + $IP link del dummy1 + + # If we got here without crashing, we're good. + return 0 +} + # Tests on route add and replace @@ -1591,6 +1605,7 @@ do fib_carrier_test|carrier) fib_carrier_test;; fib_rp_filter_test|rp_filter) fib_rp_filter_test;; fib_nexthop_test|nexthop) fib_nexthop_test;; + fib_suppress_test|suppress) fib_suppress_test;; ipv6_route_test|ipv6_rt)ipv6_route_test;; ipv4_route_test|ipv4_rt)ipv4_route_test;; ipv6_addr_metric) ipv6_addr_metric_test;; -- 2.21.0