| Message ID | 20200901065758.1141786-1-brianvv@google.com |
|---|---|
| State | New |
| 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 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.
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? > > >
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(-)