yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-file-posix-Skip-effectiveless-OFD-lock-operations.patch

b38b0f
From b9aaae95df14d93aab128376c40943259a453730 Mon Sep 17 00:00:00 2001
b38b0f
From: Max Reitz <mreitz@redhat.com>
b38b0f
Date: Wed, 3 Apr 2019 17:13:10 +0100
b38b0f
Subject: [PATCH 03/11] file-posix: Skip effectiveless OFD lock operations
b38b0f
b38b0f
RH-Author: Max Reitz <mreitz@redhat.com>
b38b0f
Message-id: <20190403171315.20841-4-mreitz@redhat.com>
b38b0f
Patchwork-id: 85401
b38b0f
O-Subject: [RHEL-8.1 qemu-kvm PATCH 3/8] file-posix: Skip effectiveless OFD lock operations
b38b0f
Bugzilla: 1694148
b38b0f
RH-Acked-by: John Snow <jsnow@redhat.com>
b38b0f
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
b38b0f
RH-Acked-by: Stefano Garzarella <sgarzare@redhat.com>
b38b0f
b38b0f
From: Fam Zheng <famz@redhat.com>
b38b0f
b38b0f
If we know we've already locked the bytes, don't do it again; similarly
b38b0f
don't unlock a byte if we haven't locked it. This doesn't change the
b38b0f
behavior, but fixes a corner case explained below.
b38b0f
b38b0f
Libvirt had an error handling bug that an image can get its (ownership,
b38b0f
file mode, SELinux) permissions changed (RHBZ 1584982) by mistake behind
b38b0f
QEMU. Specifically, an image in use by Libvirt VM has:
b38b0f
b38b0f
    $ ls -lhZ b.img
b38b0f
    -rw-r--r--. qemu qemu system_u:object_r:svirt_image_t:s0:c600,c690 b.img
b38b0f
b38b0f
Trying to attach it a second time won't work because of image locking.
b38b0f
And after the error, it becomes:
b38b0f
b38b0f
    $ ls -lhZ b.img
b38b0f
    -rw-r--r--. root root system_u:object_r:virt_image_t:s0 b.img
b38b0f
b38b0f
Then, we won't be able to do OFD lock operations with the existing fd.
b38b0f
In other words, the code such as in blk_detach_dev:
b38b0f
b38b0f
    blk_set_perm(blk, 0, BLK_PERM_ALL, &error_abort);
b38b0f
b38b0f
can abort() QEMU, out of environmental changes.
b38b0f
b38b0f
This patch is an easy fix to this and the change is regardlessly
b38b0f
reasonable, so do it.
b38b0f
b38b0f
Signed-off-by: Fam Zheng <famz@redhat.com>
b38b0f
Reviewed-by: Max Reitz <mreitz@redhat.com>
b38b0f
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
b38b0f
(cherry picked from commit 2996ffad3acabe890fbb4f84a069cdc325a68108)
b38b0f
Signed-off-by: Max Reitz <mreitz@redhat.com>
b38b0f
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
b38b0f
---
b38b0f
 block/file-posix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++----------
b38b0f
 1 file changed, 44 insertions(+), 10 deletions(-)
