[Qemu-devel] [PULL 00/14] Block layer patches

[Qemu-devel] [PULL 00/14] Block layer patches

[Kevin Wolf]

The following changes since commit 5704c36d25ee84e7129722cb0db53df9faefe943:

  Merge remote-tracking branch 'remotes/kraxel/tags/fixes-31-20181112-pull-request' into staging (2018-11-12 15:55:40 +0000)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 1a42e5d8298d1b0f90d2254e7d559391dd3a45ca:

  Merge remote-tracking branch 'mreitz/tags/pull-block-2018-11-12' into queue-block (2018-11-12 17:57:32 +0100)

----------------------------------------------------------------
Block layer patches:

- file-posix: Don't waste a file descriptor for locking, don't lock the
  same bit multiple times
- nvme: Fix double free and memory leak
- Misc error handling fixes
- Added NULL checks found by static analysis
- Allow more block drivers to not be included in the qemu build

----------------------------------------------------------------
Fam Zheng (4):
      file-posix: Use error API properly
      file-posix: Skip effectiveless OFD lock operations
      file-posix: Drop s->lock_fd
      tests: Add unit tests for image locking

Jeff Cody (1):
      block: Make more block drivers compile-time configurable

Kevin Wolf (1):
      Merge remote-tracking branch 'mreitz/tags/pull-block-2018-11-12' into queue-block

Li Qiang (2):
      nvme: don't unref ctrl_mem when device unrealized
      nvme: free cmbuf in nvme_exit

Liam Merwick (5):
      job: Fix off-by-one assert checks for JobSTT and JobVerbTable
      block: Null pointer dereference in blk_root_get_parent_desc()
      qemu-img: assert block_job_get() does not return NULL in img_commit()
      block: Fix potential Null pointer dereferences in vvfat.c
      qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

Peter Maydell (1):
      blockdev: Consistently use snapshot_node_name in external_snapshot_prepare()

zhenwei pi (1):
      blockdev: handle error on block latency histogram set error

 configure                  |  91 ++++++++++++++++++++++++++
 block/block-backend.c      |   3 +-
 block/file-posix.c         | 122 ++++++++++++++++++++---------------
 block/qcow2-refcount.c     |  18 +++---
 block/vvfat.c              |  46 ++++++++-----
 blockdev.c                 |  21 ++++--
 hw/block/nvme.c            |   6 +-
 job.c                      |   4 +-
 qemu-img.c                 |   1 +
 tests/test-image-locking.c | 157 +++++++++++++++++++++++++++++++++++++++++++++
 block/Makefile.objs        |  22 +++++--
 tests/Makefile.include     |   2 +
 12 files changed, 400 insertions(+), 93 deletions(-)
 create mode 100644 tests/test-image-locking.c

[Qemu-devel] [PULL 01/14] file-posix: Use error API properly

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

Use error_report for situations that affect user operation (i.e.  we're
actually returning error), and warn_report/warn_report_err when some
less critical error happened but the user operation can still carry on.

For raw_normalize_devicepath, add Error parameter to propagate to
its callers.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 0c1b81ce4b..074f0ce2b3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -205,7 +205,7 @@ static int cdrom_reopen(BlockDriverState *bs);
 #endif
 
 #if defined(__NetBSD__)
-static int raw_normalize_devicepath(const char **filename)
+static int raw_normalize_devicepath(const char **filename, Error **errp)
 {
     static char namebuf[PATH_MAX];
     const char *dp, *fname;
@@ -214,8 +214,7 @@ static int raw_normalize_devicepath(const char **filename)
     fname = *filename;
     dp = strrchr(fname, '/');
     if (lstat(fname, &sb) < 0) {
-        fprintf(stderr, "%s: stat failed: %s\n",
-            fname, strerror(errno));
+        error_setg_errno(errp, errno, "%s: stat failed", fname);
         return -errno;
     }
 
@@ -229,14 +228,13 @@ static int raw_normalize_devicepath(const char **filename)
         snprintf(namebuf, PATH_MAX, "%.*s/r%s",
             (int)(dp - fname), fname, dp + 1);
     }
-    fprintf(stderr, "%s is a block device", fname);
     *filename = namebuf;
-    fprintf(stderr, ", using %s\n", *filename);
+    warn_report("%s is a block device, using %s", fname, *filename);
 
     return 0;
 }
 #else
-static int raw_normalize_devicepath(const char **filename)
+static int raw_normalize_devicepath(const char **filename, Error **errp)
 {
     return 0;
 }
@@ -461,9 +459,8 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 
     filename = qemu_opt_get(opts, "filename");
 
-    ret = raw_normalize_devicepath(&filename);
+    ret = raw_normalize_devicepath(&filename, errp);
     if (ret != 0) {
-        error_setg_errno(errp, -ret, "Could not normalize device path");
         goto fail;
     }
 
@@ -492,11 +489,10 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     case ON_OFF_AUTO_ON:
         s->use_lock = true;
         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");
+            warn_report("File lock requested but OFD locking syscall is "
+                        "unavailable, falling back to POSIX file locks");
+            error_printf("Due to the implementation, locks can be lost "
+                         "unexpectedly.\n");
         }
         break;
     case ON_OFF_AUTO_OFF:
