[PATCH] qemu: Jump to cleanup label on umount failure

Jim Fehlig posted 1 patch 1 year, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20230206180803.4340-1-jfehlig@suse.com
src/qemu/qemu_namespace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] qemu: Jump to cleanup label on umount failure
Posted by Jim Fehlig 1 year, 1 month ago
Similar to other error paths in qemuDomainUnshareNamespace(), jump to
the cleanup label on umount error instead of directly returning -1.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---

I noticed this while looking at a bug report containing the error. ATM I'm not
sure why the umount failed, but have asked for more info in the bug

https://bugzilla.opensuse.org/show_bug.cgi?id=1207889

 src/qemu/qemu_namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 5769a4dfe0..833313d5a6 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -779,7 +779,7 @@ qemuDomainUnshareNamespace(virQEMUDriverConfig *cfg,
 #if defined(__linux__)
     if (umount("/dev") < 0) {
         virReportSystemError(errno, "%s", _("failed to umount devfs on /dev"));
-        return -1;
+        goto cleanup;
     }
 #endif /* !defined(__linux__) */
 
-- 
2.39.1
Re: [PATCH] qemu: Jump to cleanup label on umount failure
Posted by Michal Prívozník 1 year, 1 month ago
On 2/6/23 19:08, Jim Fehlig wrote:
> Similar to other error paths in qemuDomainUnshareNamespace(), jump to
> the cleanup label on umount error instead of directly returning -1.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> I noticed this while looking at a bug report containing the error. ATM I'm not
> sure why the umount failed, but have asked for more info in the bug
> 
> https://bugzilla.opensuse.org/show_bug.cgi?id=1207889
> 
>  src/qemu/qemu_namespace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index 5769a4dfe0..833313d5a6 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -779,7 +779,7 @@ qemuDomainUnshareNamespace(virQEMUDriverConfig *cfg,
>  #if defined(__linux__)
>      if (umount("/dev") < 0) {
>          virReportSystemError(errno, "%s", _("failed to umount devfs on /dev"));
> -        return -1;
> +        goto cleanup;
>      }
>  #endif /* !defined(__linux__) */
>  

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

And huge thanks to Martin Kletzander who actually came up with the root
cause for this problem. My code does not handle multiple mounts on the
same target. For instance:

# mount | grep /dev
udev on /dev type devtmpfs
(rw,nosuid,noexec,relatime,size=10240k,nr_inodes=4061238,mode=755)
devpts on /dev/pts type devpts (rw,relatime,gid=5,mode=620,ptmxmode=000)
tmpfs on /dev/shm type tmpfs (rw,nosuid,nodev)
mqueue on /dev/mqueue type mqueue (rw,nosuid,nodev,noexec,relatime)
tmpfs2 on /dev/shm type tmpfs (rw,relatime)

I've mounted /dev/shm for the second time as:

# mount -t tmpfs tmpfs2 /dev/shm/

The problem is that qemuDomainGetPreservedMounts() drops the one of the
/dev/shm entries, therefore, only one instance is moved to
/var/run/libvirt/... leaving the other behind. Hence, failing umount().

I'll try to come up with a fix, unless somebody else beats me to it.

Michal