| Message ID | 20200901065758.1141786-1-brianvv@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces | expand |
On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <brianvv@google.com> wrote: > > The problem is exposed when the system has multiple ifaces and > forwarding is enabled on a subset of them, __rt6_purge_dflt_routers will > clean the default route on all the ifaces which is not desired. > > This patches fixes that by cleaning only the routes where the iface has > forwarding enabled. > > Fixes: 830218c1add1 ("net: ipv6: Fix processing of RAs in presence of VRF") > Cc: David Ahern <dsa@cumulusnetworks.com> > Signed-off-by: Brian Vazquez <brianvv@google.com> > --- > net/ipv6/route.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index 5e7e25e2523a..41181cd489ea 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -4283,6 +4283,7 @@ static void __rt6_purge_dflt_routers(struct net *net, > struct fib6_table *table) > { > struct fib6_info *rt; > + bool deleted = false; > > restart: > rcu_read_lock(); > @@ -4291,16 +4292,19 @@ static void __rt6_purge_dflt_routers(struct net *net, > struct inet6_dev *idev = dev ? __in6_dev_get(dev) : NULL; > > if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && > - (!idev || idev->cnf.accept_ra != 2) && > + (!idev || (idev->cnf.forwarding == 1 && > + idev->cnf.accept_ra != 2)) && > fib6_info_hold_safe(rt)) { > rcu_read_unlock(); > ip6_del_rt(net, rt, false); > + deleted = true; > goto restart; > } > } > rcu_read_unlock(); > > - table->flags &= ~RT6_TABLE_HAS_DFLT_ROUTER; > + if (deleted) > + table->flags &= ~RT6_TABLE_HAS_DFLT_ROUTER; This seems wrong : We want to keep the flag set if at least one candidate route has not been deleted, so that next time rt6_purge_dflt_routers() is called, we can call __rt6_purge_dflt_routers() ?
On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <brianvv@google.com> wrote: > > The problem is exposed when the system has multiple ifaces and > forwarding is enabled on a subset of them, __rt6_purge_dflt_routers will > clean the default route on all the ifaces which is not desired. What is the exact problem you want to fix ? > > This patches fixes that by cleaning only the routes where the iface has > forwarding enabled. This seems like a new feature, and this might break some setups. linux always had removed any IPv6 address and default route it learnt via SLAAC. (this might be to conform to one RFC or known security rule). It would be nice to add a nice comment giving references. > > Fixes: 830218c1add1 ("net: ipv6: Fix processing of RAs in presence of VRF") Please provide a test, or a better commit message ? If your patch targets the net tree, then it should only fix a bug. Thanks.
On 9/1/20 1:56 AM, Eric Dumazet wrote: > On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <brianvv@google.com> wrote: >> >> The problem is exposed when the system has multiple ifaces and >> forwarding is enabled on a subset of them, __rt6_purge_dflt_routers will >> clean the default route on all the ifaces which is not desired. >> >> This patches fixes that by cleaning only the routes where the iface has >> forwarding enabled. >> >> Fixes: 830218c1add1 ("net: ipv6: Fix processing of RAs in presence of VRF") are you sure that is a Fixes tag for this problem? looking at that change it only handles RA for tables beyond the main table; it does not change the logic of how many or which routes are purged.
On Tue, Sep 1, 2020 at 12:56 AM Eric Dumazet <edumazet@google.com> wrote: > > On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <brianvv@google.com> wrote: > > > > The problem is exposed when the system has multiple ifaces and > > forwarding is enabled on a subset of them, __rt6_purge_dflt_routers will > > clean the default route on all the ifaces which is not desired. > > > > This patches fixes that by cleaning only the routes where the iface has > > forwarding enabled. > > > > Fixes: 830218c1add1 ("net: ipv6: Fix processing of RAs in presence of VRF") > > > > > > > Cc: David Ahern <dsa@cumulusnetworks.com> > > Signed-off-by: Brian Vazquez <brianvv@google.com> > > --- > > net/ipv6/route.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > > index 5e7e25e2523a..41181cd489ea 100644 > > --- a/net/ipv6/route.c > > +++ b/net/ipv6/route.c > > @@ -4283,6 +4283,7 @@ static void __rt6_purge_dflt_routers(struct net *net, > > struct fib6_table *table) > > { > > struct fib6_info *rt; > > + bool deleted = false; > > > > restart: > > rcu_read_lock(); > > @@ -4291,16 +4292,19 @@ static void __rt6_purge_dflt_routers(struct net *net, > > struct inet6_dev *idev = dev ? __in6_dev_get(dev) : NULL; > > > > if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && > > - (!idev || idev->cnf.accept_ra != 2) && > > + (!idev || (idev->cnf.forwarding == 1 && > > + idev->cnf.accept_ra != 2)) && > > fib6_info_hold_safe(rt)) { > > rcu_read_unlock(); > > ip6_del_rt(net, rt, false); > > + deleted = true; > > goto restart; > > } > > } > > rcu_read_unlock(); > > > > - table->flags &= ~RT6_TABLE_HAS_DFLT_ROUTER; > > + if (deleted) > > + table->flags &= ~RT6_TABLE_HAS_DFLT_ROUTER; > > > This seems wrong : We want to keep the flag set if at least one > candidate route has not been deleted, > so that next time rt6_purge_dflt_routers() is called, we can call > __rt6_purge_dflt_routers() ? Yes, you're right. Although current implementation doesn't hurt because if any of those candidate routes were not deleted means that they have accept_ra == 2 which overrules the router behaviour so we won't clean the SLAAC anyway.
On Tue, Sep 1, 2020 at 1:20 AM Eric Dumazet <edumazet@google.com> wrote: > > On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <brianvv@google.com> wrote: > > > > The problem is exposed when the system has multiple ifaces and > > forwarding is enabled on a subset of them, __rt6_purge_dflt_routers will > > clean the default route on all the ifaces which is not desired. > > What is the exact problem you want to fix ? Imagine you have a host with 2 interfaces. 1 is using SLAAC, the other one it isn't. On your main routing table you have the default SLAAC route for iface A Then you're setting a second interface B and you enable forwarding only on this iface: echo 1 > /proc/sys/net/ipv6/conf/B/forwarding. Changing the sysctl which call the rt6_purge_dflt_routers which would delete your default route on iface A, so effectively you will lose connection via iface A, until the default entry is added again which would happen because that iface has accept_ra = 1 and forwarding = 0, but it would take some time. It feels weird that modifying interface B deletes default route A which would be added back anyway, but you lose connection on A for some minutes. > > > > > This patches fixes that by cleaning only the routes where the iface has > > forwarding enabled. > > This seems like a new feature, and this might break some setups. Fair enough, the main issue here is that the behaviour of a host in a mixed environment is not well defined. > > linux always had removed any IPv6 address and default route it learnt via SLAAC. > (this might be to conform to one RFC or known security rule). > It would be nice to add a nice comment giving references. > > > > > Fixes: 830218c1add1 ("net: ipv6: Fix processing of RAs in presence of VRF") > > Please provide a test, or a better commit message ? > > If your patch targets the net tree, then it should only fix a bug. > > Thanks.
Hey David, On Tue, Sep 1, 2020 at 7:57 AM David Ahern <dsahern@gmail.com> wrote: > > On 9/1/20 1:56 AM, Eric Dumazet wrote: > > On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <brianvv@google.com> wrote: > >> > >> The problem is exposed when the system has multiple ifaces and > >> forwarding is enabled on a subset of them, __rt6_purge_dflt_routers will > >> clean the default route on all the ifaces which is not desired. > >> > >> This patches fixes that by cleaning only the routes where the iface has > >> forwarding enabled. > >> > >> Fixes: z ("net: ipv6: Fix processing of RAs in presence of VRF") > > are you sure that is a Fixes tag for this problem? looking at that > change it only handles RA for tables beyond the main table; it does not > change the logic of how many or which routes are purged. That commit also added RT6_TABLE_HAS_DFLT_ROUTER so I thought that was the commit needed to be mentioned. But probably it shouldn't? Also Am I missing something or this is only called on on the sysctl path? > > >
On 9/1/20 9:50 AM, Brian Vazquez wrote: > Hey David, > > On Tue, Sep 1, 2020 at 7:57 AM David Ahern <dsahern@gmail.com> wrote: >> >> On 9/1/20 1:56 AM, Eric Dumazet wrote: >>> On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <brianvv@google.com> wrote: >>>> >>>> The problem is exposed when the system has multiple ifaces and >>>> forwarding is enabled on a subset of them, __rt6_purge_dflt_routers will >>>> clean the default route on all the ifaces which is not desired. >>>> >>>> This patches fixes that by cleaning only the routes where the iface has >>>> forwarding enabled. >>>> >>>> Fixes: z ("net: ipv6: Fix processing of RAs in presence of VRF") >> >> are you sure that is a Fixes tag for this problem? looking at that >> change it only handles RA for tables beyond the main table; it does not >> change the logic of how many or which routes are purged. > > That commit also added RT6_TABLE_HAS_DFLT_ROUTER so I thought that was > the commit needed to be mentioned. But probably it shouldn't? nah. That flag was added as an optimization. The patch referenced earlier changed the code from looking at one table to looking at all of them. The flag indicates which table have an RA based default route to avoid unnecessary walks. You could probably change it to a counter to handle the case of multiple default route entries. > Also Am I missing something or this is only called on on the sysctl path? It is only called when accept_ra sysctl is enabled as I recall. That setting requires forwarding to be disabled or overridden. See Documentation/networking/ip-sysctl.rst. It should be fairly easy to create a selftest using radvd and network namespaces.
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 5e7e25e2523a..41181cd489ea 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -4283,6 +4283,7 @@ static void __rt6_purge_dflt_routers(struct net *net, struct fib6_table *table) { struct fib6_info *rt; + bool deleted = false; restart: rcu_read_lock(); @@ -4291,16 +4292,19 @@ static void __rt6_purge_dflt_routers(struct net *net, struct inet6_dev *idev = dev ? __in6_dev_get(dev) : NULL; if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && - (!idev || idev->cnf.accept_ra != 2) && + (!idev || (idev->cnf.forwarding == 1 && + idev->cnf.accept_ra != 2)) && fib6_info_hold_safe(rt)) { rcu_read_unlock(); ip6_del_rt(net, rt, false); + deleted = true; goto restart; } } rcu_read_unlock(); - table->flags &= ~RT6_TABLE_HAS_DFLT_ROUTER; + if (deleted) + table->flags &= ~RT6_TABLE_HAS_DFLT_ROUTER; } void rt6_purge_dflt_routers(struct net *net)
The problem is exposed when the system has multiple ifaces and forwarding is enabled on a subset of them, __rt6_purge_dflt_routers will clean the default route on all the ifaces which is not desired. This patches fixes that by cleaning only the routes where the iface has forwarding enabled. Fixes: 830218c1add1 ("net: ipv6: Fix processing of RAs in presence of VRF") Cc: David Ahern <dsa@cumulusnetworks.com> Signed-off-by: Brian Vazquez <brianvv@google.com> --- net/ipv6/route.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)