@@ -818,7 +814,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
             /* Theoretically the above call only unlocks bytes and it cannot
              * fail. Something weird happened, report it.
              */
-            error_report_err(local_err);
+            warn_report_err(local_err);
         }
         break;
     case RAW_PL_COMMIT:
@@ -828,7 +824,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
             /* Theoretically the above call only unlocks bytes and it cannot
              * fail. Something weird happened, report it.
              */
-            error_report_err(local_err);
+            warn_report_err(local_err);
         }
         break;
     }
@@ -905,10 +901,8 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
     if (rs->fd == -1) {
         const char *normalized_filename = state->bs->filename;
-        ret = raw_normalize_devicepath(&normalized_filename);
-        if (ret < 0) {
-            error_setg_errno(errp, -ret, "Could not normalize device path");
-        } else {
+        ret = raw_normalize_devicepath(&normalized_filename, errp);
+        if (ret >= 0) {
             assert(!(rs->open_flags & O_CREAT));
             rs->fd = qemu_open(normalized_filename, rs->open_flags);
             if (rs->fd == -1) {
@@ -1788,7 +1782,7 @@ static int aio_worker(void *arg)
         ret = handle_aiocb_truncate(aiocb);
         break;
     default:
-        fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
+        error_report("invalid aio request (0x%x)", aiocb->aio_type);
         ret = -EINVAL;
         break;
     }
@@ -2276,7 +2270,7 @@ out_unlock:
          * not mean the whole creation operation has failed.  So
          * report it the user for their convenience, but do not report
          * it to the caller. */
-        error_report_err(local_err);
+        warn_report_err(local_err);
     }
 
 out_close:
@@ -3141,9 +3135,8 @@ static int coroutine_fn hdev_co_create_opts(const char *filename, QemuOpts *opts
 
     (void)has_prefix;
 
-    ret = raw_normalize_devicepath(&filename);
+    ret = raw_normalize_devicepath(&filename, errp);
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "Could not normalize device path");
         return ret;
     }
 
-- 
2.19.1

[Qemu-devel] [PULL 02/14] blockdev: handle error on block latency histogram set error

[Kevin Wolf]

From: zhenwei pi <pizhenwei@bytedance.com>

Function block_latency_histogram_set may return error, but qapi ignore this.
This can be reproduced easily by qmp command:
virsh qemu-monitor-command INSTANCE '{"execute":"x-block-latency-histogram-set",
"arguments":{"device":"drive-virtio-disk1","boundaries":[10,200,40]}}'
In fact this command does not work, but we still get success result.

qmp_x_block_latency_histogram_set is a batch setting API, report error ASAP.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e5b5eb46e2..9310ff3e7c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4413,6 +4413,7 @@ void qmp_x_block_latency_histogram_set(
 {
     BlockBackend *blk = blk_by_name(device);
     BlockAcctStats *stats;
+    int ret;
 
     if (!blk) {
         error_setg(errp, "Device '%s' not found", device);
@@ -4428,21 +4429,33 @@ void qmp_x_block_latency_histogram_set(
     }
 
     if (has_boundaries || has_boundaries_read) {
-        block_latency_histogram_set(
+        ret = block_latency_histogram_set(
             stats, BLOCK_ACCT_READ,
             has_boundaries_read ? boundaries_read : boundaries);
+        if (ret) {
+            error_setg(errp, "Device '%s' set read boundaries fail", device);
+            return;
+        }
     }
 
     if (has_boundaries || has_boundaries_write) {
-        block_latency_histogram_set(
+        ret = block_latency_histogram_set(
             stats, BLOCK_ACCT_WRITE,
             has_boundaries_write ? boundaries_write : boundaries);
+        if (ret) {
+            error_setg(errp, "Device '%s' set write boundaries fail", device);
+            return;
+        }
     }
 
     if (has_boundaries || has_boundaries_flush) {
-        block_latency_histogram_set(
+        ret = block_latency_histogram_set(
             stats, BLOCK_ACCT_FLUSH,
             has_boundaries_flush ? boundaries_flush : boundaries);
+        if (ret) {
+            error_setg(errp, "Device '%s' set flush boundaries fail", device);
+            return;
+        }
     }
 }
 
-- 
2.19.1

[Qemu-devel] [PULL 03/14] blockdev: Consistently use snapshot_node_name in external_snapshot_prepare()

[Kevin Wolf]

From: Peter Maydell <peter.maydell@linaro.org>

In the function external_snapshot_prepare() we have a
BlockdevSnapshotSync struct, which has the usual combination
of has_snapshot_node_name and snapshot_node_name fields for an
optional field. We set up a local variable
        const char *snapshot_node_name =
            s->has_snapshot_node_name ? s->snapshot_node_name : NULL;

and then mostly use "if (!snapshot_node_name)" for checking
whether we have a snapshot node name. The exception is that in
one place we check s->has_snapshot_node_name instead. This
confuses Coverity (CID 1396473), which thinks it might be
possible to get here with s->has_snapshot_node_name true but
snapshot_node_name NULL, and warns that the call to
qdict_put_str() will segfault in that case.

Make the code consistent and unconfuse Coverity by using
the same check for this conditional that we do in the rest
of the surrounding code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 9310ff3e7c..81f95d920b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1640,7 +1640,7 @@ static void external_snapshot_prepare(BlkActionState *common,
         }
 
         options = qdict_new();
-        if (s->has_snapshot_node_name) {
+        if (snapshot_node_name) {
             qdict_put_str(options, "node-name", snapshot_node_name);
         }
         qdict_put_str(options, "driver", format);
-- 
2.19.1

[Qemu-devel] [PULL 04/14] nvme: don't unref ctrl_mem when device unrealized

[Kevin Wolf]

From: Li Qiang <liq3ea@gmail.com>

Currently, when hotplug/unhotplug nvme device, it will cause an
assert in object.c. Following is the backtrack:

ERROR:qom/object.c:981:object_unref: assertion failed: (obj->ref > 0)

Thread 2 "qemu-system-x86" received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffcbd32700 (LWP 18844)]
0x00007fffdb9e4fff in raise () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
/lib/x86_64-linux-gnu/libglib-2.0.so.0
/lib/x86_64-linux-gnu/libglib-2.0.so.0
qom/object.c:981
/home/liqiang02/qemu-upstream/qemu/memory.c:1732
/home/liqiang02/qemu-upstream/qemu/memory.c:285
util/qemu-thread-posix.c:504
/lib/x86_64-linux-gnu/libpthread.so.0

This is caused by memory_region_unref in nvme_exit.

Remove it to make the PCIdevice refcount correct.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/nvme.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb816..359a06d0ad 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1331,9 +1331,6 @@ static void nvme_exit(PCIDevice *pci_dev)
     g_free(n->namespaces);
     g_free(n->cq);
     g_free(n->sq);
-    if (n->cmbsz) {
-        memory_region_unref(&n->ctrl_mem);
-    }
 
     msix_uninit_exclusive_bar(pci_dev);
 }
-- 
2.19.1

[Qemu-devel] [PULL 05/14] nvme: free cmbuf in nvme_exit

[Kevin Wolf]

From: Li Qiang <liq3ea@gmail.com>

This avoid a memory leak in unhotplug nvme device.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/nvme.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 359a06d0ad..09d7c90259 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1332,6 +1332,9 @@ static void nvme_exit(PCIDevice *pci_dev)
     g_free(n->cq);
     g_free(n->sq);
 
+    if (n->cmb_size_mb) {
+        g_free(n->cmbuf);
+    }
     msix_uninit_exclusive_bar(pci_dev);
 }
 
-- 
2.19.1

[Qemu-devel] [PULL 06/14] file-posix: Skip effectiveless OFD lock operations

[Kevin Wolf]

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>
---
 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 074f0ce2b3..1b4ff8de00 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
@@ -689,43 +694,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;
             }
         }
     }
