[libvirt] [PATCH v2 0/2] BSD implementation of virNetDevTapCreate() and virNetDevTapDelete()
Changes from the previous version: * virNetDevSetupControl patch: remove pointless <config.h> include from virnetdev.h * virnetdevtap.c patch: rebase to include new tapfdSize param for virNetDevTapCreate() Roman Bogorodskiy (2): Make virNetDevSetupControl() public. BSD: implement virNetDevTapCreate() and virNetDevTapDelete() src/libvirt_private.syms | 1 + src/util/virnetdev.c | 15 ++++- src/util/virnetdev.h | 11 ++++ src/util/virnetdevmacvlan.c | 2 +- src/util/virnetdevtap.c | 116 ++++++++++++++++++++++++++++++++++++++- src/util/virnetdevvportprofile.c | 2 +- 6 files changed, 141 insertions(+), 6 deletions(-) -- 1.8.0
This method is useful not only in virnetdev.c. --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 15 +++++++++++++-- src/util/virnetdev.h | 11 +++++++++++ src/util/virnetdevmacvlan.c | 2 +- src/util/virnetdevvportprofile.c | 2 +- 5 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9d5f74b..da3dc63 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1529,6 +1529,7 @@ virNetDevSetMTUFromDevice; virNetDevSetName; virNetDevSetNamespace; virNetDevSetOnline; +virNetDevSetupControl; virNetDevValidateConfig; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index cee4001..f658c6d 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -88,11 +88,22 @@ static int virNetDevSetupControlFull(const char *ifname, } -static int virNetDevSetupControl(const char *ifname, - struct ifreq *ifr) +int +virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) { return virNetDevSetupControlFull(ifname, ifr, VIR_NETDEV_FAMILY, SOCK_DGRAM); } +#else +int +virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED, + void *ifr ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, + _("%s is not supported on this platform"), + __func__); + return -1; +} #endif diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index bc0777c..97369c1 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -23,11 +23,22 @@ #ifndef __VIR_NETDEV_H__ # define __VIR_NETDEV_H__ +# include <net/if.h> + # include "virsocketaddr.h" # include "virnetlink.h" # include "virmacaddr.h" # include "virpci.h" +#ifdef HAVE_STRUCT_IFREQ +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +#else +int virNetDevSetupControl(const char *ifname, + void *ifr); +#endif + int virNetDevExists(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 6c338fa..a61c45f 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -49,7 +49,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # include <sys/socket.h> # include <sys/ioctl.h> -# include <linux/if.h> +# include <net/if.h> # include <linux/if_tun.h> /* Older kernels lacked this enum value. */ diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index c337fb3..0f1b6a1 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -49,7 +49,7 @@ VIR_ENUM_IMPL(virNetDevVPortProfileOp, VIR_NETDEV_VPORT_PROFILE_OP_LAST, # include <sys/socket.h> # include <sys/ioctl.h> -# include <linux/if.h> +# include <net/if.h> # include <linux/if_tun.h> # include "virnetlink.h" -- 1.8.0
On 05/25/2013 09:24 AM, Roman Bogorodskiy wrote:
This method is useful not only in virnetdev.c. --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 15 +++++++++++++-- src/util/virnetdev.h | 11 +++++++++++ src/util/virnetdevmacvlan.c | 2 +- src/util/virnetdevvportprofile.c | 2 +- 5 files changed, 27 insertions(+), 4 deletions(-)
You don't have 'cppi' installed, or 'make syntax-check' would have pointed out these nits: preprocessor_indentation cppi: src/util/virnetdev.h: line 33: not properly indented cppi: src/util/virnetdev.h: line 37: not properly indented cppi: src/util/virnetdev.h: line 40: not properly indented maint.mk: incorrect preprocessor indentation
+#else +int +virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED, + void *ifr ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, + _("%s is not supported on this platform"), + __func__);
I liked Dan's suggestion for how to word this:
How about
"Network device configuration is not supported on this platform"
+#ifdef HAVE_STRUCT_IFREQ +int virNetDevSetupControl(const char *ifname, + struct ifreq *ifr) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +#else +int virNetDevSetupControl(const char *ifname, + void *ifr); +#endif
I'm not sure I'm a fan of changing the compile-time attributes based on whether the struct exists; I'm proposing an alternate solution below.
+++ b/src/util/virnetdevmacvlan.c @@ -49,7 +49,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST, # include <sys/socket.h> # include <sys/ioctl.h>
-# include <linux/if.h> +# include <net/if.h> # include <linux/if_tun.h>
Independently useful. I might split your patch into two, and apply the <net/if.h> usage right away while awaiting feedback on my proposed tweaks. diff --git i/src/util/virnetdev.c w/src/util/virnetdev.c index f658c6d..1316bc4 100644 --- i/src/util/virnetdev.c +++ w/src/util/virnetdev.c @@ -99,9 +99,9 @@ int virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED, void *ifr ATTRIBUTE_UNUSED) { - virReportSystemError(ENOSYS, - _("%s is not supported on this platform"), - __func__); + virReportSystemError(ENOSYS, "%s", + _("Network device configuration is not supported " + "on this platform")); return -1; } #endif diff --git i/src/util/virnetdev.h w/src/util/virnetdev.h index 97369c1..3faff48 100644 --- i/src/util/virnetdev.h +++ w/src/util/virnetdev.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -30,14 +30,15 @@ # include "virmacaddr.h" # include "virpci.h" -#ifdef HAVE_STRUCT_IFREQ +# ifdef HAVE_STRUCT_IFREQ +typedef struct ifreq virIfreq; +# else +struct virIfreq { }; +# endif + int virNetDevSetupControl(const char *ifname, - struct ifreq *ifr) + virIfreq *ifr) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -#else -int virNetDevSetupControl(const char *ifname, - void *ifr); -#endif int virNetDevExists(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Implementation uses SIOCIFCREATE2 and SIOCIFDESTROY ioctls. --- src/util/virnetdevtap.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 2 deletions(-) diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index bb4b3ba..a12f816 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -275,7 +275,119 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } -#else /* ! TUNSETIFF */ +#elif defined(SIOCIFCREATE2) && defined(SIOCIFDESTROY) +int virNetDevTapCreate(char **ifname, + int *tapfd, + int tapfdSize, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int s; + struct ifreq ifr; + int ret = -1; + char *newifname = NULL; + + if (tapfdSize > 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiqueue devices are not supported on this system")); + goto cleanup; + } + + /* As FreeBSD determines interface type by name, + * we have to create 'tap' interface first and + * then rename it to 'vnet' + */ + if ((s = virNetDevSetupControl("tap", &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create tap device")); + goto cleanup; + } + + /* In case we were given exact interface name (e.g. 'vnetN'), + * we just rename to it. If we have format string like + * 'vnet%d', we need to find the first available name that + * matches this pattern + */ + if (strstr(*ifname, "%d") != NULL) { + int i; + for (i = 0; i <= IF_MAXUNIT; i++) { + char *newname; + if (virAsprintf(&newname, *ifname, i) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virNetDevExists(newname) == 0) { + newifname = newname; + break; + } + } + if (newifname) { + VIR_FREE(*ifname); + *ifname = newifname; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to generate new name for interface %s"), + ifr.ifr_name); + goto cleanup; + } + } + + if (tapfd) { + char *dev_path = NULL; + if (virAsprintf(&dev_path, "/dev/%s", ifr.ifr_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((*tapfd = open(dev_path, O_RDWR)) < 0) { + virReportSystemError(errno, + _("Unable to open %s"), + dev_path); + VIR_FREE(dev_path); + goto cleanup; + } + + VIR_FREE(dev_path); + } + + if (virNetDevSetName(ifr.ifr_name, *ifname) == -1) { + goto cleanup; + } + + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + + return ret; +} + +int virNetDevTapDelete(const char *ifname) +{ + int s; + struct ifreq ifr; + int ret = -1; + + if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) + return -1; + + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to remove tap device %s"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(s); + return ret; +} + +#else int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, int tapfdSize ATTRIBUTE_UNUSED, @@ -291,7 +403,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) _("Unable to delete TAP devices on this platform")); return -1; } -#endif /* ! TUNSETIFF */ +#endif /** -- 1.8.0
On 05/25/2013 09:24 AM, Roman Bogorodskiy wrote:
Implementation uses SIOCIFCREATE2 and SIOCIFDESTROY ioctls. --- src/util/virnetdevtap.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 2 deletions(-)
+ /* In case we were given exact interface name (e.g. 'vnetN'), + * we just rename to it. If we have format string like + * 'vnet%d', we need to find the first available name that + * matches this pattern + */ + if (strstr(*ifname, "%d") != NULL) { + int i; + for (i = 0; i <= IF_MAXUNIT; i++) { + char *newname; + if (virAsprintf(&newname, *ifname, i) < 0) {
Are we POSITIVE that this string is sanitized to have exactly one %d, or is there a risk that *ifname is user-supplied and can be used to exploit us?
+ virReportOOMError(); + goto cleanup; + } + + if (virNetDevExists(newname) == 0) { + newifname = newname; + break; + } + }
Memory leak if you go through the loop more than once. You must free newname on each failed iteration. The rest of the code compiles fine on my FreeBSD VM, although I'm not sure how to test it. It looks like a fixed version would make sense for 1.0.7 (although you may need someone else to step in and commit if I'm still on my vacation at the time we hit code freeze). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/25/2013 09:24 AM, Roman Bogorodskiy wrote:
Implementation uses SIOCIFCREATE2 and SIOCIFDESTROY ioctls. --- src/util/virnetdevtap.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 2 deletions(-)
+ /* In case we were given exact interface name (e.g. 'vnetN'), + * we just rename to it. If we have format string like + * 'vnet%d', we need to find the first available name that + * matches this pattern + */ + if (strstr(*ifname, "%d") != NULL) { + int i; + for (i = 0; i <= IF_MAXUNIT; i++) { + char *newname; + if (virAsprintf(&newname, *ifname, i) < 0) {
Are we POSITIVE that this string is sanitized to have exactly one %d, or is there a risk that *ifname is user-supplied and can be used to exploit us?
ifname could be supplied by user, but if I understood code correctly, it's checked that: * user supplied ifname doesn't start with auto-generated prefix ('vnet') * user supplied ifname doesn't have '%' in it otherwise we fallback to the auto-generated name (prefix + "%d"). Relevant code pieces: qemu/qemu_command.c:339 uml/uml_conf.c:114
+ virReportOOMError(); + goto cleanup; + } + + if (virNetDevExists(newname) == 0) { + newifname = newname; + break; + } + }
Memory leak if you go through the loop more than once. You must free newname on each failed iteration.
Will fix.
The rest of the code compiles fine on my FreeBSD VM, although I'm not sure how to test it. It looks like a fixed version would make sense for 1.0.7 (although you may need someone else to step in and commit if I'm still on my vacation at the time we hit code freeze).
Well, I test it with domain definition like that: https://dpaste.de/5jK67/ Networking obviously is not functional, but it's enough to test device creation. Roman Bogorodskiy
participants (2)
-
Eric Blake -
Roman Bogorodskiy