[PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted

[PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted

[Max Reitz]

Hi,

While reviewing Berto’s block-status/write-zeroes series for quorum, I
wondered how quorum’s permission code handles rewrite-corrupted.  It
turns out it doesn’t, and so qemu with a read-only rewrite-corrupted
quorum node simply crashes once there is a mismatch that leads to a
rewrite.

It looks to me like this bug has existed for quite some time, so I don’t
think this series must go into 5.2.  OTOH, it’s a simple bug fix, so I
suppose it might as well.


Max Reitz (3):
  quorum: Require WRITE perm with rewrite-corrupted
  iotests/081: Filter image format after testdir
  iotests/081: Test rewrite-corrupted without WRITE

 block/quorum.c             |  5 ++++
 tests/qemu-iotests/081     | 61 ++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/081.out | 27 +++++++++++++++++
 3 files changed, 90 insertions(+), 3 deletions(-)

-- 
2.28.0

[PATCH 1/3] quorum: Require WRITE perm with rewrite-corrupted

[Max Reitz]

Using rewrite-corrupted means quorum may issue writes to its children
just from receiving read requests from its parents.  Thus, it must take
the WRITE permission when rewrite-corrupted is used.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/quorum.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/quorum.c b/block/quorum.c
index b70d365ba9..01e49e94a3 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1195,7 +1195,12 @@ static void quorum_child_perm(BlockDriverState *bs, BdrvChild *c,
                               uint64_t perm, uint64_t shared,
                               uint64_t *nperm, uint64_t *nshared)
 {
+    BDRVQuorumState *s = bs->opaque;
+
     *nperm = perm & DEFAULT_PERM_PASSTHROUGH;
+    if (s->rewrite_corrupted) {
+        *nperm |= BLK_PERM_WRITE;
+    }
 
     /*
      * We cannot share RESIZE or WRITE, as this would make the
-- 
2.28.0

Re: [PATCH 1/3] quorum: Require WRITE perm with rewrite-corrupted

[Alberto Garcia]

On Fri 13 Nov 2020 10:17:16 PM CET, Max Reitz wrote:
> Using rewrite-corrupted means quorum may issue writes to its children
> just from receiving read requests from its parents.  Thus, it must take
> the WRITE permission when rewrite-corrupted is used.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[PATCH 2/3] iotests/081: Filter image format after testdir

[Max Reitz]

Otherwise, this breaks whenever the test directory contains the image
format (e.g. "/tmp/test-raw-file" is filtered to "/tmp/test-IMGFMT-file"
instead of "TEST_DIR").

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/081 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index 537d40dfd5..253b7576be 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -45,15 +45,16 @@ _require_drivers quorum
 
 do_run_qemu()
 {
-    echo Testing: "$@" | _filter_imgfmt
+    echo Testing: "$@"
     $QEMU -nographic -qmp stdio -serial none "$@"
     echo
 }
 
 run_qemu()
 {
-    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp\
-                          | _filter_qemu_io | _filter_generated_node_ids
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_imgfmt | _filter_qemu \
+                          | _filter_qmp | _filter_qemu_io \
+                          | _filter_generated_node_ids
 }
 
 quorum="driver=raw,file.driver=quorum,file.vote-threshold=2"
-- 
2.28.0

[PATCH 3/3] iotests/081: Test rewrite-corrupted without WRITE

[Max Reitz]

Test what happens when a rewrite-corrupted quorum node performs such a
rewrite, while there is no parent that has taken the WRITE permission.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/081     | 54 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/081.out | 27 +++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index 253b7576be..4e19972931 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -42,6 +42,7 @@ _supported_fmt raw
 _supported_proto file
 _supported_os Linux
 _require_drivers quorum
+_require_devices virtio-scsi
 
 do_run_qemu()
 {
@@ -155,6 +156,59 @@ echo "== checking that quorum has corrected the corrupted file =="
 
 $QEMU_IO -c "read -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io
 
+echo
+echo "== using quorum rewrite corrupted mode without WRITE permission =="
+
+# The same as above, but this time, do it on a quorum node whose only
+# parent will not take the WRITE permission
+
+echo '-- corrupting --'
+# Only corrupt a portion: The guest device (scsi-hd on virtio-scsi)
+# will read some data (looking for a partition table to guess the
+# disk's geometry), which would trigger a quorum mismatch if the
+# beginning of the image was corrupted.  The subsequent
+# QUORUM_REPORT_BAD event would be suppressed (because at that point,
+# there cannot have been a qmp_capabilities on the monitor).  Because
+# that event is rate-limited, the next QUORUM_REPORT_BAD that happens
+# thanks to our qemu-io read (which should trigger a mismatch) would
+# then be delayed past the VM quit and not appear in the output.
+# So we keep the first 1M intact to see a QUORUM_REPORT_BAD resulting
+# from the qemu-io invocation.
+$QEMU_IO -c "write -P 0x42 1M 1M" "$TEST_DIR/2.raw" | _filter_qemu_io
+
+# Fix the corruption (on a read-only quorum node, i.e. without taking
+# the WRITE permission on it -- its child nodes need to be R/W OTOH,
+# so that rewrite-corrupted works)
+echo
+echo '-- running quorum --'
+run_qemu \
+    -blockdev file,node-name=file1,filename="$TEST_DIR/1.raw" \
+    -blockdev file,node-name=file2,filename="$TEST_DIR/2.raw" \
+    -blockdev file,node-name=file3,filename="$TEST_DIR/3.raw" \
+    -blockdev '{
+        "driver": "quorum",
+        "node-name": "quorum",
+        "read-only": true,
+        "vote-threshold": 2,
+        "rewrite-corrupted": true,
+        "children": [ "file1", "file2", "file3" ]
+    }' \
+    -device virtio-scsi,id=scsi \
+    -device scsi-hd,id=quorum-drive,bus=scsi.0,drive=quorum \
+    <<EOF
+{ "execute": "qmp_capabilities" }
+{
+    "execute": "human-monitor-command",
+    "arguments": {
+        "command-line": 'qemu-io -d quorum-drive "read -P 0x32 0 $size"'
+    }
+}
+{ "execute": "quit" }
+EOF
+
+echo '-- checking that the image has been corrected --'
+$QEMU_IO -c "read -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io
+
 echo
 echo "== breaking quorum =="
 
diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out
index 04091b64e5..1974262fac 100644
--- a/tests/qemu-iotests/081.out
+++ b/tests/qemu-iotests/081.out
@@ -47,6 +47,33 @@ read 10485760/10485760 bytes at offset 0
 read 10485760/10485760 bytes at offset 0
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+== using quorum rewrite corrupted mode without WRITE permission ==
+-- corrupting --
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+-- running quorum --
+Testing: -blockdev file,node-name=file1,filename=TEST_DIR/1.IMGFMT -blockdev file,node-name=file2,filename=TEST_DIR/2.IMGFMT -blockdev file,node-name=file3,filename=TEST_DIR/3.IMGFMT -blockdev {
+        "driver": "quorum",
+        "node-name": "quorum",
+        "read-only": true,
+        "vote-threshold": 2,
+        "rewrite-corrupted": true,
+        "children": [ "file1", "file2", "file3" ]
+    } -device virtio-scsi,id=scsi -device scsi-hd,id=quorum-drive,bus=scsi.0,drive=quorum
+QMP_VERSION
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "QUORUM_REPORT_BAD", "data": {"node-name": "file2", "sectors-count": 20480, "sector-num": 0, "type": "read"}}
+read 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+-- checking that the image has been corrected --
+read 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 == breaking quorum ==
 wrote 10485760/10485760 bytes at offset 0
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.28.0

Re: [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted

[Kevin Wolf]

Am 13.11.2020 um 22:17 hat Max Reitz geschrieben:
> Hi,
> 
> While reviewing Berto’s block-status/write-zeroes series for quorum, I
> wondered how quorum’s permission code handles rewrite-corrupted.  It
> turns out it doesn’t, and so qemu with a read-only rewrite-corrupted
> quorum node simply crashes once there is a mismatch that leads to a
> rewrite.
> 
> It looks to me like this bug has existed for quite some time, so I don’t
> think this series must go into 5.2.  OTOH, it’s a simple bug fix, so I
> suppose it might as well.

Thanks, applied to the block branch.

Kevin