@@ -793,7 +827,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) {
@@ -808,7 +842,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
@@ -818,7 +852,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
@@ -2220,7 +2254,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;
     }
@@ -2264,7 +2298,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
-- 
2.19.1

[Qemu-devel] [PULL 07/14] file-posix: Drop s->lock_fd

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

The lock_fd field is not strictly necessary because transferring locked
bytes from old fd to the new one shouldn't fail anyway. This spares the
user one fd per image.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1b4ff8de00..58c86a01ea 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -142,7 +142,6 @@ do { \
 
 typedef struct BDRVRawState {
     int fd;
-    int lock_fd;
     bool use_lock;
     int type;
     int open_flags;
@@ -153,7 +152,7 @@ typedef struct BDRVRawState {
     uint64_t shared_perm;
 
     /* The perms bits whose corresponding bytes are already locked in
-     * s->lock_fd. */
+     * s->fd. */
     uint64_t locked_perm;
     uint64_t locked_shared_perm;
 
@@ -551,18 +550,6 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
     s->fd = fd;
 
-    s->lock_fd = -1;
-    if (s->use_lock) {
-        fd = qemu_open(filename, s->open_flags);
-        if (fd < 0) {
-            ret = -errno;
-            error_setg_errno(errp, errno, "Could not open '%s' for locking",
-                             filename);
-            qemu_close(s->fd);
-            goto fail;
-        }
-        s->lock_fd = fd;
-    }
     s->perm = 0;
     s->shared_perm = BLK_PERM_ALL;
 
@@ -823,15 +810,13 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         return 0;
     }
 
-    assert(s->lock_fd > 0);
-
     switch (op) {
     case RAW_PL_PREPARE:
-        ret = raw_apply_lock_bytes(s, s->lock_fd, s->perm | new_perm,
+        ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
                                    ~s->shared_perm | ~new_shared,
                                    false, errp);
         if (!ret) {
-            ret = raw_check_lock_bytes(s->lock_fd, new_perm, new_shared, errp);
+            ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, errp);
             if (!ret) {
                 return 0;
             }
@@ -842,7 +827,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, s->lock_fd, s->perm, ~s->shared_perm,
+        raw_apply_lock_bytes(s, s->fd, s->perm, ~s->shared_perm,
                              true, &local_err);
         if (local_err) {
             /* Theoretically the above call only unlocks bytes and it cannot
@@ -852,7 +837,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         }
         break;
     case RAW_PL_COMMIT:
-        raw_apply_lock_bytes(s, s->lock_fd, new_perm, ~new_shared,
+        raw_apply_lock_bytes(s, s->fd, new_perm, ~new_shared,
                              true, &local_err);
         if (local_err) {
             /* Theoretically the above call only unlocks bytes and it cannot
@@ -967,10 +952,18 @@ static void raw_reopen_commit(BDRVReopenState *state)
 {
     BDRVRawReopenState *rs = state->opaque;
     BDRVRawState *s = state->bs->opaque;
+    Error *local_err = NULL;
 
     s->check_cache_dropped = rs->check_cache_dropped;
     s->open_flags = rs->open_flags;
 
+    /* Copy locks to the new fd before closing the old one. */
+    raw_apply_lock_bytes(NULL, rs->fd, s->locked_perm,
+                         ~s->locked_shared_perm, false, &local_err);
+    if (local_err) {
+        /* shouldn't fail in a sane host, but report it just in case. */
+        error_report_err(local_err);
+    }
     qemu_close(s->fd);
     s->fd = rs->fd;
 
@@ -1963,10 +1956,6 @@ static void raw_close(BlockDriverState *bs)
         qemu_close(s->fd);
         s->fd = -1;
     }
-    if (s->lock_fd >= 0) {
-        qemu_close(s->lock_fd);
-        s->lock_fd = -1;
-    }
 }
 
 /**
-- 
2.19.1

[Qemu-devel] [PULL 08/14] tests: Add unit tests for image locking

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/test-image-locking.c | 157 +++++++++++++++++++++++++++++++++++++
 tests/Makefile.include     |   2 +
 2 files changed, 159 insertions(+)
 create mode 100644 tests/test-image-locking.c

diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
new file mode 100644
index 0000000000..7614cbf90c
--- /dev/null
+++ b/tests/test-image-locking.c
@@ -0,0 +1,157 @@
+/*
+ * Image locking tests
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Author: Fam Zheng <famz@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "sysemu/block-backend.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+
+static BlockBackend *open_image(const char *path,
+                                uint64_t perm, uint64_t shared_perm,
+                                Error **errp)
+{
+    Error *local_err = NULL;
+    BlockBackend *blk;
+    QDict *options = qdict_new();
+
+    qdict_put_str(options, "driver", "raw");
+    blk = blk_new_open(path, NULL, options, BDRV_O_RDWR, &local_err);
+    if (blk) {
+        g_assert_null(local_err);
+        if (blk_set_perm(blk, perm, shared_perm, errp)) {
+            blk_unref(blk);
+            blk = NULL;
+        }
+    } else {
+        error_propagate(errp, local_err);
+    }
+    return blk;
+}
+
+static void check_locked_bytes(int fd, uint64_t perm_locks,
+                               uint64_t shared_perm_locks)
+{
+    int i;
+
+    if (!perm_locks && !shared_perm_locks) {
+        g_assert(!qemu_lock_fd_test(fd, 0, 0, true));
+        return;
+    }
+    for (i = 0; (1ULL << i) <= BLK_PERM_ALL; i++) {
+        uint64_t bit = (1ULL << i);
+        bool perm_expected = !!(bit & perm_locks);
+        bool shared_perm_expected = !!(bit & shared_perm_locks);
+        g_assert_cmpint(perm_expected, ==,
+                        !!qemu_lock_fd_test(fd, 100 + i, 1, true));
+        g_assert_cmpint(shared_perm_expected, ==,
+                        !!qemu_lock_fd_test(fd, 200 + i, 1, true));
+    }
+}
+
+static void test_image_locking_basic(void)
+{
+    BlockBackend *blk1, *blk2, *blk3;
+    char img_path[] = "/tmp/qtest.XXXXXX";
+    uint64_t perm, shared_perm;
+
+    int fd = mkstemp(img_path);
+    assert(fd >= 0);
+
+    perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
+    shared_perm = BLK_PERM_ALL;
+    blk1 = open_image(img_path, perm, shared_perm, &error_abort);
+    g_assert(blk1);
+
+    check_locked_bytes(fd, perm, ~shared_perm);
+
+    /* compatible perm between blk1 and blk2 */
+    blk2 = open_image(img_path, perm | BLK_PERM_RESIZE, shared_perm, NULL);
+    g_assert(blk2);
+    check_locked_bytes(fd, perm | BLK_PERM_RESIZE, ~shared_perm);
+
+    /* incompatible perm with already open blk1 and blk2 */
+    blk3 = open_image(img_path, perm, BLK_PERM_WRITE_UNCHANGED, NULL);
+    g_assert_null(blk3);
+
+    blk_unref(blk2);
+
+    /* Check that extra bytes in blk2 are correctly unlocked */
+    check_locked_bytes(fd, perm, ~shared_perm);
+
+    blk_unref(blk1);
+
+    /* Image is unused, no lock there */
+    check_locked_bytes(fd, 0, 0);
+    blk3 = open_image(img_path, perm, BLK_PERM_WRITE_UNCHANGED, &error_abort);
+    g_assert(blk3);
+    blk_unref(blk3);
+    close(fd);
+    unlink(img_path);
+}
+
+static void test_set_perm_abort(void)
+{
+    BlockBackend *blk1, *blk2;
+    char img_path[] = "/tmp/qtest.XXXXXX";
+    uint64_t perm, shared_perm;
+    int r;
+    int fd = mkstemp(img_path);
+    assert(fd >= 0);
+
+    perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
+    shared_perm = BLK_PERM_ALL;
+    blk1 = open_image(img_path, perm, shared_perm, &error_abort);
+    g_assert(blk1);
+
+    blk2 = open_image(img_path, perm, shared_perm, &error_abort);
+    g_assert(blk2);
+
+    check_locked_bytes(fd, perm, ~shared_perm);
+
+    /* A failed blk_set_perm mustn't change perm status (locked bytes) */
+    r = blk_set_perm(blk2, perm | BLK_PERM_RESIZE, BLK_PERM_WRITE_UNCHANGED,
+                     NULL);
+    g_assert_cmpint(r, !=, 0);
+    check_locked_bytes(fd, perm, ~shared_perm);
+    blk_unref(blk1);
+    blk_unref(blk2);
+}
+
+int main(int argc, char **argv)
+{
+    bdrv_init();
+    qemu_init_main_loop(&error_abort);
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (qemu_has_ofd_lock()) {
+        g_test_add_func("/image-locking/basic", test_image_locking_basic);
+        g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
+    }
+
+    return g_test_run();
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 074eece558..613242bc6e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -70,6 +70,7 @@ check-unit-y += tests/test-bdrv-drain$(EXESUF)
 check-unit-y += tests/test-blockjob$(EXESUF)
 check-unit-y += tests/test-blockjob-txn$(EXESUF)
 check-unit-y += tests/test-block-backend$(EXESUF)
+check-unit-y += tests/test-image-locking$(EXESUF)
 check-unit-y += tests/test-x86-cpuid$(EXESUF)
 # all code tested by test-x86-cpuid is inside topology.h
 ifeq ($(CONFIG_SOFTMMU),y)
@@ -537,6 +538,7 @@ tests/test-bdrv-drain$(EXESUF): tests/test-bdrv-drain.o $(test-block-obj-y) $(te
 tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-block-backend$(EXESUF): tests/test-block-backend.o $(test-block-obj-y) $(test-util-obj-y)
+tests/test-image-locking$(EXESUF): tests/test-image-locking.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
 tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) $(test-crypto-obj-y)
-- 
2.19.1

[Qemu-devel] [PULL 09/14] block: Make more block drivers compile-time configurable

[Kevin Wolf]

From: Jeff Cody <jcody@redhat.com>

This adds configure options to control the following block drivers:

* Bochs
* Cloop
* Dmg
* Qcow (V1)
* Vdi
* Vvfat
* qed
* parallels
* sheepdog

Each of these defaults to being enabled.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-id: 20181107063644.2254-1-armbru@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 configure           | 91 +++++++++++++++++++++++++++++++++++++++++++++
 block/Makefile.objs | 22 ++++++++---
 2 files changed, 107 insertions(+), 6 deletions(-)

diff --git a/configure b/configure
index 74e313a810..5b1d83ea26 100755
--- a/configure
+++ b/configure
@@ -470,6 +470,15 @@ tcmalloc="no"
 jemalloc="no"
 replication="yes"
 vxhs=""
+bochs="yes"
+cloop="yes"
+dmg="yes"
+qcow1="yes"
+vdi="yes"
+vvfat="yes"
+qed="yes"
+parallels="yes"
+sheepdog="yes"
 libxml2=""
 docker="no"
 debug_mutex="no"
@@ -1416,6 +1425,42 @@ for opt do
   ;;
   --enable-vxhs) vxhs="yes"
   ;;
+  --disable-bochs) bochs="no"
+  ;;
+  --enable-bochs) bochs="yes"
+  ;;
+  --disable-cloop) cloop="no"
+  ;;
+  --enable-cloop) cloop="yes"
+  ;;
+  --disable-dmg) dmg="no"
+  ;;
+  --enable-dmg) dmg="yes"
+  ;;
+  --disable-qcow1) qcow1="no"
+  ;;
+  --enable-qcow1) qcow1="yes"
+  ;;
+  --disable-vdi) vdi="no"
+  ;;
+  --enable-vdi) vdi="yes"
+  ;;
+  --disable-vvfat) vvfat="no"
+  ;;
+  --enable-vvfat) vvfat="yes"
+  ;;
+  --disable-qed) qed="no"
+  ;;
+  --enable-qed) qed="yes"
+  ;;
+  --disable-parallels) parallels="no"
+  ;;
+  --enable-parallels) parallels="yes"
+  ;;
+  --disable-sheepdog) sheepdog="no"
+  ;;
+  --enable-sheepdog) sheepdog="yes"
+  ;;
   --disable-vhost-user) vhost_user="no"
   ;;
   --enable-vhost-user)
