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

[Laine Stump <laine@xxxxxxxxxx>]

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.


> 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@xxxxxxxxxx>
> ---
>   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));
>       }
>