[Qemu-devel] [PATCH 0/2] block: Do OFD lock check at runtime

[Qemu-devel] [PATCH 0/2] block: Do OFD lock check at runtime

[Fam Zheng]

This fixes the image opening failure reported by Andrew Baumann:

> I'm running a recent Linux build of qemu on Windows Subsystem for Linux (WSL)
> which doesn't appear to implement file locking:
>
> $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -device virtio-blk-pci,drive=hd0
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock byte 100

It appears to be that the binary is built for Linux targets, but the WSL
runtime doesn't recognize the ops (-EINVAL).

Convert to runtime check to cope with that.

Fam Zheng (2):
  osdep: Add runtime OFD lock detection
  file-posix: Do runtime check for ofd lock API

 block/file-posix.c   | 19 ++++++--------
 include/qemu/osdep.h |  1 +
 util/osdep.c         | 72 +++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 72 insertions(+), 20 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH 1/2] osdep: Add runtime OFD lock detection

[Fam Zheng]

Build time check of OFD lock is not sufficient and can cause image open
errors when the runtime environment doesn't support it.

Add a helper function to probe it at runtime, additionally. Also provide
a qemu_has_ofd_lock() for callers to check the status.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/qemu/osdep.h |  1 +
 util/osdep.c         | 72 +++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 3b74f6fcb2..6855b94bbf 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -357,6 +357,7 @@ int qemu_dup(int fd);
 int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
 int qemu_unlock_fd(int fd, int64_t start, int64_t len);
 int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
+bool qemu_has_ofd_lock(void);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index a2863c8e53..b275c6579a 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -38,13 +38,8 @@ extern int madvise(caddr_t, size_t, int);
 #include "qemu/error-report.h"
 #include "monitor/monitor.h"
 
-#ifdef F_OFD_SETLK
-#define QEMU_SETLK F_OFD_SETLK
-#define QEMU_GETLK F_OFD_GETLK
-#else
-#define QEMU_SETLK F_SETLK
-#define QEMU_GETLK F_GETLK
-#endif
+static int fcntl_op_setlk = -1;
+static int fcntl_op_getlk = -1;
 
 static bool fips_enabled = false;
 
@@ -149,6 +144,63 @@ static int qemu_parse_fdset(const char *param)
     return qemu_parse_fd(param);
 }
 
+static void qemu_probe_lock_ops(void)
+{
+    if (fcntl_op_setlk == -1) {
+#ifdef F_OFD_SETLK
+        int fd;
+        int ret;
+        const char *tmpdir;
+        char *filename;
+        struct flock fl = {
+            .l_whence = SEEK_SET,
+            .l_start  = 0,
+            .l_len    = 0,
+            .l_type   = F_WRLCK,
+        };
+
+        tmpdir = getenv("TMPDIR");
+        if (!tmpdir) {
+            tmpdir = "/var/tmp";
+        }
+        filename = g_strdup_printf("%s/qemu_lock_probe.XXXXXX", tmpdir);
+        fd = mkstemp(filename);
+        if (fd < 0) {
+            fprintf(stderr, "Failed to create temporary file '%s': %s\n",
+                    filename, strerror(errno));
+            fcntl_op_setlk = F_SETLK;
+            fcntl_op_getlk = F_GETLK;
+            goto out;
+        }
+        ret = fcntl(fd, F_OFD_SETLK, &fl);
+        close(fd);
+        unlink(filename);
+        if (!ret) {
+            fcntl_op_setlk = F_OFD_SETLK;
+            fcntl_op_getlk = F_OFD_GETLK;
+        } else {
+            fcntl_op_setlk = F_SETLK;
+            fcntl_op_getlk = F_GETLK;
+        }
+out:
+        g_free(filename);
+#else
+        fcntl_op_setlk = F_SETLK;
+        fcntl_op_getlk = F_GETLK;
+#endif
+    }
+}
+
+bool qemu_has_ofd_lock(void)
+{
+    qemu_probe_lock_ops();
+#ifdef F_OFD_SETLK
+    return fcntl_op_setlk == F_OFD_SETLK;
+#else
+    return false;
+#endif
+}
+
 static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
 {
     int ret;
@@ -158,7 +210,8 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
         .l_len    = len,
         .l_type   = fl_type,
     };