@@ -1718,6 +1763,15 @@ disabled with --disable-FEATURE, default is enabled if available:
   qom-cast-debug  cast debugging support
   tools           build qemu-io, qemu-nbd and qemu-image tools
   vxhs            Veritas HyperScale vDisk backend support
+  bochs           bochs image format support
+  cloop           cloop image format support
+  dmg             dmg image format support
+  qcow1           qcow v1 image format support
+  vdi             vdi image format support
+  vvfat           vvfat image format support
+  qed             qed image format support
+  parallels       parallels image format support
+  sheepdog        sheepdog block driver support
   crypto-afalg    Linux AF_ALG crypto backend driver
   vhost-user      vhost-user support
   capstone        capstone disassembler support
@@ -6043,6 +6097,15 @@ echo "jemalloc support  $jemalloc"
 echo "avx2 optimization $avx2_opt"
 echo "replication support $replication"
 echo "VxHS block device $vxhs"
+echo "bochs support     $bochs"
+echo "cloop support     $cloop"
+echo "dmg support       $dmg"
+echo "qcow v1 support   $qcow1"
+echo "vdi support       $vdi"
+echo "vvfat support     $vvfat"
+echo "qed support       $qed"
+echo "parallels support $parallels"
+echo "sheepdog support  $sheepdog"
 echo "capstone          $capstone"
 echo "docker            $docker"
 echo "libpmem support   $libpmem"
