From b9aaae95df14d93aab128376c40943259a453730 Mon Sep 17 00:00:00 2001

From: Max Reitz <mreitz@redhat.com>

Date: Wed, 3 Apr 2019 17:13:10 +0100

Subject: [PATCH 03/11] file-posix: Skip effectiveless OFD lock operations

RH-Author: Max Reitz <mreitz@redhat.com>

Message-id: <20190403171315.20841-4-mreitz@redhat.com>

Patchwork-id: 85401

O-Subject: [RHEL-8.1 qemu-kvm PATCH 3/8] file-posix: Skip effectiveless OFD lock operations

Bugzilla: 1694148

RH-Acked-by: John Snow <jsnow@redhat.com>

RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

RH-Acked-by: Stefano Garzarella <sgarzare@redhat.com>

From: Fam Zheng <famz@redhat.com>

If we know we've already locked the bytes, don't do it again; similarly

don't unlock a byte if we haven't locked it. This doesn't change the

behavior, but fixes a corner case explained below.

Libvirt had an error handling bug that an image can get its (ownership,

file mode, SELinux) permissions changed (RHBZ 1584982) by mistake behind

QEMU. Specifically, an image in use by Libvirt VM has:

    $ ls -lhZ b.img

    -rw-r--r--. qemu qemu system_u:object_r:svirt_image_t:s0:c600,c690 b.img

Trying to attach it a second time won't work because of image locking.

And after the error, it becomes:

    $ ls -lhZ b.img

    -rw-r--r--. root root system_u:object_r:virt_image_t:s0 b.img

Then, we won't be able to do OFD lock operations with the existing fd.

In other words, the code such as in blk_detach_dev:

    blk_set_perm(blk, 0, BLK_PERM_ALL, &error_abort);

can abort() QEMU, out of environmental changes.

This patch is an easy fix to this and the change is regardlessly

reasonable, so do it.

Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Max Reitz <mreitz@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

(cherry picked from commit 2996ffad3acabe890fbb4f84a069cdc325a68108)

Signed-off-by: Max Reitz <mreitz@redhat.com>

Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>

---

 block/file-posix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++----------

 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c

index c2403ba..2a05193 100644

--- a/block/file-posix.c

+++ b/block/file-posix.c

@@ -152,6 +152,11 @@ typedef struct BDRVRawState {

     uint64_t perm;

     uint64_t shared_perm;

+    /* The perms bits whose corresponding bytes are already locked in

+     * s->lock_fd. */

+    uint64_t locked_perm;

+    uint64_t locked_shared_perm;

+

 #ifdef CONFIG_XFS

     bool is_xfs:1;

 #endif

@@ -677,43 +682,72 @@ typedef enum {

  * file; if @unlock == true, also unlock the unneeded bytes.

  * @shared_perm_lock_bits is the mask of all permissions that are NOT shared.

  */

-static int raw_apply_lock_bytes(int fd,

+static int raw_apply_lock_bytes(BDRVRawState *s, int fd,

                                 uint64_t perm_lock_bits,

                                 uint64_t shared_perm_lock_bits,

                                 bool unlock, Error **errp)

 {

     int ret;

     int i;

+    uint64_t locked_perm, locked_shared_perm;

+

+    if (s) {

+        locked_perm = s->locked_perm;

+        locked_shared_perm = s->locked_shared_perm;

+    } else {

+        /*

+         * We don't have the previous bits, just lock/unlock for each of the

+         * requested bits.

+         */

+        if (unlock) {

+            locked_perm = BLK_PERM_ALL;

+            locked_shared_perm = BLK_PERM_ALL;

+        } else {

+            locked_perm = 0;

+            locked_shared_perm = 0;

+        }

+    }

     PERM_FOREACH(i) {

         int off = RAW_LOCK_PERM_BASE + i;

-        if (perm_lock_bits & (1ULL << i)) {

+        uint64_t bit = (1ULL << i);

+        if ((perm_lock_bits & bit) && !(locked_perm & bit)) {

             ret = qemu_lock_fd(fd, off, 1, false);

             if (ret) {

                 error_setg(errp, "Failed to lock byte %d", off);

                 return ret;

+            } else if (s) {

+                s->locked_perm |= bit;

             }

-        } else if (unlock) {

+        } else if (unlock && (locked_perm & bit) && !(perm_lock_bits & bit)) {

             ret = qemu_unlock_fd(fd, off, 1);

             if (ret) {

                 error_setg(errp, "Failed to unlock byte %d", off);

                 return ret;

+            } else if (s) {

+                s->locked_perm &= ~bit;

             }

         }

     }

     PERM_FOREACH(i) {

         int off = RAW_LOCK_SHARED_BASE + i;

-        if (shared_perm_lock_bits & (1ULL << i)) {

+        uint64_t bit = (1ULL << i);

+        if ((shared_perm_lock_bits & bit) && !(locked_shared_perm & bit)) {

             ret = qemu_lock_fd(fd, off, 1, false);

             if (ret) {

                 error_setg(errp, "Failed to lock byte %d", off);

                 return ret;

+            } else if (s) {

+                s->locked_shared_perm |= bit;

             }

-        } else if (unlock) {

+        } else if (unlock && (locked_shared_perm & bit) &&

+                   !(shared_perm_lock_bits & bit)) {

             ret = qemu_unlock_fd(fd, off, 1);

             if (ret) {

                 error_setg(errp, "Failed to unlock byte %d", off);

                 return ret;

+            } else if (s) {

+                s->locked_shared_perm &= ~bit;

             }

         }

     }

@@ -781,7 +815,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,

     switch (op) {

     case RAW_PL_PREPARE:

-        ret = raw_apply_lock_bytes(s->lock_fd, s->perm | new_perm,

+        ret = raw_apply_lock_bytes(s, s->lock_fd, s->perm | new_perm,

                                    ~s->shared_perm | ~new_shared,

                                    false, errp);

         if (!ret) {

@@ -796,7 +830,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,

         op = RAW_PL_ABORT;

         /* fall through to unlock bytes. */

     case RAW_PL_ABORT:

-        raw_apply_lock_bytes(s->lock_fd, s->perm, ~s->shared_perm,

+        raw_apply_lock_bytes(s, s->lock_fd, s->perm, ~s->shared_perm,

                              true, &local_err);

         if (local_err) {

             /* Theoretically the above call only unlocks bytes and it cannot

@@ -806,7 +840,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,

         }

         break;

     case RAW_PL_COMMIT:

-        raw_apply_lock_bytes(s->lock_fd, new_perm, ~new_shared,

+        raw_apply_lock_bytes(s, s->lock_fd, new_perm, ~new_shared,

                              true, &local_err);

         if (local_err) {

             /* Theoretically the above call only unlocks bytes and it cannot

@@ -2160,7 +2194,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)

     shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;

     /* Step one: Take locks */

-    result = raw_apply_lock_bytes(fd, perm, ~shared, false, errp);

+    result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp);

     if (result < 0) {

         goto out_close;

     }

@@ -2204,7 +2238,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)

     }

 out_unlock:

-    raw_apply_lock_bytes(fd, 0, 0, true, &local_err);

+    raw_apply_lock_bytes(NULL, fd, 0, 0, true, &local_err);

     if (local_err) {

         /* The above call should not fail, and if it does, that does

          * not mean the whole creation operation has failed.  So

-- 

1.8.3.1