[PATCH 0/2] qemu_namespace: Two simple improvements
These are inspired by Nikola's question here: https://listman.redhat.com/archives/libvir-list/2022-February/228812.html See my reply here: https://listman.redhat.com/archives/libvir-list/2022-March/229263.html Michal Prívozník (2): qemu_namespace: Don't unlink paths from cgroupDeviceACL qemu_namespace: Be less aggressive in removing /dev nodes from namespace src/qemu/qemu_namespace.c | 92 +++++++++++++++++++++++++++------------ 1 file changed, 63 insertions(+), 29 deletions(-) -- 2.34.1
When building namespace for a domain there are couple of devices that are created independent of domain config (see qemuDomainPopulateDevices()). The idea behind is that these devices are crucial for QEMU or one of its libraries, or user is passing through a device and wants us to create it in the namespace too. That's the reason that these devices are allowed in the devices CGroup controller as well. However, during unplug it may happen that a device is configured to use one of such devices and since we remove /dev nodes on hotplug we would remove such device too. For example, /dev/urandom belongs onto the list of implicit devices and users can hotplug and hotunplug an RNG device with /dev/urandom as backend. The fix is fortunately simple - just consult the list of implicit devices before removing the device from the namespace. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_namespace.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 3b41d72630..1132fd04e5 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -1364,6 +1364,8 @@ qemuNamespaceUnlinkPaths(virDomainObj *vm, if (STRPREFIX(path, QEMU_DEVPREFIX)) { GStrv mount; bool inSubmount = false; + const char *const *devices = (const char *const *)cfg->cgroupDeviceACL; + bool inDevices = false; for (mount = devMountsPath; *mount; mount++) { if (STREQ(*mount, "/dev")) @@ -1375,8 +1377,23 @@ qemuNamespaceUnlinkPaths(virDomainObj *vm, } } - if (!inSubmount) - unlinkPaths = g_slist_prepend(unlinkPaths, g_strdup(path)); + if (inSubmount) + continue; + + if (!devices) + devices = defaultDeviceACL; + + for (; devices; devices++) { + if (STREQ(path, *devices)) { + inDevices = true; + break; + } + } + + if (inDevices) + continue; + + unlinkPaths = g_slist_prepend(unlinkPaths, g_strdup(path)); } } -- 2.34.1
On a Monday in 2022, Michal Privoznik wrote:
When building namespace for a domain there are couple of devices that are created independent of domain config (see qemuDomainPopulateDevices()). The idea behind is that these devices are crucial for QEMU or one of its libraries, or user is passing through a device and wants us to create it in the namespace too. That's the reason that these devices are allowed in the devices CGroup controller as well.
However, during unplug it may happen that a device is configured to use one of such devices and since we remove /dev nodes on hotplug we would remove such device too. For example, /dev/urandom belongs onto the list of implicit devices and users can hotplug and hotunplug an RNG device with /dev/urandom as backend.
The fix is fortunately simple - just consult the list of implicit devices before removing the device from the namespace.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_namespace.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 3b41d72630..1132fd04e5 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -1364,6 +1364,8 @@ qemuNamespaceUnlinkPaths(virDomainObj *vm, if (STRPREFIX(path, QEMU_DEVPREFIX)) { GStrv mount; bool inSubmount = false; + const char *const *devices = (const char *const *)cfg->cgroupDeviceACL; + bool inDevices = false;
for (mount = devMountsPath; *mount; mount++) { if (STREQ(*mount, "/dev")) @@ -1375,8 +1377,23 @@ qemuNamespaceUnlinkPaths(virDomainObj *vm, } }
- if (!inSubmount) - unlinkPaths = g_slist_prepend(unlinkPaths, g_strdup(path)); + if (inSubmount) + continue; + + if (!devices) + devices = defaultDeviceACL; +
+ for (; devices; devices++) { + if (STREQ(path, *devices)) { + inDevices = true; + break; + } + } + + if (inDevices) + continue; +
something like: if (g_strv_contains(devices, path)) continue; should do the same without the need for the bool variable. (Not sure how to nicely eliminate the other one) Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
+ unlinkPaths = g_slist_prepend(unlinkPaths, g_strdup(path)); } }
-- 2.34.1
When creating /dev nodes in a QEMU domain's namespace the first thing we simply do is unlink() the path and create it again. This aims to solve the case when a file changed type/major/minor in the host and thus we need to reflect this in the guest's namespace. Fair enough, except we can be a bit more clever about it: firstly check whether the path doesn't already exist or isn't already of the correct type/major/minor and do the unlink+creation only if needed. Currently, this is implemented only for symlinks and block/character devices. For regular files/directories (which are less common) this might be implemented one day, but not today. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Use --ignore-space-change for the best experience. src/qemu/qemu_namespace.c | 71 ++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 1132fd04e5..0714a2d0de 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -948,38 +948,55 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data) } if (isLink) { - VIR_DEBUG("Creating symlink %s -> %s", data->file, data->target); + g_autoptr(GError) gerr = NULL; + g_autofree char *target = NULL; - /* First, unlink the symlink target. Symlinks change and - * therefore we have no guarantees that pre-existing - * symlink is still valid. */ - if (unlink(data->file) < 0 && - errno != ENOENT) { - virReportSystemError(errno, - _("Unable to remove symlink %s"), - data->file); - goto cleanup; - } - - if (symlink(data->target, data->file) < 0) { - virReportSystemError(errno, - _("Unable to create symlink %s (pointing to %s)"), - data->file, data->target); - goto cleanup; + if ((target = g_file_read_link(data->file, &gerr)) && + STREQ(target, data->target)) { + VIR_DEBUG("Skipping symlink %s -> %s which exists and points to correct target", + data->file, data->target); } else { - delDevice = true; + VIR_DEBUG("Creating symlink %s -> %s", data->file, data->target); + + /* First, unlink the symlink target. Symlinks change and + * therefore we have no guarantees that pre-existing + * symlink is still valid. */ + if (unlink(data->file) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove symlink %s"), + data->file); + goto cleanup; + } + + if (symlink(data->target, data->file) < 0) { + virReportSystemError(errno, + _("Unable to create symlink %s (pointing to %s)"), + data->file, data->target); + goto cleanup; + } else { + delDevice = true; + } } } else if (isDev) { - VIR_DEBUG("Creating dev %s (%d,%d)", - data->file, major(data->sb.st_rdev), minor(data->sb.st_rdev)); - unlink(data->file); - if (mknod(data->file, data->sb.st_mode, data->sb.st_rdev) < 0) { - virReportSystemError(errno, - _("Unable to create device %s"), - data->file); - goto cleanup; + GStatBuf sb; + + if (g_lstat(data->file, &sb) >= 0 && + sb.st_rdev == data->sb.st_rdev) { + VIR_DEBUG("Skipping dev %s (%d,%d) which exists and has correct MAJ:MIN", + data->file, major(data->sb.st_rdev), minor(data->sb.st_rdev)); } else { - delDevice = true; + VIR_DEBUG("Creating dev %s (%d,%d)", + data->file, major(data->sb.st_rdev), minor(data->sb.st_rdev)); + unlink(data->file); + if (mknod(data->file, data->sb.st_mode, data->sb.st_rdev) < 0) { + virReportSystemError(errno, + _("Unable to create device %s"), + data->file); + goto cleanup; + } else { + delDevice = true; + } } } else if (isReg || isDir) { /* We are not cleaning up disks on virDomainDetachDevice -- 2.34.1
participants (2)
-
Ján Tomko -
Michal Privoznik