@@ -6799,6 +6862,34 @@ if test "$libpmem" = "yes" ; then
   echo "CONFIG_LIBPMEM=y" >> $config_host_mak
 fi
 
+if test "$bochs" = "yes" ; then
+  echo "CONFIG_BOCHS=y" >> $config_host_mak
+fi
+if test "$cloop" = "yes" ; then
+  echo "CONFIG_CLOOP=y" >> $config_host_mak
+fi
+if test "$dmg" = "yes" ; then
+  echo "CONFIG_DMG=y" >> $config_host_mak
+fi
+if test "$qcow1" = "yes" ; then
+  echo "CONFIG_QCOW1=y" >> $config_host_mak
+fi
+if test "$vdi" = "yes" ; then
+  echo "CONFIG_VDI=y" >> $config_host_mak
+fi
+if test "$vvfat" = "yes" ; then
+  echo "CONFIG_VVFAT=y" >> $config_host_mak
+fi
+if test "$qed" = "yes" ; then
+  echo "CONFIG_QED=y" >> $config_host_mak
+fi
+if test "$parallels" = "yes" ; then
+  echo "CONFIG_PARALLELS=y" >> $config_host_mak
+fi
+if test "$sheepdog" = "yes" ; then
+  echo "CONFIG_SHEEPDOG=y" >> $config_host_mak
+fi
+
 if test "$tcg_interpreter" = "yes"; then
   QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
 elif test "$ARCH" = "sparc64" ; then
