[PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[Brian Vazquez]

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;
 }
 
 void rt6_purge_dflt_routers(struct net *net)
-- 
2.28.0.402.g5ffc5be6b7-goog

Re: [PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[Eric Dumazet]

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() ?

Re: [PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[Eric Dumazet]

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.

Re: [PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[David Ahern]

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.

Re: [PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[Brian Vazquez]

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.

Re: [PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[Brian Vazquez]

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.

Re: [PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[Brian Vazquez]

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?

>
>
>

Re: [PATCH] net: ipv6: fix __rt6_purge_dflt_routers when forwarding is not set on all ifaces

[David Ahern]

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.