[libvirt] [PATCH v2 0/5] Prevent losing IPv6 routes due to forwarding
Hi Laine, all, Here is the v2 of my series. The changes are: * Add a commit to create a virNetDevGetName() function * Fix Laine's comments Cédric Bosdonnat (5): util: extract the request sending code from virNetlinkCommand() util: add virNetlinkDumpCommand() bridge_driver.c: more uses of SYSCTL_PATH util: add virNetDevGetName() function network: check accept_ra before enabling ipv6 forwarding src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 25 ++++--- src/util/virnetdev.c | 19 ++++++ src/util/virnetdev.h | 2 + src/util/virnetdevip.c | 158 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevip.h | 1 + src/util/virnetlink.c | 145 ++++++++++++++++++++++++++++++---------- src/util/virnetlink.h | 9 +++ 8 files changed, 319 insertions(+), 43 deletions(-) -- 2.11.0
Allow to reuse as much as possible from virNetlinkCommand(). This comment prepares for the introduction of virNetlindDumpCommand() only differing by how it handles the responses. --- src/util/virnetlink.c | 89 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index a5d10fa8e..be00351db 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -209,61 +209,38 @@ virNetlinkCreateSocket(int protocol) goto cleanup; } - -/** - * virNetlinkCommand: - * @nlmsg: pointer to netlink message - * @respbuf: pointer to pointer where response buffer will be allocated - * @respbuflen: pointer to integer holding the size of the response buffer - * on return of the function. - * @src_pid: the pid of the process to send a message - * @dst_pid: the pid of the process to talk to, i.e., pid = 0 for kernel - * @protocol: netlink protocol - * @groups: the group identifier - * - * Send the given message to the netlink layer and receive response. - * Returns 0 on success, -1 on error. In case of error, no response - * buffer will be returned. - */ -int virNetlinkCommand(struct nl_msg *nl_msg, - struct nlmsghdr **resp, unsigned int *respbuflen, - uint32_t src_pid, uint32_t dst_pid, +static virNetlinkHandle * +virNetlinkSendRequest(struct nl_msg *nl_msg, uint32_t src_pid, + struct sockaddr_nl nladdr, unsigned int protocol, unsigned int groups) { - int ret = -1; - struct sockaddr_nl nladdr = { - .nl_family = AF_NETLINK, - .nl_pid = dst_pid, - .nl_groups = 0, - }; ssize_t nbytes; - struct pollfd fds[1]; int fd; int n; - struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg); virNetlinkHandle *nlhandle = NULL; - int len = 0; + struct pollfd fds[1]; + struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg); if (protocol >= MAX_LINKS) { virReportSystemError(EINVAL, _("invalid protocol argument: %d"), protocol); - goto cleanup; + goto error; } if (!(nlhandle = virNetlinkCreateSocket(protocol))) - goto cleanup; + goto error; fd = nl_socket_get_fd(nlhandle); if (fd < 0) { virReportSystemError(errno, "%s", _("cannot get netlink socket fd")); - goto cleanup; + goto error; } if (groups && nl_socket_add_membership(nlhandle, groups) < 0) { virReportSystemError(errno, "%s", _("cannot add netlink membership")); - goto cleanup; + goto error; } nlmsg_set_dst(nl_msg, &nladdr); @@ -274,10 +251,11 @@ int virNetlinkCommand(struct nl_msg *nl_msg, if (nbytes < 0) { virReportSystemError(errno, "%s", _("cannot send to netlink socket")); - goto cleanup; + goto error; } memset(fds, 0, sizeof(fds)); + fds[0].fd = fd; fds[0].events = POLLIN; @@ -289,9 +267,51 @@ int virNetlinkCommand(struct nl_msg *nl_msg, if (n == 0) virReportSystemError(ETIMEDOUT, "%s", _("no valid netlink response was received")); - goto cleanup; } + return nlhandle; + + error: + virNetlinkFree(nlhandle); + return NULL; +} + +/** + * virNetlinkCommand: + * @nlmsg: pointer to netlink message + * @respbuf: pointer to pointer where response buffer will be allocated + * @respbuflen: pointer to integer holding the size of the response buffer + * on return of the function. + * @src_pid: the pid of the process to send a message + * @dst_pid: the pid of the process to talk to, i.e., pid = 0 for kernel + * @protocol: netlink protocol + * @groups: the group identifier + * + * Send the given message to the netlink layer and receive response. + * Returns 0 on success, -1 on error. In case of error, no response + * buffer will be returned. + */ +int virNetlinkCommand(struct nl_msg *nl_msg, + struct nlmsghdr **resp, unsigned int *respbuflen, + uint32_t src_pid, uint32_t dst_pid, + unsigned int protocol, unsigned int groups) +{ + int ret = -1; + struct sockaddr_nl nladdr = { + .nl_family = AF_NETLINK, + .nl_pid = dst_pid, + .nl_groups = 0, + }; + struct pollfd fds[1]; + virNetlinkHandle *nlhandle = NULL; + int len = 0; + + memset(fds, 0, sizeof(fds)); + + if (!(nlhandle = virNetlinkSendRequest(nl_msg, src_pid, nladdr, + protocol, groups))) + goto cleanup; + len = nl_recv(nlhandle, &nladdr, (unsigned char **)resp, NULL); if (len == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -315,7 +335,6 @@ int virNetlinkCommand(struct nl_msg *nl_msg, return ret; } - /** * virNetlinkDumpLink: * -- 2.11.0
On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
Allow to reuse as much as possible from virNetlinkCommand(). This comment prepares for the introduction of virNetlindDumpCommand()
s/Netlind/Netlink/ :-)
only differing by how it handles the responses. --- src/util/virnetlink.c | 89 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 35 deletions(-)
ACK.
virNetlinkCommand() processes only one response message, while some netlink commands like routes dumping need to process several ones. Add virNetlinkDumpCommand() as a virNetlinkCommand() sister. --- src/libvirt_private.syms | 1 + src/util/virnetlink.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 9 ++++++++ 3 files changed, 68 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4efea0098..1f25e42d8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2127,6 +2127,7 @@ virNetDevVPortProfileOpTypeToString; # util/virnetlink.h virNetlinkCommand; virNetlinkDelLink; +virNetlinkDumpCommand; virNetlinkDumpLink; virNetlinkEventAddClient; virNetlinkEventRemoveClient; diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index be00351db..9bc1f0f2b 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -335,6 +335,52 @@ int virNetlinkCommand(struct nl_msg *nl_msg, return ret; } +int +virNetlinkDumpCommand(struct nl_msg *nl_msg, + virNetlinkDumpCallback callback, + uint32_t src_pid, uint32_t dst_pid, + unsigned int protocol, unsigned int groups, + void *opaque) +{ + int ret = -1; + bool end = false; + int len = 0; + struct nlmsghdr *resp = NULL; + struct nlmsghdr *msg = NULL; + + struct sockaddr_nl nladdr = { + .nl_family = AF_NETLINK, + .nl_pid = dst_pid, + .nl_groups = 0, + }; + virNetlinkHandle *nlhandle = NULL; + + if (!(nlhandle = virNetlinkSendRequest(nl_msg, src_pid, nladdr, + protocol, groups))) + goto cleanup; + + while (!end) { + len = nl_recv(nlhandle, &nladdr, (unsigned char **)&resp, NULL); + + for (msg = resp; NLMSG_OK(msg, len); msg = NLMSG_NEXT(msg, len)) { + if (msg->nlmsg_type == NLMSG_DONE) + end = true; + + if (virNetlinkGetErrorCode(msg, len) < 0) + goto cleanup; + + if (callback(msg, opaque) < 0) + goto cleanup; + } + } + + ret = 0; + + cleanup: + virNetlinkFree(nlhandle); + return ret; +} + /** * virNetlinkDumpLink: * @@ -1061,6 +1107,18 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, return -1; } +int +virNetlinkDumpCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, + virNetlinkDumpCallback callback ATTRIBUTE_UNUSED, + uint32_t src_pid ATTRIBUTE_UNUSED, + uint32_t dst_pid ATTRIBUTE_UNUSED, + unsigned int protocol ATTRIBUTE_UNUSED, + unsigned int groups ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return -1; +} int virNetlinkDumpLink(const char *ifname ATTRIBUTE_UNUSED, diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 11e817c82..088b01343 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -52,6 +52,15 @@ int virNetlinkCommand(struct nl_msg *nl_msg, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups); +typedef int (*virNetlinkDumpCallback)(const struct nlmsghdr *resp, + void *data); + +int virNetlinkDumpCommand(struct nl_msg *nl_msg, + virNetlinkDumpCallback callback, + uint32_t src_pid, uint32_t dst_pid, + unsigned int protocol, unsigned int groups, + void *opaque); + typedef int (*virNetlinkDelLinkFallback)(const char *ifname); int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback); -- 2.11.0
On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
virNetlinkCommand() processes only one response message, while some netlink commands like routes dumping need to process several ones.
"while some netlink commands, like route dumping, need to process several."
Add virNetlinkDumpCommand() as a virNetlinkCommand() sister. --- src/libvirt_private.syms | 1 + src/util/virnetlink.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 9 ++++++++ 3 files changed, 68 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4efea0098..1f25e42d8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2127,6 +2127,7 @@ virNetDevVPortProfileOpTypeToString; # util/virnetlink.h virNetlinkCommand; virNetlinkDelLink; +virNetlinkDumpCommand; virNetlinkDumpLink; virNetlinkEventAddClient; virNetlinkEventRemoveClient; diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index be00351db..9bc1f0f2b 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -335,6 +335,52 @@ int virNetlinkCommand(struct nl_msg *nl_msg, return ret; }
+int +virNetlinkDumpCommand(struct nl_msg *nl_msg, + virNetlinkDumpCallback callback, + uint32_t src_pid, uint32_t dst_pid, + unsigned int protocol, unsigned int groups, + void *opaque) +{ + int ret = -1; + bool end = false; + int len = 0; + struct nlmsghdr *resp = NULL; + struct nlmsghdr *msg = NULL; + + struct sockaddr_nl nladdr = { + .nl_family = AF_NETLINK, + .nl_pid = dst_pid, + .nl_groups = 0, + }; + virNetlinkHandle *nlhandle = NULL; + + if (!(nlhandle = virNetlinkSendRequest(nl_msg, src_pid, nladdr, + protocol, groups))) + goto cleanup; + + while (!end) { + len = nl_recv(nlhandle, &nladdr, (unsigned char **)&resp, NULL); + + for (msg = resp; NLMSG_OK(msg, len); msg = NLMSG_NEXT(msg, len)) { + if (msg->nlmsg_type == NLMSG_DONE) + end = true; + + if (virNetlinkGetErrorCode(msg, len) < 0) + goto cleanup;
Seeing this reminded me - a followup patch to add virNetlinkGetErrorCode() to virNetlinkCommand() (and remove it, or the equivalent open code, from all its callers) would be nice. Not required for this series though... ACK. (Nice job of refactoring to reuse the existing code btw :-)
+ + if (callback(msg, opaque) < 0) + goto cleanup; + } + } + + ret = 0; + + cleanup: + virNetlinkFree(nlhandle); + return ret; +} + /** * virNetlinkDumpLink: * @@ -1061,6 +1107,18 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, return -1; }
+int +virNetlinkDumpCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, + virNetlinkDumpCallback callback ATTRIBUTE_UNUSED, + uint32_t src_pid ATTRIBUTE_UNUSED, + uint32_t dst_pid ATTRIBUTE_UNUSED, + unsigned int protocol ATTRIBUTE_UNUSED, + unsigned int groups ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return -1; +}
int virNetlinkDumpLink(const char *ifname ATTRIBUTE_UNUSED, diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 11e817c82..088b01343 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -52,6 +52,15 @@ int virNetlinkCommand(struct nl_msg *nl_msg, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups);
+typedef int (*virNetlinkDumpCallback)(const struct nlmsghdr *resp, + void *data); + +int virNetlinkDumpCommand(struct nl_msg *nl_msg, + virNetlinkDumpCallback callback, + uint32_t src_pid, uint32_t dst_pid, + unsigned int protocol, unsigned int groups, + void *opaque); + typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
Replace a few occurences of /proc/sys by the corresponding macro defined a few lines after: SYSCTL_PATH --- src/network/bridge_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c5ec2823d..3f6561055 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -85,6 +85,8 @@ */ #define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX (32 * 1024 * 1024) +#define SYSCTL_PATH "/proc/sys" + VIR_LOG_INIT("network.bridge_driver"); static virNetworkDriverStatePtr network_driver; @@ -2080,15 +2082,14 @@ networkEnableIPForwarding(bool enableIPv4, bool enableIPv6) &enabled, sizeof(enabled)); #else if (enableIPv4) - ret = virFileWriteStr("/proc/sys/net/ipv4/ip_forward", "1\n", 0); + ret = virFileWriteStr(SYSCTL_PATH "/net/ipv4/ip_forward", "1\n", 0); if (enableIPv6 && ret == 0) - ret = virFileWriteStr("/proc/sys/net/ipv6/conf/all/forwarding", "1\n", 0); + ret = virFileWriteStr(SYSCTL_PATH "/net/ipv6/conf/all/forwarding", "1\n", 0); + #endif return ret; } -#define SYSCTL_PATH "/proc/sys" - static int networkSetIPv6Sysctls(virNetworkObjPtr network) { -- 2.11.0
On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
Replace a few occurences of /proc/sys by the corresponding macro defined a few lines after: SYSCTL_PATH --- src/network/bridge_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
ACK.
Add a function getting the name of a network interface out of its index. --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 19 +++++++++++++++++++ src/util/virnetdev.h | 2 ++ 3 files changed, 22 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1f25e42d8..0fe88c3fa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1982,6 +1982,7 @@ virNetDevFeatureTypeToString; virNetDevGetFeatures; virNetDevGetIndex; virNetDevGetLinkInfo; +virNetDevGetName; virNetDevGetMAC; virNetDevGetMTU; virNetDevGetOnline; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index d12324878..91a5274aa 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -899,6 +899,25 @@ virNetDevGetRcvAllMulti(const char *ifname, return virNetDevGetIFFlag(ifname, VIR_IFF_ALLMULTI, receive); } +char *virNetDevGetName(int ifindex) +{ + char name[IFNAMSIZ]; + char *ifname = NULL; + + memset(&name, 0, sizeof(name)); + + if (!if_indextoname(ifindex, name)) { + virReportSystemError(errno, + _("Failed to convert interface index %d to a name"), + ifindex); + goto cleanup; + } + + ignore_value(VIR_STRDUP(ifname, name)); + + cleanup: + return ifname; +} /** * virNetDevGetIndex: diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 236cf83ef..01e9c5b95 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -157,6 +157,8 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) int virNetDevSetName(const char *ifname, const char *newifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +char *virNetDevGetName(int ifindex) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virNetDevGetIndex(const char *ifname, int *ifindex) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -- 2.11.0
On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
Add a function getting the name of a network interface out of its index. --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 19 +++++++++++++++++++ src/util/virnetdev.h | 2 ++ 3 files changed, 22 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1f25e42d8..0fe88c3fa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1982,6 +1982,7 @@ virNetDevFeatureTypeToString; virNetDevGetFeatures; virNetDevGetIndex; virNetDevGetLinkInfo; +virNetDevGetName; virNetDevGetMAC; virNetDevGetMTU; virNetDevGetOnline; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index d12324878..91a5274aa 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -899,6 +899,25 @@ virNetDevGetRcvAllMulti(const char *ifname, return virNetDevGetIFFlag(ifname, VIR_IFF_ALLMULTI, receive); }
+char *virNetDevGetName(int ifindex) +{ + char name[IFNAMSIZ]; + char *ifname = NULL; + + memset(&name, 0, sizeof(name)); + + if (!if_indextoname(ifindex, name)) { + virReportSystemError(errno, + _("Failed to convert interface index %d to a name"), + ifindex); + goto cleanup; + } + + ignore_value(VIR_STRDUP(ifname, name)); + + cleanup: + return ifname; +}
This is defined outside of any #ifdefs, which I *think* is okay, since the function exists in both Linux and FreeBSD. I guess if anybody complains then we can think about testing for the function at configure time, but no sense in complicating configure.ac if we don't need to... ACK. (BTW, although I didn't have a need for this function when I suggested it, just today I was writing some code that will need it! :-)
/** * virNetDevGetIndex: diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 236cf83ef..01e9c5b95 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -157,6 +157,8 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) int virNetDevSetName(const char *ifname, const char *newifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+char *virNetDevGetName(int ifindex) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virNetDevGetIndex(const char *ifname, int *ifindex) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
Add a function getting the name of a network interface out of its index. --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 19 +++++++++++++++++++ src/util/virnetdev.h | 2 ++ 3 files changed, 22 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1f25e42d8..0fe88c3fa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1982,6 +1982,7 @@ virNetDevFeatureTypeToString; virNetDevGetFeatures; virNetDevGetIndex; virNetDevGetLinkInfo; +virNetDevGetName; virNetDevGetMAC; virNetDevGetMTU; virNetDevGetOnline;
Oops! I just now noticed (when I ran make check after adding this patch locally) that you have the ordering mixed up here - virNetDevGetName should be just after virNetDevGetMTU. Please fix that before pushing.
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index d12324878..91a5274aa 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -899,6 +899,25 @@ virNetDevGetRcvAllMulti(const char *ifname, return virNetDevGetIFFlag(ifname, VIR_IFF_ALLMULTI, receive); }
+char *virNetDevGetName(int ifindex) +{ + char name[IFNAMSIZ]; + char *ifname = NULL; + + memset(&name, 0, sizeof(name)); + + if (!if_indextoname(ifindex, name)) { + virReportSystemError(errno, + _("Failed to convert interface index %d to a name"), + ifindex); + goto cleanup; + } + + ignore_value(VIR_STRDUP(ifname, name)); + + cleanup: + return ifname; +}
/** * virNetDevGetIndex: diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 236cf83ef..01e9c5b95 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -157,6 +157,8 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) int virNetDevSetName(const char *ifname, const char *newifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+char *virNetDevGetName(int ifindex) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virNetDevGetIndex(const char *ifname, int *ifindex) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote: [...]
/** * virNetDevGetIndex: diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 236cf83ef..01e9c5b95 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -157,6 +157,8 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) int virNetDevSetName(const char *ifname, const char *newifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+char *virNetDevGetName(int ifindex) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
ATTRIBUTE_NONNULL should have been removed - (un)fortunately not seen in builds, but my Coverity build fails. John
int virNetDevGetIndex(const char *ifname, int *ifindex) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
When enabling IPv6 on all interfaces, we may get the host Router Advertisement routes discarded. To avoid this, the user needs to set accept_ra to 2 for the interfaces with such routes. See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt on this topic. To avoid user mistakenly losing routes on their hosts, check accept_ra values before enabling IPv6 forwarding. If a RA route is detected, but neither the corresponding device nor global accept_ra is set to 2, the network will fail to start. --- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 16 +++-- src/util/virnetdevip.c | 158 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevip.h | 1 + 4 files changed, 171 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0fe88c3fa..ec6553520 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2056,6 +2056,7 @@ virNetDevBridgeSetVlanFiltering; virNetDevIPAddrAdd; virNetDevIPAddrDel; virNetDevIPAddrGet; +virNetDevIPCheckIPv6Forwarding; virNetDevIPInfoAddToDev; virNetDevIPInfoClear; virNetDevIPRouteAdd; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3f6561055..d02cd19f9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -61,6 +61,7 @@ #include "virlog.h" #include "virdnsmasq.h" #include "configmake.h" +#include "virnetlink.h" #include "virnetdev.h" #include "virnetdevip.h" #include "virnetdevbridge.h" @@ -2377,11 +2378,16 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, } /* If forward.type != NONE, turn on global IP forwarding */ - if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE && - networkEnableIPForwarding(v4present, v6present) < 0) { - virReportSystemError(errno, "%s", - _("failed to enable IP forwarding")); - goto err3; + if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE) { + if (!virNetDevIPCheckIPv6Forwarding()) + goto err3; /* Precise error message already provided */ + + + if (networkEnableIPForwarding(v4present, v6present) < 0) { + virReportSystemError(errno, "%s", + _("failed to enable IP forwarding")); + goto err3; + } } diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 42fbba1eb..a4d382427 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -508,6 +508,158 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) return ret; } +static int +virNetDevIPGetAcceptRA(const char *ifname) +{ + char *path = NULL; + char *buf = NULL; + char *suffix; + int accept_ra = -1; + + if (virAsprintf(&path, "/proc/sys/net/ipv6/conf/%s/accept_ra", + ifname ? ifname : "all") < 0) + goto cleanup; + + if ((virFileReadAll(path, 512, &buf) < 0) || + (virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0)) + goto cleanup; + + cleanup: + VIR_FREE(path); + VIR_FREE(buf); + + return accept_ra; +} + +struct virNetDevIPCheckIPv6ForwardingData { + bool hasRARoutes; + + /* Devices with conflicting accept_ra */ + char **devices; + size_t ndevices; +}; + +static int +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, + void *opaque) +{ + struct rtmsg *rtmsg = NLMSG_DATA(resp); + int accept_ra = -1; + struct rtattr *rta; + char *ifname = NULL; + struct virNetDevIPCheckIPv6ForwardingData *data = opaque; + int ret = 0; + int len = RTM_PAYLOAD(resp); + int oif = -1; + + /* Ignore messages other than route ones */ + if (resp->nlmsg_type != RTM_NEWROUTE) + return ret; + + /* Extract a few attributes */ + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { + switch (rta->rta_type) { + case RTA_OIF: + oif = *(int *)RTA_DATA(rta); + + if (!(ifname = virNetDevGetName(oif))) + goto error; + break; + } + } + + /* No need to do anything else for non RA routes */ + if (rtmsg->rtm_protocol != RTPROT_RA) + goto cleanup; + + data->hasRARoutes = true; + + /* Check the accept_ra value for the interface */ + accept_ra = virNetDevIPGetAcceptRA(ifname); + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); + + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + goto error; + + cleanup: + VIR_FREE(ifname); + return ret; + + error: + ret = -1; + goto cleanup; +} + +bool +virNetDevIPCheckIPv6Forwarding(void) +{ + struct nl_msg *nlmsg = NULL; + bool valid = false; + struct rtgenmsg genmsg; + size_t i; + struct virNetDevIPCheckIPv6ForwardingData data = { + .hasRARoutes = false, + .devices = NULL, + .ndevices = 0 + }; + + + /* Prepare the request message */ + if (!(nlmsg = nlmsg_alloc_simple(RTM_GETROUTE, + NLM_F_REQUEST | NLM_F_DUMP))) { + virReportOOMError(); + goto cleanup; + } + + memset(&genmsg, 0, sizeof(genmsg)); + genmsg.rtgen_family = AF_INET6; + + if (nlmsg_append(nlmsg, &genmsg, sizeof(genmsg), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + goto cleanup; + } + + /* Send the request and loop over the responses */ + if (virNetlinkDumpCommand(nlmsg, virNetDevIPCheckIPv6ForwardingCallback, + 0, 0, NETLINK_ROUTE, 0, &data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to loop over IPv6 routes")); + goto cleanup; + } + + valid = !data.hasRARoutes || data.ndevices == 0; + + /* Check the global accept_ra if at least one isn't set on a + per-device basis */ + if (!valid && data.hasRARoutes) { + int accept_ra = virNetDevIPGetAcceptRA(NULL); + valid = accept_ra == 2; + VIR_DEBUG("Checked global accept_ra: %d", accept_ra); + } + + if (!valid) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (i = 0; i < data.ndevices; i++) { + virBufferAdd(&buf, data.devices[i], -1); + if (i < data.ndevices - 1) + virBufferAddLit(&buf, ", "); + } + + 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"), + virBufferCurrentContent(&buf)); + virBufferFreeAndReset(&buf); + } + + cleanup: + nlmsg_free(nlmsg); + for (i = 0; i < data.ndevices; i++) + VIR_FREE(data.devices[i]); + return valid; +} #else /* defined(__linux__) && defined(HAVE_LIBNL) */ @@ -655,6 +807,12 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs ATTRIBUTE_UNUSED, return -1; } +bool +virNetDevIPCheckIPv6Forwarding(void) +{ + VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled"); + return true; +} #endif /* defined(__linux__) && defined(HAVE_LIBNL) */ diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h index b7abdf94d..cc466ca25 100644 --- a/src/util/virnetdevip.h +++ b/src/util/virnetdevip.h @@ -83,6 +83,7 @@ int virNetDevIPAddrGet(const char *ifname, virSocketAddrPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) ATTRIBUTE_NONNULL(1); +bool virNetDevIPCheckIPv6Forwarding(void); /* virNetDevIPRoute object */ void virNetDevIPRouteFree(virNetDevIPRoutePtr def); -- 2.11.0
On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
When enabling IPv6 on all interfaces, we may get the host Router Advertisement routes discarded. To avoid this, the user needs to set accept_ra to 2 for the interfaces with such routes.
See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt on this topic.
To avoid user mistakenly losing routes on their hosts, check accept_ra values before enabling IPv6 forwarding. If a RA route is detected, but neither the corresponding device nor global accept_ra is set to 2, the network will fail to start.
Since I already asked the question and didn't hear a positive response, I'm guessing "no news is bad news", i.e. there isn't a reliable way to fix this problem automatically. Assuming that, reporting the problem and failing seems like the next best (least worse) thing...
--- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 16 +++-- src/util/virnetdevip.c | 158 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevip.h | 1 + 4 files changed, 171 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0fe88c3fa..ec6553520 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2056,6 +2056,7 @@ virNetDevBridgeSetVlanFiltering; virNetDevIPAddrAdd; virNetDevIPAddrDel; virNetDevIPAddrGet; +virNetDevIPCheckIPv6Forwarding; virNetDevIPInfoAddToDev; virNetDevIPInfoClear; virNetDevIPRouteAdd; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3f6561055..d02cd19f9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -61,6 +61,7 @@ #include "virlog.h" #include "virdnsmasq.h" #include "configmake.h" +#include "virnetlink.h" #include "virnetdev.h" #include "virnetdevip.h" #include "virnetdevbridge.h" @@ -2377,11 +2378,16 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, }
/* If forward.type != NONE, turn on global IP forwarding */ - if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE && - networkEnableIPForwarding(v4present, v6present) < 0) { - virReportSystemError(errno, "%s", - _("failed to enable IP forwarding")); - goto err3; + if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE) { + if (!virNetDevIPCheckIPv6Forwarding()) + goto err3; /* Precise error message already provided */ + + + if (networkEnableIPForwarding(v4present, v6present) < 0) { + virReportSystemError(errno, "%s", + _("failed to enable IP forwarding")); + goto err3; + } }
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 42fbba1eb..a4d382427 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -508,6 +508,158 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) return ret; }
+static int +virNetDevIPGetAcceptRA(const char *ifname) +{ + char *path = NULL; + char *buf = NULL; + char *suffix; + int accept_ra = -1; + + if (virAsprintf(&path, "/proc/sys/net/ipv6/conf/%s/accept_ra", + ifname ? ifname : "all") < 0) + goto cleanup; + + if ((virFileReadAll(path, 512, &buf) < 0) || + (virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0)) + goto cleanup; + + cleanup: + VIR_FREE(path); + VIR_FREE(buf); + + return accept_ra; +} + +struct virNetDevIPCheckIPv6ForwardingData { + bool hasRARoutes; + + /* Devices with conflicting accept_ra */ + char **devices; + size_t ndevices; +}; + +static int +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, + void *opaque) +{ + struct rtmsg *rtmsg = NLMSG_DATA(resp); + int accept_ra = -1; + struct rtattr *rta; + char *ifname = NULL; + struct virNetDevIPCheckIPv6ForwardingData *data = opaque; + int ret = 0; + int len = RTM_PAYLOAD(resp); + int oif = -1; + + /* Ignore messages other than route ones */ + if (resp->nlmsg_type != RTM_NEWROUTE) + return ret; + + /* Extract a few attributes */ + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { + switch (rta->rta_type) { + case RTA_OIF: + oif = *(int *)RTA_DATA(rta); + + if (!(ifname = virNetDevGetName(oif))) + goto error; + break; + } + } + + /* No need to do anything else for non RA routes */ + if (rtmsg->rtm_protocol != RTPROT_RA) + goto cleanup; + + data->hasRARoutes = true; + + /* Check the accept_ra value for the interface */ + accept_ra = virNetDevIPGetAcceptRA(ifname); + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); + + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + goto error; + + cleanup: + VIR_FREE(ifname); + return ret; + + error: + ret = -1; + goto cleanup; +} + +bool +virNetDevIPCheckIPv6Forwarding(void) +{ + struct nl_msg *nlmsg = NULL; + bool valid = false; + struct rtgenmsg genmsg; + size_t i; + struct virNetDevIPCheckIPv6ForwardingData data = { + .hasRARoutes = false, + .devices = NULL, + .ndevices = 0 + }; + + + /* Prepare the request message */ + if (!(nlmsg = nlmsg_alloc_simple(RTM_GETROUTE, + NLM_F_REQUEST | NLM_F_DUMP))) { + virReportOOMError(); + goto cleanup; + } + + memset(&genmsg, 0, sizeof(genmsg)); + genmsg.rtgen_family = AF_INET6; + + if (nlmsg_append(nlmsg, &genmsg, sizeof(genmsg), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + goto cleanup; + } + + /* Send the request and loop over the responses */ + if (virNetlinkDumpCommand(nlmsg, virNetDevIPCheckIPv6ForwardingCallback, + 0, 0, NETLINK_ROUTE, 0, &data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to loop over IPv6 routes")); + goto cleanup; + } + + valid = !data.hasRARoutes || data.ndevices == 0; + + /* Check the global accept_ra if at least one isn't set on a + per-device basis */ + if (!valid && data.hasRARoutes) { + int accept_ra = virNetDevIPGetAcceptRA(NULL); + valid = accept_ra == 2; + VIR_DEBUG("Checked global accept_ra: %d", accept_ra); + } + + if (!valid) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (i = 0; i < data.ndevices; i++) { + virBufferAdd(&buf, data.devices[i], -1); + if (i < data.ndevices - 1) + virBufferAddLit(&buf, ", "); + } + + 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"), + virBufferCurrentContent(&buf)); + virBufferFreeAndReset(&buf); + } + + cleanup: + nlmsg_free(nlmsg); + for (i = 0; i < data.ndevices; i++) + VIR_FREE(data.devices[i]); + return valid; +}
#else /* defined(__linux__) && defined(HAVE_LIBNL) */
@@ -655,6 +807,12 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs ATTRIBUTE_UNUSED, return -1; }
+bool +virNetDevIPCheckIPv6Forwarding(void) +{ + VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled"); + return true; +}
Thanks for remembering this stub (I usually forget it).
#endif /* defined(__linux__) && defined(HAVE_LIBNL) */
diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h index b7abdf94d..cc466ca25 100644 --- a/src/util/virnetdevip.h +++ b/src/util/virnetdevip.h @@ -83,6 +83,7 @@ int virNetDevIPAddrGet(const char *ifname, virSocketAddrPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) ATTRIBUTE_NONNULL(1); +bool virNetDevIPCheckIPv6Forwarding(void);
/* virNetDevIPRoute object */ void virNetDevIPRouteFree(virNetDevIPRoutePtr def);
ACK. Nicely done! +++ Would review again :-)
On Thu, 2017-03-16 at 21:12 -0400, Laine Stump wrote:
On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
When enabling IPv6 on all interfaces, we may get the host Router Advertisement routes discarded. To avoid this, the user needs to set accept_ra to 2 for the interfaces with such routes.
See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt on this topic.
To avoid user mistakenly losing routes on their hosts, check accept_ra values before enabling IPv6 forwarding. If a RA route is detected, but neither the corresponding device nor global accept_ra is set to 2, the network will fail to start.
Since I already asked the question and didn't hear a positive response, I'm guessing "no news is bad news", i.e. there isn't a reliable way to fix this problem automatically. Assuming that, reporting the problem and failing seems like the next best (least worse) thing...
The only automatic thing that we could do is remember the RA routes that are set before enabling ipv6 forwarding and reset them after having lost them. IMHO automatically changing the accept_ra parameter for the user could lead to unknown (possibly buggy) cases. Before this patch, the network is started, but the host may loose routes (the default one in my case). After, it keeps the host routes, fails to start the network and tells the user to fix his host network config. -- Cedric
--- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 16 +++-- src/util/virnetdevip.c | 158 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevip.h | 1 + 4 files changed, 171 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0fe88c3fa..ec6553520 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2056,6 +2056,7 @@ virNetDevBridgeSetVlanFiltering; virNetDevIPAddrAdd; virNetDevIPAddrDel; virNetDevIPAddrGet; +virNetDevIPCheckIPv6Forwarding; virNetDevIPInfoAddToDev; virNetDevIPInfoClear; virNetDevIPRouteAdd; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3f6561055..d02cd19f9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -61,6 +61,7 @@ #include "virlog.h" #include "virdnsmasq.h" #include "configmake.h" +#include "virnetlink.h" #include "virnetdev.h" #include "virnetdevip.h" #include "virnetdevbridge.h" @@ -2377,11 +2378,16 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, } /* If forward.type != NONE, turn on global IP forwarding */ - if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE && - networkEnableIPForwarding(v4present, v6present) < 0) { - virReportSystemError(errno, "%s", - _("failed to enable IP forwarding")); - goto err3; + if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE) { + if (!virNetDevIPCheckIPv6Forwarding()) + goto err3; /* Precise error message already provided */ + + + if (networkEnableIPForwarding(v4present, v6present) < 0) { + virReportSystemError(errno, "%s", + _("failed to enable IP forwarding")); + goto err3; + } } diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 42fbba1eb..a4d382427 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -508,6 +508,158 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) return ret; } +static int +virNetDevIPGetAcceptRA(const char *ifname) +{ + char *path = NULL; + char *buf = NULL; + char *suffix; + int accept_ra = -1; + + if (virAsprintf(&path, "/proc/sys/net/ipv6/conf/%s/accept_ra", + ifname ? ifname : "all") < 0) + goto cleanup; + + if ((virFileReadAll(path, 512, &buf) < 0) || + (virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0)) + goto cleanup; + + cleanup: + VIR_FREE(path); + VIR_FREE(buf); + + return accept_ra; +} + +struct virNetDevIPCheckIPv6ForwardingData { + bool hasRARoutes; + + /* Devices with conflicting accept_ra */ + char **devices; + size_t ndevices; +}; + +static int +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, + void *opaque) +{ + struct rtmsg *rtmsg = NLMSG_DATA(resp); + int accept_ra = -1; + struct rtattr *rta; + char *ifname = NULL; + struct virNetDevIPCheckIPv6ForwardingData *data = opaque; + int ret = 0; + int len = RTM_PAYLOAD(resp); + int oif = -1; + + /* Ignore messages other than route ones */ + if (resp->nlmsg_type != RTM_NEWROUTE) + return ret; + + /* Extract a few attributes */ + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { + switch (rta->rta_type) { + case RTA_OIF: + oif = *(int *)RTA_DATA(rta); + + if (!(ifname = virNetDevGetName(oif))) + goto error; + break; + } + } + + /* No need to do anything else for non RA routes */ + if (rtmsg->rtm_protocol != RTPROT_RA) + goto cleanup; + + data->hasRARoutes = true; + + /* Check the accept_ra value for the interface */ + accept_ra = virNetDevIPGetAcceptRA(ifname); + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); + + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + goto error; + + cleanup: + VIR_FREE(ifname); + return ret; + + error: + ret = -1; + goto cleanup; +} + +bool +virNetDevIPCheckIPv6Forwarding(void) +{ + struct nl_msg *nlmsg = NULL; + bool valid = false; + struct rtgenmsg genmsg; + size_t i; + struct virNetDevIPCheckIPv6ForwardingData data = { + .hasRARoutes = false, + .devices = NULL, + .ndevices = 0 + }; + + + /* Prepare the request message */ + if (!(nlmsg = nlmsg_alloc_simple(RTM_GETROUTE, + NLM_F_REQUEST | NLM_F_DUMP))) { + virReportOOMError(); + goto cleanup; + } + + memset(&genmsg, 0, sizeof(genmsg)); + genmsg.rtgen_family = AF_INET6; + + if (nlmsg_append(nlmsg, &genmsg, sizeof(genmsg), NLMSG_ALIGNTO) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + goto cleanup; + } + + /* Send the request and loop over the responses */ + if (virNetlinkDumpCommand(nlmsg, virNetDevIPCheckIPv6ForwardingCallback, + 0, 0, NETLINK_ROUTE, 0, &data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to loop over IPv6 routes")); + goto cleanup; + } + + valid = !data.hasRARoutes || data.ndevices == 0; + + /* Check the global accept_ra if at least one isn't set on a + per-device basis */ + if (!valid && data.hasRARoutes) { + int accept_ra = virNetDevIPGetAcceptRA(NULL); + valid = accept_ra == 2; + VIR_DEBUG("Checked global accept_ra: %d", accept_ra); + } + + if (!valid) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (i = 0; i < data.ndevices; i++) { + virBufferAdd(&buf, data.devices[i], -1); + if (i < data.ndevices - 1) + virBufferAddLit(&buf, ", "); + } + + 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"), + virBufferCurrentContent(&buf)); + virBufferFreeAndReset(&buf); + } + + cleanup: + nlmsg_free(nlmsg); + for (i = 0; i < data.ndevices; i++) + VIR_FREE(data.devices[i]); + return valid; +} #else /* defined(__linux__) && defined(HAVE_LIBNL) */ @@ -655,6 +807,12 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs ATTRIBUTE_UNUSED, return -1; } +bool +virNetDevIPCheckIPv6Forwarding(void) +{ + VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled"); + return true; +}
Thanks for remembering this stub (I usually forget it).
#endif /* defined(__linux__) && defined(HAVE_LIBNL) */ diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h index b7abdf94d..cc466ca25 100644 --- a/src/util/virnetdevip.h +++ b/src/util/virnetdevip.h @@ -83,6 +83,7 @@ int virNetDevIPAddrGet(const char *ifname, virSocketAddrPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count) ATTRIBUTE_NONNULL(1); +bool virNetDevIPCheckIPv6Forwarding(void); /* virNetDevIPRoute object */ void virNetDevIPRouteFree(virNetDevIPRoutePtr def);
ACK. Nicely done! +++ Would review again :-)
[...]
+ +static int +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, + void *opaque) +{ + struct rtmsg *rtmsg = NLMSG_DATA(resp); + int accept_ra = -1; + struct rtattr *rta; + char *ifname = NULL; + struct virNetDevIPCheckIPv6ForwardingData *data = opaque; + int ret = 0; + int len = RTM_PAYLOAD(resp); + int oif = -1; + + /* Ignore messages other than route ones */ + if (resp->nlmsg_type != RTM_NEWROUTE) + return ret; + + /* Extract a few attributes */ + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { + switch (rta->rta_type) { + case RTA_OIF: + oif = *(int *)RTA_DATA(rta); + + if (!(ifname = virNetDevGetName(oif))) + goto error; + break;
Did you really mean to break from the for loop if ifname is set? This breaks only from the switch/case. Of course Coverity doesn't know much more than you'd be going back to the top of the for loop and could overwrite ifname again. It proclaims a resource leak... John
+ } + } + + /* No need to do anything else for non RA routes */ + if (rtmsg->rtm_protocol != RTPROT_RA) + goto cleanup; + + data->hasRARoutes = true; + + /* Check the accept_ra value for the interface */ + accept_ra = virNetDevIPGetAcceptRA(ifname); + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); + + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + goto error; + + cleanup: + VIR_FREE(ifname); + return ret; + + error: + ret = -1; + goto cleanup; +}
On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
[...]
+ +static int +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, + void *opaque) +{ + struct rtmsg *rtmsg = NLMSG_DATA(resp); + int accept_ra = -1; + struct rtattr *rta; + char *ifname = NULL; + struct virNetDevIPCheckIPv6ForwardingData *data = opaque; + int ret = 0; + int len = RTM_PAYLOAD(resp); + int oif = -1; + + /* Ignore messages other than route ones */ + if (resp->nlmsg_type != RTM_NEWROUTE) + return ret; + + /* Extract a few attributes */ + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { + switch (rta->rta_type) { + case RTA_OIF: + oif = *(int *)RTA_DATA(rta); + + if (!(ifname = virNetDevGetName(oif))) + goto error; + break;
Did you really mean to break from the for loop if ifname is set? This breaks only from the switch/case. Of course Coverity doesn't know much more than you'd be going back to the top of the for loop and could overwrite ifname again. It proclaims a resource leak...
In my dev version I was also getting the RTA_DST attribute to print some debugging message that I removed in the submitted version. The break was here between the switch cases. In the current case I don't really care if we break out of the loop or not. As there aren't that many attributes in an rtnetlink message to loop over, breaking wouldn't gain a lot of cycles. What I don't get though is what this break is actually doing? isn't it for the switch case even though there is no other case after it? -- Cedric
John
+ } + } + + /* No need to do anything else for non RA routes */ + if (rtmsg->rtm_protocol != RTPROT_RA) + goto cleanup; + + data->hasRARoutes = true; + + /* Check the accept_ra value for the interface */ + accept_ra = virNetDevIPGetAcceptRA(ifname); + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); + + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + goto error; + + cleanup: + VIR_FREE(ifname); + return ret; + + error: + ret = -1; + goto cleanup; +}
On 03/23/2017 04:44 AM, Cedric Bosdonnat wrote:
On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
[...]
+ +static int +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, + void *opaque) +{ + struct rtmsg *rtmsg = NLMSG_DATA(resp); + int accept_ra = -1; + struct rtattr *rta; + char *ifname = NULL; + struct virNetDevIPCheckIPv6ForwardingData *data = opaque; + int ret = 0; + int len = RTM_PAYLOAD(resp); + int oif = -1; + + /* Ignore messages other than route ones */ + if (resp->nlmsg_type != RTM_NEWROUTE) + return ret; + + /* Extract a few attributes */ + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { + switch (rta->rta_type) { + case RTA_OIF: + oif = *(int *)RTA_DATA(rta); + + if (!(ifname = virNetDevGetName(oif))) + goto error; + break;
Did you really mean to break from the for loop if ifname is set? This breaks only from the switch/case. Of course Coverity doesn't know much more than you'd be going back to the top of the for loop and could overwrite ifname again. It proclaims a resource leak...
In my dev version I was also getting the RTA_DST attribute to print some debugging message that I removed in the submitted version. The break was here between the switch cases.
In the current case I don't really care if we break out of the loop or not. As there aren't that many attributes in an rtnetlink message to loop over, breaking wouldn't gain a lot of cycles.
What I don't get though is what this break is actually doing? isn't it for the switch case even though there is no other case after it?
-- Cedric
So should this be changed to: if (rta->rta_type == RTA_OIF) { oif = *(int *)RTA_DATA(rta); if (!(ifname = virNetDevGetName(oif))) goto error; break; } leaving two questions in my mind 1. Can there be more than one RTA_OIF 2. If we don't finish the loop (e.g. we break out), then does one of the subsequent checks fail? Is it more of a problem that we find *two* RTA_OIF's and thus should add a: if (ifname) { VIR_DEBUG("some sort of message"); goto error; } prior to the virNetDevGetName call and then remove the break;? John (who doesn't know the answer!) BTW: The issue from patch4 is resolved by my 22 patch bomb from yesterday
John
+ } + } + + /* No need to do anything else for non RA routes */ + if (rtmsg->rtm_protocol != RTPROT_RA) + goto cleanup; + + data->hasRARoutes = true; + + /* Check the accept_ra value for the interface */ + accept_ra = virNetDevIPGetAcceptRA(ifname); + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); + + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + goto error; + + cleanup: + VIR_FREE(ifname); + return ret; + + error: + ret = -1; + goto cleanup; +}
On Thu, 2017-03-23 at 12:09 -0400, John Ferlan wrote:
On 03/23/2017 04:44 AM, Cedric Bosdonnat wrote:
On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
[...]
+ +static int +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, + void *opaque) +{ + struct rtmsg *rtmsg = NLMSG_DATA(resp); + int accept_ra = -1; + struct rtattr *rta; + char *ifname = NULL; + struct virNetDevIPCheckIPv6ForwardingData *data = opaque; + int ret = 0; + int len = RTM_PAYLOAD(resp); + int oif = -1; + + /* Ignore messages other than route ones */ + if (resp->nlmsg_type != RTM_NEWROUTE) + return ret; + + /* Extract a few attributes */ + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { + switch (rta->rta_type) { + case RTA_OIF: + oif = *(int *)RTA_DATA(rta); + + if (!(ifname = virNetDevGetName(oif))) + goto error; + break;
Did you really mean to break from the for loop if ifname is set? This breaks only from the switch/case. Of course Coverity doesn't know much more than you'd be going back to the top of the for loop and could overwrite ifname again. It proclaims a resource leak...
In my dev version I was also getting the RTA_DST attribute to print some debugging message that I removed in the submitted version. The break was here between the switch cases.
In the current case I don't really care if we break out of the loop or not. As there aren't that many attributes in an rtnetlink message to loop over, breaking wouldn't gain a lot of cycles.
What I don't get though is what this break is actually doing? isn't it for the switch case even though there is no other case after it?
-- Cedric
So should this be changed to:
if (rta->rta_type == RTA_OIF) { oif = *(int *)RTA_DATA(rta);
if (!(ifname = virNetDevGetName(oif))) goto error; break; }
leaving two questions in my mind
1. Can there be more than one RTA_OIF 2. If we don't finish the loop (e.g. we break out), then does one of the subsequent checks fail?
IMHO getting more than one RTA_OIF per message would be a bug from the kernel: a route is associated to one device. Think of these messages like the lines printed by 'ip r'
Is it more of a problem that we find *two* RTA_OIF's and thus should add a:
if (ifname) { VIR_DEBUG("some sort of message"); goto error; }
prior to the virNetDevGetName call and then remove the break;?
I can remove the break without that for sure.
John (who doesn't know the answer!)
BTW: The issue from patch4 is resolved by my 22 patch bomb from yesterday
Yep, seen that one: thanks a lot -- Cedric
John
+ } + } + + /* No need to do anything else for non RA routes */ + if (rtmsg->rtm_protocol != RTPROT_RA) + goto cleanup; + + data->hasRARoutes = true; + + /* Check the accept_ra value for the interface */ + accept_ra = virNetDevIPGetAcceptRA(ifname); + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); + + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + goto error; + + cleanup: + VIR_FREE(ifname); + return ret; + + error: + ret = -1; + goto cleanup; +}
Hi Cédric, I have tested it, it works well. But the interface name will repeat 2 times. Please help to confirm this, and if below test for a single port host is enough? # cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra 1 enable network default with ipv6 ip section # virsh net-start default error: Failed to start network default error: 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: enp0s25, enp0s25 # echo 2 > /proc/sys/net/ipv6/conf/enp0s25/accept_ra # virsh net-start default Network default started try create: # virsh net-create default.xml error: Failed to create network from default.xml error: 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: enp0s25, enp0s25 On Wed, Mar 15, 2017 at 10:45 PM, Cédric Bosdonnat <cbosdonnat@suse.com> wrote:
Hi Laine, all,
Here is the v2 of my series. The changes are:
* Add a commit to create a virNetDevGetName() function * Fix Laine's comments
Cédric Bosdonnat (5): util: extract the request sending code from virNetlinkCommand() util: add virNetlinkDumpCommand() bridge_driver.c: more uses of SYSCTL_PATH util: add virNetDevGetName() function network: check accept_ra before enabling ipv6 forwarding
src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 25 ++++--- src/util/virnetdev.c | 19 ++++++ src/util/virnetdev.h | 2 + src/util/virnetdevip.c | 158 ++++++++++++++++++++++++++++++ ++++++++++++++ src/util/virnetdevip.h | 1 + src/util/virnetlink.c | 145 ++++++++++++++++++++++++++++++ ---------- src/util/virnetlink.h | 9 +++ 8 files changed, 319 insertions(+), 43 deletions(-)
-- 2.11.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Best Regards, Yalan Zhang IRC: yalzhang Internal phone: 8389413
Yalan 你好 On Mon, 2017-04-17 at 17:30 +0800, Yalan Zhang wrote:
I have tested it, it works well. But the interface name will repeat 2 times. Please help to confirm this, and if below test for a single port host is enough?
# cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra 1
enable network default with ipv6 ip section
# virsh net-start default error: Failed to start network default error: 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: enp0s25, enp0s25
Just to help me confirm my intuition: do you have several RA routes defined for the same device?
# echo 2 > /proc/sys/net/ipv6/conf/enp0s25/accept_ra
# virsh net-start default Network default started
try create:
# virsh net-create default.xml error: Failed to create network from default.xml error: 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: enp0s25, enp0s25
This one sounds weird: if the accept_ra is set to 2 as you report you did, you shouldn't get that error. -- Cedric
On Wed, Mar 15, 2017 at 10:45 PM, Cédric Bosdonnat <cbosdonnat@suse.com> wrote:
Hi Laine, all,
Here is the v2 of my series. The changes are:
* Add a commit to create a virNetDevGetName() function * Fix Laine's comments
Cédric Bosdonnat (5): util: extract the request sending code from virNetlinkCommand() util: add virNetlinkDumpCommand() bridge_driver.c: more uses of SYSCTL_PATH util: add virNetDevGetName() function network: check accept_ra before enabling ipv6 forwarding
src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 25 ++++--- src/util/virnetdev.c | 19 ++++++ src/util/virnetdev.h | 2 + src/util/virnetdevip.c | 158 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevip.h | 1 + src/util/virnetlink.c | 145 ++++++++++++++++++++++++++++++---------- src/util/virnetlink.h | 9 +++ 8 files changed, 319 insertions(+), 43 deletions(-)
-- 2.11.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi Cédric, 您好 ：） I'm sorry that I missed the mail. But currently I can not reproduce it. For the error by net-create, it is executed when I set accept_ra to 1. I have just test on libvirt-3.2.0-4.el7.x86_64, the behavior changes, it seems like there is no check for accept_ra before start a network with ipv6. 1. define and start a network with ipv6 settings # virsh net-dumpxml default6 <network> <name>default6</name> <uuid>c502d02c-fbd0-49d9-91e4-0fcf0ef159d0</uuid> <forward mode='nat'/> <bridge name='virbr4' stp='on' delay='0'/> <mac address='52:54:00:04:d5:3c'/> <ip address='192.168.10.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.10.2' end='192.168.10.254'/> </dhcp> </ip> <ip family='ipv6' address='2001:db8:ca2:2::1' prefix='64'> <dhcp> <range start='2001:db8:ca2:2:1::10' end='2001:db8:ca2:2:1::ff'/> </dhcp> </ip> </network> # cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra 1 # virsh net-start default6 =====> the network can start as well with accept_ra=1 Network default6 started It seems that the "virNetDevIPGetAcceptRA()" in patch "network: check accept_ra before enabling ipv6 forwarding" with commit 00d28a78 is not executed when I start a network. Please help to check, Thank you. Best Regards, Yalan Zhang IRC: yalzhang Internal phone: 8389413 On Tue, Apr 18, 2017 at 5:54 PM, Cedric Bosdonnat <cbosdonnat@suse.com> wrote:
Yalan 你好
On Mon, 2017-04-17 at 17:30 +0800, Yalan Zhang wrote:
I have tested it, it works well. But the interface name will repeat 2 times. Please help to confirm this, and if below test for a single port host is enough?
# cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra 1
enable network default with ipv6 ip section
# virsh net-start default error: Failed to start network default error: 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: enp0s25, enp0s25
Just to help me confirm my intuition: do you have several RA routes defined for the same device?
# echo 2 > /proc/sys/net/ipv6/conf/enp0s25/accept_ra
# virsh net-start default Network default started
try create:
# virsh net-create default.xml error: Failed to create network from default.xml error: 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: enp0s25, enp0s25
This one sounds weird: if the accept_ra is set to 2 as you report you did, you shouldn't get that error.
-- Cedric
On Wed, Mar 15, 2017 at 10:45 PM, Cédric Bosdonnat <cbosdonnat@suse.com> wrote:
Hi Laine, all,
Here is the v2 of my series. The changes are:
* Add a commit to create a virNetDevGetName() function * Fix Laine's comments
Cédric Bosdonnat (5): util: extract the request sending code from virNetlinkCommand() util: add virNetlinkDumpCommand() bridge_driver.c: more uses of SYSCTL_PATH util: add virNetDevGetName() function network: check accept_ra before enabling ipv6 forwarding
src/libvirt_private.syms | 3 + src/network/bridge_driver.c | 25 ++++--- src/util/virnetdev.c | 19 ++++++ src/util/virnetdev.h | 2 + src/util/virnetdevip.c | 158 ++++++++++++++++++++++++++++++ ++++++++++++++ src/util/virnetdevip.h | 1 + src/util/virnetlink.c | 145 ++++++++++++++++++++++++++++++
src/util/virnetlink.h | 9 +++ 8 files changed, 319 insertions(+), 43 deletions(-)
-- 2.11.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2017-05-10 at 13:30 +0800, Yalan Zhang wrote:
I'm sorry that I missed the mail.
没关系
But currently I can not reproduce it. For the error by net-create, it is executed when I set accept_ra to 1.
That sounds more normal. net-create and net-start are triggering the same code in the end.
I have just test on libvirt-3.2.0-4.el7.x86_64, the behavior changes, it seems like there is no check for accept_ra before start a network with ipv6.
1. define and start a network with ipv6 settings # virsh net-dumpxml default6 <network> <name>default6</name> <uuid>c502d02c-fbd0-49d9-91e4-0fcf0ef159d0</uuid> <forward mode='nat'/> <bridge name='virbr4' stp='on' delay='0'/> <mac address='52:54:00:04:d5:3c'/> <ip address='192.168.10.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.10.2' end='192.168.10.254'/> </dhcp> </ip> <ip family='ipv6' address='2001:db8:ca2:2::1' prefix='64'> <dhcp> <range start='2001:db8:ca2:2:1::10' end='2001:db8:ca2:2:1::ff'/> </dhcp> </ip> </network>
# cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra 1
# virsh net-start default6 =====> the network can start as well with accept_ra=1 Network default6 started
It seems that the "virNetDevIPGetAcceptRA()" in patch "network: check accept_ra before enabling ipv6 forwarding" with commit 00d28a78 is not executed when I start a network. Please help to check, Thank you.
It won't complain at all if there is no RA route set on the host. To reproduce, you need to setup a machine acting as an ipv6 router with radvd on the guest network. Do you actually have an RA route for the enp0s25 device? You can check it by running `ip -6 r`. These routes are indicated with 'proto ra' -- Cedric
I have no RA route set. I will try, Thank you very much! Best Regards, Yalan Zhang IRC: yalzhang Internal phone: 8389413 On Wed, May 10, 2017 at 3:34 PM, Cedric Bosdonnat <cbosdonnat@suse.com> wrote:
On Wed, 2017-05-10 at 13:30 +0800, Yalan Zhang wrote:
I'm sorry that I missed the mail.
没关系
But currently I can not reproduce it. For the error by net-create, it is executed when I set accept_ra to 1.
That sounds more normal. net-create and net-start are triggering the same code in the end.
I have just test on libvirt-3.2.0-4.el7.x86_64, the behavior changes, it seems like there is no check for accept_ra before start a network with ipv6.
1. define and start a network with ipv6 settings # virsh net-dumpxml default6 <network> <name>default6</name> <uuid>c502d02c-fbd0-49d9-91e4-0fcf0ef159d0</uuid> <forward mode='nat'/> <bridge name='virbr4' stp='on' delay='0'/> <mac address='52:54:00:04:d5:3c'/> <ip address='192.168.10.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.10.2' end='192.168.10.254'/> </dhcp> </ip> <ip family='ipv6' address='2001:db8:ca2:2::1' prefix='64'> <dhcp> <range start='2001:db8:ca2:2:1::10' end='2001:db8:ca2:2:1::ff'/> </dhcp> </ip> </network>
# cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra 1
# virsh net-start default6 =====> the network can start as well with accept_ra=1 Network default6 started
It seems that the "virNetDevIPGetAcceptRA()" in patch "network: check accept_ra before enabling ipv6 forwarding" with commit 00d28a78 is not executed when I start a network. Please help to check, Thank you.
It won't complain at all if there is no RA route set on the host. To reproduce, you need to setup a machine acting as an ipv6 router with radvd on the guest network.
Do you actually have an RA route for the enp0s25 device? You can check it by running `ip -6 r`. These routes are indicated with 'proto ra'
-- Cedric
Hi Cédric, I think I find the machine with a RA route. (It is the original machine in the first mail) # ip a show enp0s25 2: enp0s25: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000 link/ether 00:24:7e:05:42:32 brd ff:ff:ff:ff:ff:ff inet 10.66.71.67/23 brd 10.66.71.255 scope global dynamic enp0s25 valid_lft 85595sec preferred_lft 85595sec inet6 2620:52:0:4246:224:7eff:fe05:4232/64 scope global mngtmpaddr dynamic valid_lft 2591915sec preferred_lft 604715sec inet6 fe80::224:7eff:fe05:4232/64 scope link valid_lft forever preferred_lft forever # ip -6 r unreachable ::/96 dev lo metric 1024 error -113 unreachable ::ffff:0.0.0.0/96 dev lo metric 1024 error -113 unreachable 2002:a00::/24 dev lo metric 1024 error -113 unreachable 2002:7f00::/24 dev lo metric 1024 error -113 unreachable 2002:a9fe::/32 dev lo metric 1024 error -113 unreachable 2002:ac10::/28 dev lo metric 1024 error -113 unreachable 2002:c0a8::/32 dev lo metric 1024 error -113 unreachable 2002:e000::/19 dev lo metric 1024 error -113 2620:52:0:4246::/64 dev enp0s25 proto kernel metric 256 expires 2591970sec unreachable 3ffe:ffff::/32 dev lo metric 1024 error -113 fe80::/64 dev enp0s25 proto kernel metric 256 default via fe80::26e9:b3ff:fe23:44cd dev enp0s25 proto ra metric 1024 expires 1770sec hoplimit 64 default via fe80::26e9:b3ff:fe0f:654d dev enp0s25 proto ra metric 1024 expires 1657sec hoplimit 64 I think it is because there is 2 items for the single interface enp0s25. And I don't know why there are 2 link local address. Could you please help? Thank you~ Best Regards, Yalan Zhang IRC: yalzhang Internal phone: 8389413 On Wed, May 10, 2017 at 3:41 PM, Yalan Zhang <yalzhang@redhat.com> wrote:
I have no RA route set. I will try, Thank you very much!
Best Regards, Yalan Zhang IRC: yalzhang Internal phone: 8389413
On Wed, May 10, 2017 at 3:34 PM, Cedric Bosdonnat <cbosdonnat@suse.com> wrote:
On Wed, 2017-05-10 at 13:30 +0800, Yalan Zhang wrote:
I'm sorry that I missed the mail.
没关系
But currently I can not reproduce it. For the error by net-create, it is executed when I set accept_ra to 1.
That sounds more normal. net-create and net-start are triggering the same code in the end.
I have just test on libvirt-3.2.0-4.el7.x86_64, the behavior changes, it seems like there is no check for accept_ra before start a network with ipv6.
1. define and start a network with ipv6 settings # virsh net-dumpxml default6 <network> <name>default6</name> <uuid>c502d02c-fbd0-49d9-91e4-0fcf0ef159d0</uuid> <forward mode='nat'/> <bridge name='virbr4' stp='on' delay='0'/> <mac address='52:54:00:04:d5:3c'/> <ip address='192.168.10.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.10.2' end='192.168.10.254'/> </dhcp> </ip> <ip family='ipv6' address='2001:db8:ca2:2::1' prefix='64'> <dhcp> <range start='2001:db8:ca2:2:1::10' end='2001:db8:ca2:2:1::ff'/> </dhcp> </ip> </network>
# cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra 1
# virsh net-start default6 =====> the network can start as well with accept_ra=1 Network default6 started
It seems that the "virNetDevIPGetAcceptRA()" in patch "network: check accept_ra before enabling ipv6 forwarding" with commit 00d28a78 is not executed when I start a network. Please help to check, Thank you.
It won't complain at all if there is no RA route set on the host. To reproduce, you need to setup a machine acting as an ipv6 router with radvd on the guest network.
Do you actually have an RA route for the enp0s25 device? You can check it by running `ip -6 r`. These routes are indicated with 'proto ra'
-- Cedric
participants (5)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
John Ferlan -
Laine Stump -
Yalan Zhang