diff --git a/block/Makefile.objs b/block/Makefile.objs
index c8337bf186..46d585cfd0 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,10 +1,18 @@
-block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o
+block-obj-y += raw-format.o vmdk.o vpc.o
+block-obj-$(CONFIG_QCOW1) += qcow.o
+block-obj-$(CONFIG_VDI) += vdi.o
+block-obj-$(CONFIG_CLOOP) += cloop.o
+block-obj-$(CONFIG_BOCHS) += bochs.o
+block-obj-$(CONFIG_VVFAT) += vvfat.o
+block-obj-$(CONFIG_DMG) += dmg.o
+
 block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o
-block-obj-y += qed.o qed-l2-cache.o qed-table.o qed-cluster.o
-block-obj-y += qed-check.o
+block-obj-$(CONFIG_QED) += qed.o qed-l2-cache.o qed-table.o qed-cluster.o
+block-obj-$(CONFIG_QED) += qed-check.o
 block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
 block-obj-y += quorum.o
-block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
+block-obj-y += blkdebug.o blkverify.o blkreplay.o
+block-obj-$(CONFIG_PARALLELS) += parallels.o
 block-obj-y += blklogwrites.o
 block-obj-y += block-backend.o snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o
@@ -14,7 +22,8 @@ block-obj-y += null.o mirror.o commit.o io.o create.o
 block-obj-y += throttle-groups.o
 block-obj-$(CONFIG_LINUX) += nvme.o
 
-block-obj-y += nbd.o nbd-client.o sheepdog.o
+block-obj-y += nbd.o nbd-client.o
+block-obj-$(CONFIG_SHEEPDOG) += sheepdog.o
 block-obj-$(CONFIG_LIBISCSI) += iscsi.o
 block-obj-$(if $(CONFIG_LIBISCSI),y,n) += iscsi-opts.o
 block-obj-$(CONFIG_LIBNFS) += nfs.o
@@ -45,7 +54,8 @@ gluster.o-libs     := $(GLUSTERFS_LIBS)
 vxhs.o-libs        := $(VXHS_LIBS)
 ssh.o-cflags       := $(LIBSSH2_CFLAGS)
 ssh.o-libs         := $(LIBSSH2_LIBS)
-block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
+block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
+block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
 dmg-bz2.o-libs     := $(BZIP2_LIBS)
 qcow.o-libs        := -lz
 linux-aio.o-libs   := -laio
-- 
2.19.1

[Qemu-devel] [PULL 10/14] job: Fix off-by-one assert checks for JobSTT and JobVerbTable

[Kevin Wolf]

From: Liam Merwick <Liam.Merwick@oracle.com>

In the assert checking the array dereference of JobVerbTable[verb]
in job_apply_verb() the check of the index, verb, allows an overrun
because an index equal to the array size is permitted.

Similarly, in the assert check of JobSTT[s0][s1] with index s1
in job_state_transition(), an off-by-one overrun is not flagged
either.

