[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] network: allow accept_ra == 0 when enabling ipv6 forwarding



On Wed, 2020-08-12 at 19:21 -0400, Laine Stump wrote:
> Yay! A user who follows up their conversation on the libvirt-users list 
> with a patch! :-)
> 
> On 8/11/20 8:14 PM, Ian Wienand wrote:
> > The checks modified here were added with
> > 00d28a78b5d1f6eaf79f06ac59e31c568af9da37 to avoid losing routes on
> > hosts.
> 
> I'm Cc'ing the author of that patch, C├ędric Bosdonnat, to make sure that 
> whatever we end up doing doesn't upset his use case.

Ouch! that is old... even reading my bugzilla log I have troubles
explaining that thing properly. I'll try it though.

So the hypervisor has at least one (Router Advertised) RA route.
After defining a network like the following, the RA route is removed if
accept_ra isn't set to 2.

<network ipv6="yes">
  <name>test5</name>
  <forward mode="nat"/>
  <bridge name="708837c1d27-br0" stp="off"/>
  <mac address="52:54:00:45:5f:27"/>
  <ip
     family="ipv6"
     address="fc00:0000:0000:000f:0000:0000:0000:0001"
     prefix="64"/>
</network>

The RA route was removed in networkEnableIPForwarding() when
setting /proc/sys/net/ipv6/conf/all/forwarding to 1.

Me not being a network expert (and even less on ipv6) doesn't help.

I hope this explanation will help you better see the use case I had.

--
Cedric

> > However, tools such as systemd-networking and NetworkManager manage
> > RA's in userspace and thus IPv6 may be up and working on an interface
> > even with accept_ra == 0.
> > 
> > This modifies the check to only error if an interface's accept_ra is
> > already set to "1"; as noted inline this seems to when it is likely
> > that enabling forwarding may change the RA acceptance behaviour of the
> > interface.
> 
> Unfortunately, on my Fedora 32 machine that has NetworkManager enabled, 
> not all the interfaces have accept_ra=0 by default. One of the bridge 
> devices (created by NetworkManager in response to an ifcfg file in 
> /etc/sysconfig/network-scripts) has accept_ra = 1. (there are several 
> other interfaces that have accept_ra=1, but those interfaces are either 
> not online, or they don't have an IPv6 address.
> 
> 
> So this doesn't work for all cases. I think we need to get more 
> information on how to most easily, generically, and reliably determine 
> if the kernel accept_ra setting can be ignored. Possibly the 
> NetworkManager people can help us out here.
> 
> 
> (or alternately we could punt and just make a configuration setting, 
> although that is taking the easy road, and not a good precedent to set)
> 
> 
> > I have noticed this because I am using the IPv6 NAT features enabled
> > with 927acaedec7effbe67a154d8bfa0e67f7d08e6c7.  I am using this on my
> > laptop which switches between wired and wireless connections; both of
> > which are configured in an unremarkable way by Fedora's NetworkManager
> > and get configured for IPv6 via SLAAC and whatever NetworkManager
> > magic it does.  With this I can define and start a libvirt network
> > with <nat ipv6="yes"> and <ip family='ipv6'
> > address='fc00:abcd:ef12:34::' prefix='64'> and it seems to "just work"
> > for guests.
> > 
> > Signed-off-by: Ian Wienand <iwienand redhat com>
> > ---
> >   src/util/virnetdevip.c | 41 +++++++++++++++++++++++++++--------------
> >   1 file changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > index 409f062c5c..de27cacfc9 100644
> > --- a/src/util/virnetdevip.c
> > +++ b/src/util/virnetdevip.c
> > @@ -496,7 +496,7 @@ virNetDevIPGetAcceptRA(const char *ifname)
> >   }
> >   
> >   struct virNetDevIPCheckIPv6ForwardingData {
> > -    bool hasRARoutes;
> > +    bool hasKernelRARoutes;
> >   
> >       /* Devices with conflicting accept_ra */
> >       char **devices;
> > @@ -552,15 +552,26 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp,
> >           if (!ifname)
> >              return -1;
> >   
> > -        accept_ra = virNetDevIPGetAcceptRA(ifname);
> > -
> >           VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d",
> >                     ifname, ifindex, accept_ra);
> >   
> > -        if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
> > +        accept_ra = virNetDevIPGetAcceptRA(ifname);
> > +        /* 0 = do no accept RA
> > +         * 1 = accept if forwarding disabled
> > +         * 2 = ovveride and accept RA when forwarding enabled
> > +         *
> > +         * When RA is managed by userspace (systemd-networkd or
> > +         * NetworkManager) accept_ra is unset and we don't need to
> > +         * worry about it.  If it is 1, enabling forwarding might
> > +         * change the behaviour so the user needs to be warned.
> > +         */
> > +        if (accept_ra == 0)
> > +            return 0;
> > +
> > +        if (accept_ra == 1 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
> >               return -1;
> >   
> > -        data->hasRARoutes = true;
> > +        data->hasKernelRARoutes = true;
> >           return 0;
> >       }
> >   
> > @@ -590,11 +601,13 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp,
> >               VIR_DEBUG("Checking multipath route nexthop device %s (%d), accept_ra: %d",
> >                         ifname, nh->rtnh_ifindex, accept_ra);
> >   
> > -            if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
> > -                return -1;
> > +            if (accept_ra == 1) {
> > +                if (virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
> > +                    return -1;
> > +                data->hasKernelRARoutes = true;
> > +            }
> >   
> >               VIR_FREE(ifname);
> > -            data->hasRARoutes = true;
> >   
> >               len -= NLMSG_ALIGN(nh->rtnh_len);
> >               VIR_WARNINGS_NO_CAST_ALIGN
> > @@ -613,7 +626,7 @@ virNetDevIPCheckIPv6Forwarding(void)
> >       struct rtgenmsg genmsg;
> >       size_t i;
> >       struct virNetDevIPCheckIPv6ForwardingData data = {
> > -        .hasRARoutes = false,
> > +        .hasKernelRARoutes = false,
> >           .devices = NULL,
> >           .ndevices = 0
> >       };
> > @@ -644,11 +657,11 @@ virNetDevIPCheckIPv6Forwarding(void)
> >           goto cleanup;
> >       }
> >   
> > -    valid = !data.hasRARoutes || data.ndevices == 0;
> > +    valid = !data.hasKernelRARoutes || data.ndevices == 0;
> >   
> >       /* Check the global accept_ra if at least one isn't set on a
> >          per-device basis */
> > -    if (!valid && data.hasRARoutes) {
> > +    if (!valid && data.hasKernelRARoutes) {
> >           int accept_ra = virNetDevIPGetAcceptRA(NULL);
> >           valid = accept_ra == 2;
> >           VIR_DEBUG("Checked global accept_ra: %d", accept_ra);
> > @@ -663,9 +676,9 @@ virNetDevIPCheckIPv6Forwarding(void)
> >           }
> >   
> >           virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("Check the host setup: enabling IPv6 forwarding with "
> > -                         "RA routes without accept_ra set to 2 is likely to cause "
> > -                         "routes loss. Interfaces to look at: %s"),
> > +                       _("Check the host setup: interface has accept_ra set to 1 "
> > +                         "and enabling forwarding without accept_ra set to 2 is "
> > +                         "likely to cause routes loss. Interfaces to look at: %s"),
> >                          virBufferCurrentContent(&buf));
> >       }
> >   
> 
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]