10:38 a.m.
On 12/8/22 10:17 AM, Michal Privoznik wrote:
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 | 31 ++++++++++++++++++++++++++++++-
src/util/virnetdevtap.h | 2 ++
3 files changed, 34 insertions(+), 1 deletion(-)
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..406339c583 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
@@ -182,6 +185,19 @@ int virNetDevTapCreate(char **ifname,
if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0)
return -1;
+ if (!(flags & VIR_NETDEV_TAP_CREATE_ALLOW_EXISTING)) {
+ int 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;
+ }
+ }
+
It's unfortunate we need the extra overhead of this separate operation
(virNetDevExists() - it creates a socket, calls ioctl(), and then closes
the socket, so 3 system calls) for every created tap device even though
it's only useful in the miniscule percentage of cases where the user
specifies the exact device name in the config (otherwise
virNetDevGenerateName() is already guaranteeing that the name it
generates is of a non-existing device).
Maybe virNetDevGenerateName() could set a bool if it actually did
generate a name, and we could go to the trouble of checking for the
device existing only if virNetDevGenerateName() returned that bool ==
false? (all the other places virNetDevGenerateName() is called would
just ignore this bool)?
if (!tunpath)
tunpath = "/dev/net/tun";
@@ -319,6 +335,19 @@ int virNetDevTapCreate(char **ifname,
if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0)
return -1;
+ if (!(flags & VIR_NETDEV_TAP_CREATE_ALLOW_EXISTING)) {
+ int 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;
+ }
+ }
+
I wonder if this chunk is needed at all - in FreeBSD the code creates a
new tap device with name provided by the kernel, and then renames that
device to the desired name. I would assume that if you attempted to
rename a device to the same name as an existing device, it would return
an error. I don't have a running FreeBSD system to try this out, but
would be very surprised if it behaved differently.
(I had first thought it might be a good idea to just put this chunk in
virNetDevGenerateName() as something common to all cases, but then
realized that in the case of macvtap/macvlan/veth interfaces we use the
netlink "NEWLINK" message, which is already returning an error if we try
to make a new device with the same name as an existing device (rather
than just returning a handle to the already-existing device as the API
for tap devices does.)
Reviewed-by: Laine Stump <laine(a)redhat.com>
(but maybe 1) remove the chunk from virNetDevTapCreate() for FreeBSD,
and possibly/probably 2) think about optimizing to only call
virNetDevExists() in the case when virNetDevGenerateName() is a NOP, as
I suggested up at the top)
/* As FreeBSD determines interface type by name,
* we have to create 'tap' interface first and
* then rename it to 'vnet'
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