This is not a run-time issue as there are no callers actually
passing in the max value.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 1541453919-25973-2-git-send-email-Liam.Merwick@oracle.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 job.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/job.c b/job.c
index c65e01bbfa..da8e4b7bf2 100644
--- a/job.c
+++ b/job.c
@@ -159,7 +159,7 @@ bool job_is_internal(Job *job)
 static void job_state_transition(Job *job, JobStatus s1)
 {
     JobStatus s0 = job->status;
-    assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
+    assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
     trace_job_state_transition(job, job->ret,
                                JobSTT[s0][s1] ? "allowed" : "disallowed",
                                JobStatus_str(s0), JobStatus_str(s1));
@@ -174,7 +174,7 @@ static void job_state_transition(Job *job, JobStatus s1)
 int job_apply_verb(Job *job, JobVerb verb, Error **errp)
 {
     JobStatus s0 = job->status;
-    assert(verb >= 0 && verb <= JOB_VERB__MAX);
+    assert(verb >= 0 && verb < JOB_VERB__MAX);
     trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
                          JobVerbTable[verb][s0] ? "allowed" : "prohibited");
     if (JobVerbTable[verb][s0]) {
-- 
2.19.1

[Qemu-devel] [PULL 11/14] block: Null pointer dereference in blk_root_get_parent_desc()

[Kevin Wolf]

From: Liam Merwick <Liam.Merwick@oracle.com>

The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL).

Instead of just checking this case before before dereferencing,
adjust blk_get_attached_dev_id() to return the empty string if no
object path can be found (similar to the case when blk->dev is NULL
and an empty string is returned).

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Message-id: 1541453919-25973-3-git-send-email-Liam.Merwick@oracle.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 2a8f3b55f8..60d37a0c3d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -918,7 +918,8 @@ char *blk_get_attached_dev_id(BlockBackend *blk)
     } else if (dev->id) {
         return g_strdup(dev->id);
     }
-    return object_get_canonical_path(OBJECT(dev));
+
+    return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");
 }
 
 /*
-- 
2.19.1

[Qemu-devel] [PULL 12/14] qemu-img: assert block_job_get() does not return NULL in img_commit()

[Kevin Wolf]

From: Liam Merwick <Liam.Merwick@oracle.com>

Although the function block_job_get() can return NULL, it would be a
serious bug if it did so (because the job yields before executing anything
(if it started successfully); but otherwise, commit_active_start() would
have returned an error).  However, as a precaution, before dereferencing
the 'job' pointer in img_commit() assert it is not NULL.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 1541453919-25973-4-git-send-email-Liam.Merwick@oracle.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-img.c b/qemu-img.c
index 4c96db7ba4..13a6ca31b4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1029,6 +1029,7 @@ static int img_commit(int argc, char **argv)
     }
 
     job = block_job_get("commit");
+    assert(job);
     run_block_job(job, &local_err);
     if (local_err) {
         goto unref_backing;
-- 
2.19.1

[Qemu-devel] [PULL 13/14] block: Fix potential Null pointer dereferences in vvfat.c

[Kevin Wolf]

From: Liam Merwick <Liam.Merwick@oracle.com>

The calls to find_mapping_for_cluster() may return NULL but it
isn't always checked for before dereferencing the value returned.
Additionally, add some asserts to cover cases where NULL can't
be returned but which might not be obvious at first glance.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Message-id: 1541453919-25973-5-git-send-email-Liam.Merwick@oracle.com
[mreitz: Dropped superfluous check of "mapping" following an assertion
         that it is not NULL, and fixed some indentation]
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vvfat.c | 46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index e4df255d58..1de5de1db4 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -100,30 +100,26 @@ static inline void array_free(array_t* array)
 /* does not automatically grow */
 static inline void* array_get(array_t* array,unsigned int index) {
     assert(index < array->next);
+    assert(array->pointer);
     return array->pointer + index * array->item_size;
 }
 
-static inline int array_ensure_allocated(array_t* array, int index)
+static inline void array_ensure_allocated(array_t *array, int index)
 {
     if((index + 1) * array->item_size > array->size) {
         int new_size = (index + 32) * array->item_size;
         array->pointer = g_realloc(array->pointer, new_size);
-        if (!array->pointer)
-            return -1;
+        assert(array->pointer);
         memset(array->pointer + array->size, 0, new_size - array->size);
         array->size = new_size;
         array->next = index + 1;
     }
-
-    return 0;
 }
 
 static inline void* array_get_next(array_t* array) {
     unsigned int next = array->next;
 
-    if (array_ensure_allocated(array, next) < 0)
-        return NULL;
-
+    array_ensure_allocated(array, next);
     array->next = next + 1;
     return array_get(array, next);
 }
@@ -2422,16 +2418,13 @@ static int commit_direntries(BDRVVVFATState* s,
     direntry_t* direntry = array_get(&(s->directory), dir_index);
     uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
     mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
-
     int factor = 0x10 * s->sectors_per_cluster;
     int old_cluster_count, new_cluster_count;
-    int current_dir_index = mapping->info.dir.first_dir_index;
-    int first_dir_index = current_dir_index;
+    int current_dir_index;
+    int first_dir_index;
     int ret, i;
     uint32_t c;
 
-DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapping->path, parent_mapping_index));
-
     assert(direntry);
     assert(mapping);
     assert(mapping->begin == first_cluster);
@@ -2439,6 +2432,11 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp
     assert(mapping->mode & MODE_DIRECTORY);
     assert(dir_index == 0 || is_directory(direntry));
 
+    DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n",
+                 mapping->path, parent_mapping_index));
+
+    current_dir_index = mapping->info.dir.first_dir_index;
+    first_dir_index = current_dir_index;
     mapping->info.dir.parent_mapping_index = parent_mapping_index;
 
     if (first_cluster == 0) {
@@ -2488,6 +2486,9 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp
         direntry = array_get(&(s->directory), first_dir_index + i);
         if (is_directory(direntry) && !is_dot(direntry)) {
             mapping = find_mapping_for_cluster(s, first_cluster);
+            if (mapping == NULL) {
+                return -1;
+            }
             assert(mapping->mode & MODE_DIRECTORY);
             ret = commit_direntries(s, first_dir_index + i,
                 array_index(&(s->mapping), mapping));
@@ -2516,6 +2517,10 @@ static int commit_one_file(BDRVVVFATState* s,
     assert(offset < size);
     assert((offset % s->cluster_size) == 0);
 
+    if (mapping == NULL) {
+        return -1;
+    }
+
     for (i = s->cluster_size; i < offset; i += s->cluster_size)
         c = modified_fat_get(s, c);
 
@@ -2662,8 +2667,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
         if (commit->action == ACTION_RENAME) {
             mapping_t* mapping = find_mapping_for_cluster(s,
                     commit->param.rename.cluster);
-            char* old_path = mapping->path;
+            char *old_path;
 
+            if (mapping == NULL) {
+                return -1;
+            }
+            old_path = mapping->path;
             assert(commit->path);
             mapping->path = commit->path;
             if (rename(old_path, mapping->path))
@@ -2684,10 +2693,15 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
                         direntry_t* d = direntry + i;
 
                         if (is_file(d) || (is_directory(d) && !is_dot(d))) {
+                            int l;
+                            char *new_path;
                             mapping_t* m = find_mapping_for_cluster(s,
                                     begin_of_direntry(d));
-                            int l = strlen(m->path);
-                            char* new_path = g_malloc(l + diff + 1);
+                            if (m == NULL) {
+                                return -1;
+                            }
+                            l = strlen(m->path);
+                            new_path = g_malloc(l + diff + 1);
 
                             assert(!strncmp(m->path, mapping->path, l2));
 
-- 
2.19.1

[Qemu-devel] [PULL 14/14] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

[Kevin Wolf]

From: Liam Merwick <Liam.Merwick@oracle.com>

The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not
add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to metadata_ol_names[].
As a result, an array dereference of metadata_ol_names[8] in
qcow2_pre_write_overlap_check() could result in a read outside of the array bounds.

Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory')

Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 1541453919-25973-6-git-send-email-Liam.Merwick@oracle.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c539f02e5..46082aeac1 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2719,15 +2719,17 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
 }
 
 static const char *metadata_ol_names[] = {
-    [QCOW2_OL_MAIN_HEADER_BITNR]    = "qcow2_header",
-    [QCOW2_OL_ACTIVE_L1_BITNR]      = "active L1 table",
-    [QCOW2_OL_ACTIVE_L2_BITNR]      = "active L2 table",
-    [QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
-    [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
-    [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
-    [QCOW2_OL_INACTIVE_L1_BITNR]    = "inactive L1 table",
-    [QCOW2_OL_INACTIVE_L2_BITNR]    = "inactive L2 table",
+    [QCOW2_OL_MAIN_HEADER_BITNR]        = "qcow2_header",
+    [QCOW2_OL_ACTIVE_L1_BITNR]          = "active L1 table",
+    [QCOW2_OL_ACTIVE_L2_BITNR]          = "active L2 table",
+    [QCOW2_OL_REFCOUNT_TABLE_BITNR]     = "refcount table",
+    [QCOW2_OL_REFCOUNT_BLOCK_BITNR]     = "refcount block",
+    [QCOW2_OL_SNAPSHOT_TABLE_BITNR]     = "snapshot table",
+    [QCOW2_OL_INACTIVE_L1_BITNR]        = "inactive L1 table",
+    [QCOW2_OL_INACTIVE_L2_BITNR]        = "inactive L2 table",
+    [QCOW2_OL_BITMAP_DIRECTORY_BITNR]   = "bitmap directory",
 };
+QEMU_BUILD_BUG_ON(QCOW2_OL_MAX_BITNR != ARRAY_SIZE(metadata_ol_names));
 
 /*
  * First performs a check for metadata overlaps (through
-- 
2.19.1

Re: [Qemu-devel] [PULL 00/14] Block layer patches

[Peter Maydell]

On 12 November 2018 at 17:05, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit 5704c36d25ee84e7129722cb0db53df9faefe943:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/fixes-31-20181112-pull-request' into staging (2018-11-12 15:55:40 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 1a42e5d8298d1b0f90d2254e7d559391dd3a45ca:
>
>   Merge remote-tracking branch 'mreitz/tags/pull-block-2018-11-12' into queue-block (2018-11-12 17:57:32 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - file-posix: Don't waste a file descriptor for locking, don't lock the
>   same bit multiple times
> - nvme: Fix double free and memory leak
> - Misc error handling fixes
> - Added NULL checks found by static analysis
> - Allow more block drivers to not be included in the qemu build
>

Applied, thanks.

-- PMM