9:45 a.m.
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
9:45 a.m.
New subject: [libvirt] [PATCH v2 1/5] util: extract the request sending code from virNetlinkCommand()
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
5:55 p.m.
New subject: [libvirt] [PATCH v2 1/5] util: extract the request sending code from virNetlinkCommand()
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.
9:45 a.m.
New subject: [libvirt] [PATCH v2 2/5] util: add virNetlinkDumpCommand()
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
6 p.m.
New subject: [libvirt] [PATCH v2 2/5] util: add virNetlinkDumpCommand()
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);
9:45 a.m.
New subject: [libvirt] [PATCH v2 3/5] bridge_driver.c: more uses of SYSCTL_PATH
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
6 p.m.
New subject: [libvirt] [PATCH v2 3/5] bridge_driver.c: more uses of SYSCTL_PATH
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.
9:45 a.m.
New subject: [libvirt] [PATCH v2 4/5] util: add virNetDevGetName() function
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
6:04 p.m.
New subject: [libvirt] [PATCH v2 4/5] util: add virNetDevGetName() function
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;
11:05 a.m.
New subject: [libvirt] [PATCH v2 4/5] util: add virNetDevGetName() function
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;
6:14 a.m.
New subject: [libvirt] [PATCH v2 4/5] util: add virNetDevGetName() function
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;
9:45 a.m.
New subject: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
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
8:12 p.m.
New subject: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
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 :-)
5:19 a.m.
New subject: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
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 :-)
6:16 a.m.
New subject: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
[...]
+
+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;
+}
3:44 a.m.
New subject: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
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;
> +}
11:09 a.m.
New subject: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
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;
>> +}
>
>
11:38 a.m.
New subject: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
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;
> > > +}
> >
> >
4:30 a.m.
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Best Regards,
Yalan Zhang
IRC: yalzhang
Internal phone: 8389413
4:54 a.m.
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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
12:30 a.m.
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(a)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(a)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(a)redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
>
>
2:34 a.m.
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
2:41 a.m.
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(a)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
2:18 a.m.
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(a)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(a)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
>
2787
days inactive
2844
days old
23 comments
5 participants
participants (5)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
John Ferlan -
Laine Stump -
Yalan Zhang