[Qemu-devel] [PULL 0/2] Block patches

[Qemu-devel] [PULL 0/2] Block patches

[Stefan Hajnoczi]

The following changes since commit 2a5b58e2405e9fe42ba356b5a1b78146a4e9a659:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-vnc-20150312-1' into staging (2015-03-12 10:35:54 +0000)

are available in the git repository at:

  git://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 87b86e7ef29771a7fa06e3e8e88fa95bbc13a39c:

  qcow2: fix the macro QCOW_MAX_L1_SIZE's use (2015-03-12 17:41:23 +0000)

----------------------------------------------------------------

----------------------------------------------------------------

Paolo Bonzini (1):
  queue: fix QSLIST_INSERT_HEAD_ATOMIC race

Wen Congyang (1):
  qcow2: fix the macro QCOW_MAX_L1_SIZE's use

 block/qcow2-snapshot.c |  2 +-
 block/qcow2.c          |  2 +-
 include/qemu/queue.h   | 11 ++++++-----
 3 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.1.0

[Qemu-devel] [PULL 1/2] queue: fix QSLIST_INSERT_HEAD_ATOMIC race

[Stefan Hajnoczi]

From: Paolo Bonzini <pbonzini@redhat.com>

There is a not-so-subtle race in QSLIST_INSERT_HEAD_ATOMIC.

Because atomic_cmpxchg returns the old value instead of a success flag,
QSLIST_INSERT_HEAD_ATOMIC was checking for success by comparing against
the second argument to atomic_cmpxchg.  Unfortunately, this only works
if the second argument is a local or thread-local variable.

If it is in memory, it can be subject to common subexpression elimination
(and then everything's fine) or reloaded after the atomic_cmpxchg,
depending on the compiler's whims.  If the latter happens, the race can
happen.  A thread can sneak in, doing something on elm->field.sle_next
after the atomic_cmpxchg and before the comparison.  This causes a wrong
failure, and then two threads are using "elm" at the same time.  In the
case discovered by Christian, the sequence was likely something like this:

    thread 1                   | thread 2
    QSLIST_INSERT_HEAD_ATOMIC  |
      atomic_cmpxchg succeeds  |
      elm added to list        |
                               | steal release_pool
                               | QSLIST_REMOVE_HEAD
                               | elm removed from list
                               | ...
                               | QSLIST_INSERT_HEAD_ATOMIC
                               |   (overwrites sle_next)
      spurious failure         |
      atomic_cmpxchg succeeds  |
      elm added to list again  |
                               |
    steal release_pool         |
    QSLIST_REMOVE_HEAD         |
    elm removed again          |

The last three steps could be done by a third thread as well.
A reproducer that failed in a matter of seconds is as follows:

- the guest has 32 VCPUs on a 28 core host (hyperthreading was enabled),
  memory was 16G just to err on the safe side (the host has 64G, but hey
  at least you need no s390)

- the guest has 24 null-aio virtio-blk devices using dataplane
  (-object iothread,id=ioN -drive if=none,id=blkN,driver=null-aio,size=500G
  -device virtio-blk-pci,iothread=ioN,drive=blkN)

- the guest also has a single network interface.  It's only doing loopback
  tests so slirp vs. tap and the model doesn't matter.

- the guest is running fio with the following script:

     [global]
     rw=randread
     blocksize=16k
     ioengine=libaio
     runtime=10m
     buffered=0
     fallocate=none
     time_based
     iodepth=32

     [virtio1a]
     filename=/dev/block/252\:16

     [virtio1b]
     filename=/dev/block/252\:16

     ...

     [virtio24a]
     filename=/dev/block/252\:384

     [virtio24b]
     filename=/dev/block/252\:384

     [listen1]
     protocol=tcp
     ioengine=net
     port=12345
     listen
     rw=read
     bs=4k
     size=1000g

     [connect1]
     protocol=tcp
     hostname=localhost
     ioengine=net
     port=12345
     protocol=tcp
     rw=write
     startdelay=1
     size=1000g

     ...

     [listen8]
     protocol=tcp
     ioengine=net
     port=12352
     listen
     rw=read
     bs=4k
     size=1000g

     [connect8]
     protocol=tcp
     hostname=localhost
     ioengine=net
     port=12352
     rw=write
     startdelay=1
     size=1000g

Moral of the story: I should refrain from writing more clever stuff.
At least it looks like it is not too clever to be undebuggable.

Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 1426002357-6889-1-git-send-email-pbonzini@redhat.com
Fixes: c740ad92d0d958fa785e5d7aa1b67ecaf30a6a54
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/queue.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index 8094150..f781aa2 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -197,11 +197,12 @@ struct {                                                                \
         (head)->slh_first = (elm);                                       \
 } while (/*CONSTCOND*/0)
 
-#define QSLIST_INSERT_HEAD_ATOMIC(head, elm, field) do {                   \
-        do {                                                               \
-            (elm)->field.sle_next = (head)->slh_first;                     \
-        } while (atomic_cmpxchg(&(head)->slh_first, (elm)->field.sle_next, \
-                               (elm)) != (elm)->field.sle_next);           \
+#define QSLIST_INSERT_HEAD_ATOMIC(head, elm, field) do {                     \
+        typeof(elm) save_sle_next;                                           \
+        do {                                                                 \
+            save_sle_next = (elm)->field.sle_next = (head)->slh_first;       \
+        } while (atomic_cmpxchg(&(head)->slh_first, save_sle_next, (elm)) != \
+                 save_sle_next);                                             \
 } while (/*CONSTCOND*/0)
 
 #define QSLIST_MOVE_ATOMIC(dest, src) do {                               \
-- 
2.1.0

[Qemu-devel] [PULL 2/2] qcow2: fix the macro QCOW_MAX_L1_SIZE's use

[Stefan Hajnoczi]

From: Wen Congyang <wency@cn.fujitsu.com>

QCOW_MAX_L1_SIZE's unit is byte, and l1_size's unit
is l1 table entry size(8 bytes).

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Message-id: 54FFB0F1.5010307@cn.fujitsu.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-snapshot.c | 2 +-
 block/qcow2.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5b3903c..2aa9dcb 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -702,7 +702,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
     sn = &s->snapshots[snapshot_index];
 
     /* Allocate and read in the snapshot's L1 table */
-    if (sn->l1_size > QCOW_MAX_L1_SIZE) {
+    if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
         error_setg(errp, "Snapshot L1 table too large");
         return -EFBIG;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 8bfb094..32bdf75 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -742,7 +742,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* read the level 1 table */
-    if (header.l1_size > QCOW_MAX_L1_SIZE) {
+    if (header.l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
         error_setg(errp, "Active L1 table too large");
         ret = -EFBIG;
         goto fail;
-- 
2.1.0

Re: [Qemu-devel] [PULL 0/2] Block patches

[Peter Maydell]

On 12 March 2015 at 19:10, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The following changes since commit 2a5b58e2405e9fe42ba356b5a1b78146a4e9a659:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/pull-vnc-20150312-1' into staging (2015-03-12 10:35:54 +0000)
>
> are available in the git repository at:
>
>   git://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to 87b86e7ef29771a7fa06e3e8e88fa95bbc13a39c:
>
>   qcow2: fix the macro QCOW_MAX_L1_SIZE's use (2015-03-12 17:41:23 +0000)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM