10:59 a.m.
v2 of:
https://listman.redhat.com/archives/libvir-list/2022-December/236197.html
diff to v1:
- Check for existing device iff virNetDevGenerateName() returned early
(per Laine's suggestion).
Michal Prívozník (2):
virnetdev: Make virNetDevGenerateName() return 1 if no name was
generated
virnetdevtap.c: Disallow pre-existing TAP devices
src/qemu/qemu_interface.c | 2 ++
src/util/virnetdev.c | 6 ++++--
src/util/virnetdevtap.c | 23 +++++++++++++++++++++--
src/util/virnetdevtap.h | 2 ++
4 files changed, 29 insertions(+), 4 deletions(-)
--
2.37.4
10:59 a.m.
New subject: [PATCH v2 1/2] virnetdev: Make virNetDevGenerateName() return 1 if no name was generated
A caller might be interested in the case when @ifname was already
set and it wasn't a template. In such case the
virNetDevGenerateName() does not touch the @ifname at all and
returns 0 to indicate success. Make it return 1 to distinguish
this case from the other case, in which a new name was generated.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 66cfc5d781..82dbb486f2 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3595,7 +3595,9 @@ virNetDevReserveName(const char *name)
* Note: if string pointed by @ifname is NOT a template or NULL, leave
* it unchanged and return it directly.
*
- * Returns 0 on success, -1 on failure.
+ * Returns: 1 if @ifname already contains a valid name,
+ * 0 on success (@ifname was generated),
+ * -1 on failure.
*/
int
virNetDevGenerateName(char **ifname, virNetDevGenNameType type)
@@ -3609,7 +3611,7 @@ virNetDevGenerateName(char **ifname, virNetDevGenNameType type)
if (*ifname &&
(strchr(*ifname, '%') != strrchr(*ifname, '%') ||
strstr(*ifname, "%d") == NULL)) {
- return 0;
+ return 1;
}
if (maxIDd <= (double)INT_MAX)
--
2.37.4
10:59 a.m.
New subject: [PATCH v2 2/2] virnetdevtap.c: Disallow pre-existing TAP devices
When starting a guest with <interface/> which has the target
device name set (i.e. not generated by us), it may happen that
the TAP device already exists. This then may lead to all sorts of
problems. For instance: for <interface type='network'/> the TAP
device is plugged into the network's bridge, but since the TAP
device is persistent it remains plugged there even after the
guest is shut off. We don't have a code that unplugs TAP devices
from the bridge because TAP devices we create are transient, i.e.
are removed automatically when QEMU closes their FD.
The only exception is <interface type='ethernet'/> with <target
managed='no'/> where we specifically want to let users use
pre-created TAP device and basically not touch it at all.
There's another reason for denying to use a pre-created TAP
devices: if we ever have bug in TAP name generation, we may
re-use a TAP device from another domain.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2144738
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_interface.c | 2 ++
src/util/virnetdevtap.c | 23 +++++++++++++++++++++--
src/util/virnetdevtap.h | 2 ++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index 4cc76e07a5..264d5e060c 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -461,6 +461,8 @@ qemuInterfaceEthernetConnect(virDomainDef *def,
if (!net->ifname)
template_ifname = true;
+ tap_create_flags |= VIR_NETDEV_TAP_CREATE_ALLOW_EXISTING;
+
if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize,
tap_create_flags) < 0) {
goto cleanup;
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 112a1e8b99..a4ead0ae93 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -148,12 +148,15 @@ virNetDevTapGetRealDeviceName(char *ifname G_GNUC_UNUSED)
* @tunpath: path to the tun device (if NULL, /dev/net/tun is used)
* @tapfds: array of file descriptors return value for the new tap device
* @tapfdSize: number of file descriptors in @tapfd
- * @flags: OR of virNetDevTapCreateFlags. Only one flag is recognized:
+ * @flags: OR of virNetDevTapCreateFlags. Only the following flags are
+ * recognized:
*
* VIR_NETDEV_TAP_CREATE_VNET_HDR
* - Enable IFF_VNET_HDR on the tap device
* VIR_NETDEV_TAP_CREATE_PERSIST
* - The device will persist after the file descriptor is closed
+ * VIR_NETDEV_TAP_CREATE_ALLOW_EXISTING
+ * - The device creation fails if @ifname already exists
*
* Creates a tap interface. The caller must use virNetDevTapDelete to
* remove a persistent TAP device when it is no longer needed. In case
@@ -170,6 +173,7 @@ int virNetDevTapCreate(char **ifname,
{
size_t i = 0;
struct ifreq ifr;
+ int rc;
int ret = -1;
int fd = -1;
@@ -179,9 +183,24 @@ int virNetDevTapCreate(char **ifname,
* can lead to race conditions). if ifname is just a
* user-provided name, virNetDevGenerateName leaves it
* unchanged. */
- if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0)
+ rc = virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET);
+ if (rc < 0)
return -1;
+ if (rc > 0 &&
+ !(flags & VIR_NETDEV_TAP_CREATE_ALLOW_EXISTING)) {
+ rc = virNetDevExists(*ifname);
+
+ if (rc < 0) {
+ return -1;
+ } else if (rc > 0) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("The %s interface already exists"),
+ *ifname);
+ return -1;
+ }
+ }
+
if (!tunpath)
tunpath = "/dev/net/tun";
diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index 197ea10f94..c9d29c0384 100644
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -56,6 +56,8 @@ typedef enum {
VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE = 1 << 2,
/* The device will persist after the file descriptor is closed */
VIR_NETDEV_TAP_CREATE_PERSIST = 1 << 3,
+ /* The device is allowed to exist before creation */
+ VIR_NETDEV_TAP_CREATE_ALLOW_EXISTING = 1 << 4,
} virNetDevTapCreateFlags;
int
--
2.37.4
3:07 p.m.
On 12/8/22 11:59 AM, Michal Privoznik wrote:
v2 of:
https://listman.redhat.com/archives/libvir-list/2022-December/236197.html
diff to v1:
- Check for existing device iff virNetDevGenerateName() returned early
(per Laine's suggestion).
Michal Prívozník (2):
virnetdev: Make virNetDevGenerateName() return 1 if no name was
generated
virnetdevtap.c: Disallow pre-existing TAP devices
src/qemu/qemu_interface.c | 2 ++
src/util/virnetdev.c | 6 ++++--
src/util/virnetdevtap.c | 23 +++++++++++++++++++++--
src/util/virnetdevtap.h | 2 ++
4 files changed, 29 insertions(+), 4 deletions(-)
Reviewed-by: Laine Stump <laine(a)redhat.com>
(As I was reviewing, I realized I only got half of the story about the
FreeeBSD virNetDevTapCreate(), and there is actually a 2nd pre-existing
bug - since FreeBSD creates the tap device by creating a device and then
renaming it to the desired name, not only is it not necessary to
separately check for a pre-existing tap in order to avoid the
"unreported error" that your patch fixes for tap devices on Linux, but
*also* it means that on FreeBSD there is no way that we can use a
pre-existing tap device when we actually want to (i.e. when managed='no'
is set in <target>).
So instead of just telling you that the call to virNetDevExists() isn't
needed in order to prevent silently opening an existing device when we
don't want to allow it, I should have also said that there should be a
check for virNetDevExists(), but it should instead be used to alter the
logic in the case that your newly added ALLOW_EXISTING flag is set.
Since it's fixing a totally separate problem, it should be a separate
patch anyway though. I can send an RFC patch for that, but haven't had a
working FreeBSD system since 1998 or so (although I did spin up a VM
maybe 5 years ago) so I would have nothing to test it on other than
verifying it could compile with libvirt CI.)
749
days inactive
749
days old
3 comments
2 participants
participants (2)
-
Laine Stump -
Michal Privoznik