-    ret = fcntl(fd, QEMU_SETLK, &fl);
+    qemu_probe_lock_ops();
+    ret = fcntl(fd, fcntl_op_setlk, &fl);
     return ret == -1 ? -errno : 0;
 }
 
@@ -181,7 +234,8 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
         .l_len    = len,
         .l_type   = exclusive ? F_WRLCK : F_RDLCK,
     };
-    ret = fcntl(fd, QEMU_GETLK, &fl);
+    qemu_probe_lock_ops();
+    ret = fcntl(fd, fcntl_op_getlk, &fl);
     if (ret == -1) {
         return -errno;
     } else {
-- 
2.13.3

Re: [Qemu-devel] [PATCH 1/2] osdep: Add runtime OFD lock detection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

On 07/21/2017 05:20 AM, Fam Zheng wrote:
> Build time check of OFD lock is not sufficient and can cause image open
> errors when the runtime environment doesn't support it.
> 
> Add a helper function to probe it at runtime, additionally. Also provide
> a qemu_has_ofd_lock() for callers to check the status.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 72 +++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 64 insertions(+), 9 deletions(-)

>  
> +static void qemu_probe_lock_ops(void)
> +{
> +    if (fcntl_op_setlk == -1) {
> +#ifdef F_OFD_SETLK
> +        int fd;
> +        int ret;
> +        const char *tmpdir;
> +        char *filename;
> +        struct flock fl = {
> +            .l_whence = SEEK_SET,
> +            .l_start  = 0,
> +            .l_len    = 0,
> +            .l_type   = F_WRLCK,

Do we actually need to grab a lock over the entire file,...

> +        };
> +
> +        tmpdir = getenv("TMPDIR");
> +        if (!tmpdir) {
> +            tmpdir = "/var/tmp";
> +        }
> +        filename = g_strdup_printf("%s/qemu_lock_probe.XXXXXX", tmpdir);
> +        fd = mkstemp(filename);

Can we skip the temporary file, and just open("/dev/null", O_RDWR), for
slightly less work?

> +        if (fd < 0) {
> +            fprintf(stderr, "Failed to create temporary file '%s': %s\n",
> +                    filename, strerror(errno));
> +            fcntl_op_setlk = F_SETLK;
> +            fcntl_op_getlk = F_GETLK;
> +            goto out;
> +        }
> +        ret = fcntl(fd, F_OFD_SETLK, &fl);

...or should we change this to the much-weaker F_OFD_GETLK?  I guess it
doesn't matter if we use a temporary file, but does if we are trying to
simplify by using /dev/null.

At any rate, the rest of the patch looks sane.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH 2/2] file-posix: Do runtime check for ofd lock API

[Fam Zheng]

It is reported that on Windows Subsystem for Linux, ofd operations fail
with -EINVAL. In other words, QEMU binary built with system headers that
exports F_OFD_SETLK doesn't necessarily run in an environment that
actually supports it:

$ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 \
    -device virtio-blk-pci,drive=hd0
qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock byte 100

Let's do a runtime check to cope with that.

Reported-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index cfbb236f6f..5ddf2729eb 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -457,22 +457,19 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     switch (locking) {
     case ON_OFF_AUTO_ON:
         s->use_lock = true;
-#ifndef F_OFD_SETLK
-        fprintf(stderr,
-                "File lock requested but OFD locking syscall is unavailable, "
-                "falling back to POSIX file locks.\n"
-                "Due to the implementation, locks can be lost unexpectedly.\n");
-#endif
+        if (!qemu_has_ofd_lock()) {
+            fprintf(stderr,
+                    "File lock requested but OFD locking syscall is "
+                    "unavailable, falling back to POSIX file locks.\n"
+                    "Due to the implementation, locks can be lost "
+                    "unexpectedly.\n");
+        }
         break;
     case ON_OFF_AUTO_OFF:
         s->use_lock = false;
         break;
     case ON_OFF_AUTO_AUTO:
-#ifdef F_OFD_SETLK
-        s->use_lock = true;
-#else
-        s->use_lock = false;
-#endif
+        s->use_lock = qemu_has_ofd_lock();
         break;
     default:
         abort();
-- 
2.13.3

Re: [Qemu-devel] [PATCH 2/2] file-posix: Do runtime check for ofd lock API

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

On 07/21/2017 05:20 AM, Fam Zheng wrote:
> It is reported that on Windows Subsystem for Linux, ofd operations fail
> with -EINVAL. In other words, QEMU binary built with system headers that
> exports F_OFD_SETLK doesn't necessarily run in an environment that
> actually supports it:
> 
> $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 \
>     -device virtio-blk-pci,drive=hd0
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock byte 100
> 
> Let's do a runtime check to cope with that.

You may want to mention that the same is possible on a system with old
kernel but new glibc (ie. this issue is not necessarily specific to WSL).

> 
> Reported-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/2] file-posix: Do runtime check for ofd lock API

[Christian Ehrhardt]

On Fri, Jul 21, 2017 at 2:36 PM, Eric Blake <eblake@redhat.com> wrote:

> On 07/21/2017 05:20 AM, Fam Zheng wrote:
> > It is reported that on Windows Subsystem for Linux, ofd operations fail
> > with -EINVAL. In other words, QEMU binary built with system headers that
> > exports F_OFD_SETLK doesn't necessarily run in an environment that
> > actually supports it:
> >
> > $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 \
> >     -device virtio-blk-pci,drive=hd0
> > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to
> unlock byte 100
> > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to
> unlock byte 100
> > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to
> lock byte 100
> >
> > Let's do a runtime check to cope with that.
>
> You may want to mention that the same is possible on a system with old
> kernel but new glibc (ie. this issue is not necessarily specific to WSL).
>

I first thought that this combination hitting me as I run KVM in containers
which can diverge glibc (in container) a lot from kernel (in host).

My issue turned out to be an apparmor block instead.
But since I clearly see how my case could lead to the mentioned
old kernel but new glibc I wanted to ping here to refresh/reconsider
this change as well.

Also the reply might be worth as documentation if people search for the
error
message and get here that the following apparmor block leads to the same.

apparmor="DENIED" operation="file_lock"
namespace="root//lxd-testkvm-artful-from_<var-lib-lxd>"
profile="libvirt-f687a9b3-5bca-41bc-b206-6e616720cc5e"
name="/var/lib/uvtool/libvirt/images/kvmguest-artful-normal.qcow" pid=7001
comm="qemu-system-x86" requested_mask="k" denied_mask="k" fsuid=0 ouid=0

I'll work on libvirt's virt-aa-helper to generate a rule appropriate for
the new behavior.

Re: [Qemu-devel] [PATCH 2/2] file-posix: Do runtime check for ofd lock API

[Andrew Baumann]

> From: Fam Zheng [mailto:famz@redhat.com]
> Sent: Friday, 21 July 2017 3:21
> 
> It is reported that on Windows Subsystem for Linux, ofd operations fail
> with -EINVAL. In other words, QEMU binary built with system headers that
> exports F_OFD_SETLK doesn't necessarily run in an environment that
> actually supports it:
> 
> $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 \
>     -device virtio-blk-pci,drive=hd0
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock
> byte 100
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock
> byte 100
> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock
> byte 100
> 
> Let's do a runtime check to cope with that.
> 
> Reported-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)

If it helps:
Tested-By: Andrew Baumann <andrew.baumann@microsoft.com>

Thanks!
Andrew

Re: [Qemu-devel] [PATCH 0/2] block: Do OFD lock check at runtime

[Kevin Wolf]

Am 21.07.2017 um 12:20 hat Fam Zheng geschrieben:
> This fixes the image opening failure reported by Andrew Baumann:
> 
> > I'm running a recent Linux build of qemu on Windows Subsystem for Linux (WSL)
> > which doesn't appear to implement file locking:
> >
> > $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -device virtio-blk-pci,drive=hd0
> > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
> > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100
> > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock byte 100
> 
> It appears to be that the binary is built for Linux targets, but the WSL
> runtime doesn't recognize the ops (-EINVAL).
> 
> Convert to runtime check to cope with that.

Fair enough in this specific case because we still support older Linux
kernels and we want to fail gracefully if the binary was built against
a newer kernel.

However, I think the real problem here is with the WSL ecosystem if qemu
is routinely built against a real Linux while WSL doesn't provide the
same functionality. WSL should provide kernel headers that match what
it can provide (i.e. either remove the unimplemnted constants or
implement them).

So for the future, I'm not sure that we should add workarounds for WSL
shortcomings when no real Linux is affected.

This is even more true when the reporter is a Microsoft employee.

Kevin

Re: [Qemu-devel] [PATCH 0/2] block: Do OFD lock check at runtime

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On 07/21/2017 06:56 AM, Kevin Wolf wrote:
>>
>> Convert to runtime check to cope with that.
> 
> Fair enough in this specific case because we still support older Linux
> kernels and we want to fail gracefully if the binary was built against
> a newer kernel.

Or, more likely: if we are built against a newer glibc (that has the
constants) but an older kernel (that lacks support for the constants).

> 
> However, I think the real problem here is with the WSL ecosystem if qemu
> is routinely built against a real Linux while WSL doesn't provide the
> same functionality. WSL should provide kernel headers that match what
> it can provide (i.e. either remove the unimplemnted constants or
> implement them).

In other words, you're arguing that binaries built for WSL should be
cross-compiled rather than native compiled (similar to how mingw
binaries are built in a Cygwin environment), such that the
cross-compiler picks up the correct altered headers for WSL limitations.
 I agree - but I have no influence on how likely that is to come about.

> 
> So for the future, I'm not sure that we should add workarounds for WSL
> shortcomings when no real Linux is affected.
> 
> This is even more true when the reporter is a Microsoft employee.
> 
> Kevin
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 0/2] block: Do OFD lock check at runtime

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 07:34:11AM -0500, Eric Blake wrote:
> On 07/21/2017 06:56 AM, Kevin Wolf wrote:
> >>
> >> Convert to runtime check to cope with that.
> > 
> > Fair enough in this specific case because we still support older Linux
> > kernels and we want to fail gracefully if the binary was built against
> > a newer kernel.
> 
> Or, more likely: if we are built against a newer glibc (that has the
> constants) but an older kernel (that lacks support for the constants).
> 
> > 
> > However, I think the real problem here is with the WSL ecosystem if qemu
> > is routinely built against a real Linux while WSL doesn't provide the
> > same functionality. WSL should provide kernel headers that match what
> > it can provide (i.e. either remove the unimplemnted constants or
> > implement them).
> 
> In other words, you're arguing that binaries built for WSL should be
> cross-compiled rather than native compiled (similar to how mingw
> binaries are built in a Cygwin environment), such that the
> cross-compiler picks up the correct altered headers for WSL limitations.
>  I agree - but I have no influence on how likely that is to come about.

IIUC, the core selling point of WSL was that you can take an existing
Ubuntu (or now various other Linux) distro  and just run it "as is",
without recompiling its specially.  So this really is best thought
of as the build with new glibc+kernel, but run on old kernel scenario.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|