b38b0f
b38b0f
diff --git a/block/file-posix.c b/block/file-posix.c
b38b0f
index c2403ba..2a05193 100644
b38b0f
--- a/block/file-posix.c
b38b0f
+++ b/block/file-posix.c
b38b0f
@@ -152,6 +152,11 @@ typedef struct BDRVRawState {
b38b0f
     uint64_t perm;
b38b0f
     uint64_t shared_perm;
b38b0f
 
b38b0f
+    /* The perms bits whose corresponding bytes are already locked in
b38b0f
+     * s->lock_fd. */
b38b0f
+    uint64_t locked_perm;
b38b0f
+    uint64_t locked_shared_perm;
b38b0f
+
b38b0f
 #ifdef CONFIG_XFS
b38b0f
     bool is_xfs:1;
b38b0f
 #endif
b38b0f
@@ -677,43 +682,72 @@ typedef enum {
b38b0f
  * file; if @unlock == true, also unlock the unneeded bytes.
b38b0f
  * @shared_perm_lock_bits is the mask of all permissions that are NOT shared.
b38b0f
  */
b38b0f
-static int raw_apply_lock_bytes(int fd,
b38b0f
+static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
b38b0f
                                 uint64_t perm_lock_bits,
b38b0f
                                 uint64_t shared_perm_lock_bits,
b38b0f
                                 bool unlock, Error **errp)
b38b0f
 {
b38b0f
     int ret;
b38b0f
     int i;
b38b0f
+    uint64_t locked_perm, locked_shared_perm;
b38b0f
+
b38b0f
+    if (s) {
b38b0f
+        locked_perm = s->locked_perm;
b38b0f
+        locked_shared_perm = s->locked_shared_perm;
b38b0f
+    } else {
b38b0f
+        /*
b38b0f
+         * We don't have the previous bits, just lock/unlock for each of the
b38b0f
+         * requested bits.
b38b0f
+         */
b38b0f
+        if (unlock) {
b38b0f
+            locked_perm = BLK_PERM_ALL;
b38b0f
+            locked_shared_perm = BLK_PERM_ALL;
b38b0f
+        } else {
b38b0f
+            locked_perm = 0;
b38b0f
+            locked_shared_perm = 0;
b38b0f
+        }
b38b0f
+    }
b38b0f
 
b38b0f
     PERM_FOREACH(i) {
b38b0f
         int off = RAW_LOCK_PERM_BASE + i;
b38b0f
-        if (perm_lock_bits & (1ULL << i)) {
b38b0f
+        uint64_t bit = (1ULL << i);
b38b0f
+        if ((perm_lock_bits & bit) && !(locked_perm & bit)) {
b38b0f
             ret = qemu_lock_fd(fd, off, 1, false);
b38b0f
             if (ret) {
b38b0f
                 error_setg(errp, "Failed to lock byte %d", off);
b38b0f
                 return ret;
b38b0f
+            } else if (s) {
b38b0f
+                s->locked_perm |= bit;
b38b0f
             }
b38b0f
-        } else if (unlock) {
b38b0f
+        } else if (unlock && (locked_perm & bit) && !(perm_lock_bits & bit)) {
b38b0f
             ret = qemu_unlock_fd(fd, off, 1);
b38b0f
             if (ret) {
b38b0f
                 error_setg(errp, "Failed to unlock byte %d", off);
b38b0f
                 return ret;
b38b0f
+            } else if (s) {
b38b0f
+                s->locked_perm &= ~bit;
b38b0f
             }
b38b0f
         }
b38b0f
     }
b38b0f
     PERM_FOREACH(i) {
b38b0f
         int off = RAW_LOCK_SHARED_BASE + i;
b38b0f
-        if (shared_perm_lock_bits & (1ULL << i)) {
b38b0f
+        uint64_t bit = (1ULL << i);
b38b0f
+        if ((shared_perm_lock_bits & bit) && !(locked_shared_perm & bit)) {
b38b0f
             ret = qemu_lock_fd(fd, off, 1, false);
b38b0f
             if (ret) {
b38b0f
                 error_setg(errp, "Failed to lock byte %d", off);
b38b0f
                 return ret;
b38b0f
+            } else if (s) {
b38b0f
+                s->locked_shared_perm |= bit;
b38b0f
             }
b38b0f
-        } else if (unlock) {
b38b0f
+        } else if (unlock && (locked_shared_perm & bit) &&
b38b0f
+                   !(shared_perm_lock_bits & bit)) {
b38b0f
             ret = qemu_unlock_fd(fd, off, 1);
b38b0f
             if (ret) {
b38b0f
                 error_setg(errp, "Failed to unlock byte %d", off);
b38b0f
                 return ret;
b38b0f
+            } else if (s) {
b38b0f
+                s->locked_shared_perm &= ~bit;
b38b0f
             }
b38b0f
         }
b38b0f
     }
b38b0f
@@ -781,7 +815,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
b38b0f
 
b38b0f
     switch (op) {
b38b0f
     case RAW_PL_PREPARE:
b38b0f
-        ret = raw_apply_lock_bytes(s->lock_fd, s->perm | new_perm,
b38b0f
+        ret = raw_apply_lock_bytes(s, s->lock_fd, s->perm | new_perm,
b38b0f
                                    ~s->shared_perm | ~new_shared,
b38b0f
                                    false, errp);
b38b0f
         if (!ret) {
b38b0f
@@ -796,7 +830,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
b38b0f
         op = RAW_PL_ABORT;
b38b0f
         /* fall through to unlock bytes. */
b38b0f
     case RAW_PL_ABORT:
b38b0f
-        raw_apply_lock_bytes(s->lock_fd, s->perm, ~s->shared_perm,
b38b0f
+        raw_apply_lock_bytes(s, s->lock_fd, s->perm, ~s->shared_perm,
b38b0f
                              true, &local_err);
b38b0f
         if (local_err) {
b38b0f
             /* Theoretically the above call only unlocks bytes and it cannot
b38b0f
@@ -806,7 +840,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
b38b0f
         }
b38b0f
         break;
b38b0f
     case RAW_PL_COMMIT:
b38b0f
-        raw_apply_lock_bytes(s->lock_fd, new_perm, ~new_shared,
b38b0f
+        raw_apply_lock_bytes(s, s->lock_fd, new_perm, ~new_shared,
b38b0f
                              true, &local_err);
b38b0f
         if (local_err) {
b38b0f
             /* Theoretically the above call only unlocks bytes and it cannot
b38b0f
@@ -2160,7 +2194,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
b38b0f
     shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
b38b0f
 
b38b0f
     /* Step one: Take locks */
b38b0f
-    result = raw_apply_lock_bytes(fd, perm, ~shared, false, errp);
b38b0f
+    result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp);
b38b0f
     if (result < 0) {
b38b0f
         goto out_close;
b38b0f
     }
b38b0f
@@ -2204,7 +2238,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
b38b0f
     }
b38b0f
 
b38b0f
 out_unlock:
b38b0f
-    raw_apply_lock_bytes(fd, 0, 0, true, &local_err);
b38b0f
+    raw_apply_lock_bytes(NULL, fd, 0, 0, true, &local_err);
b38b0f
     if (local_err) {
b38b0f
         /* The above call should not fail, and if it does, that does
b38b0f
          * not mean the whole creation operation has failed.  So
b38b0f
-- 
b38b0f
1.8.3.1
b38b0f