On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <[email protected]> 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 <[email protected]>
> Signed-off-by: Brian Vazquez <[email protected]>
> ---
> 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 9/1/20 1:56 AM, Eric Dumazet wrote:
> On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <[email protected]> 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.
Hey David,
On Tue, Sep 1, 2020 at 7:57 AM David Ahern <[email protected]> wrote:
>
> On 9/1/20 1:56 AM, Eric Dumazet wrote:
> > On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <[email protected]> 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 Tue, Sep 1, 2020 at 12:56 AM Eric Dumazet <[email protected]> wrote:
>
> On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <[email protected]> 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 <[email protected]>
> > Signed-off-by: Brian Vazquez <[email protected]>
> > ---
> > 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 9/1/20 9:50 AM, Brian Vazquez wrote:
> Hey David,
>
> On Tue, Sep 1, 2020 at 7:57 AM David Ahern <[email protected]> wrote:
>>
>> On 9/1/20 1:56 AM, Eric Dumazet wrote:
>>> On Tue, Sep 1, 2020 at 8:58 AM Brian Vazquez <[email protected]> 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.