[Qemu-devel] [PATCH 0/2] qcow2: Do not mark inactive images corrupt

[Qemu-devel] [PATCH 0/2] qcow2: Do not mark inactive images corrupt

[Max Reitz]

The non-public logs in
https://bugzilla.redhat.com/show_bug.cgi?id=1583346 (sorry...) reveal
this problem:

$ (Create a qcow2 file "foo.qcow2" with a corrupted first L1 entry)
$ echo 'qemu-io none0 "read 0 512"' \
    | x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 \
                                        -monitor stdio \
                                        -incoming exec:'cat /dev/null'
QEMU 2.12.50 monitor - type 'help' for more information
(qemu) qemu-io none0 "read 0 512"
qcow2: Marking image as corrupt: L2 table offset 0x44200 unaligned (L1 index: 0); further corruption events will be suppressed
qemu-system-x86_64: block/io.c:1691: bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed.
[1]    18444 done                 echo 'qemu-io none0 "read 0 512"' | 
       18445 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 -monitor stdi

Oops.


The first patch in this series fixes this by treating inactive images
like read-only images in this regard (which most importantly means not
trying to set the corrupt flag on them), the second one adds an iotest
case.


Max Reitz (2):
  qcow2: Do not mark inactive images corrupt
  iotests: Add case for a corrupted inactive image

 block/qcow2.c              |  4 +++-
 tests/qemu-iotests/060     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out | 14 ++++++++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/2] qcow2: Do not mark inactive images corrupt

[Max Reitz]

When signaling a corruption on a read-only image, qcow2 already makes
fatal events non-fatal (i.e., they will not result in the image being
closed, and the image header's corrupt flag will not be set).  This is
necessary because we cannot set the corrupt flag on read-only images,
and it is possible because further corruption of read-only images is
impossible.

Inactive images are effectively read-only, too, so we should do the same
for them.

(Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in
bdrv_co_pwritev() will fail, crashing qemu.)

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 59a38b9cd3..8b5f7386f7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4402,7 +4402,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
     char *message;
     va_list ap;
 
-    fatal = fatal && !bs->read_only;
+    if ((bs->open_flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) != BDRV_O_RDWR) {
+        fatal = false;
+    }
 
     if (s->signaled_corruption &&
         (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
-- 
2.17.0

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] qcow2: Do not mark inactive images corrupt

[Jeff Cody]

On Mon, Jun 04, 2018 at 04:14:36PM +0200, Max Reitz wrote:
> When signaling a corruption on a read-only image, qcow2 already makes
> fatal events non-fatal (i.e., they will not result in the image being
> closed, and the image header's corrupt flag will not be set).  This is
> necessary because we cannot set the corrupt flag on read-only images,
> and it is possible because further corruption of read-only images is
> impossible.
> 
> Inactive images are effectively read-only, too, so we should do the same
> for them.
> 
> (Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in
> bdrv_co_pwritev() will fail, crashing qemu.)
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 59a38b9cd3..8b5f7386f7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4402,7 +4402,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
>      char *message;
>      va_list ap;
>  
> -    fatal = fatal && !bs->read_only;
> +    if ((bs->open_flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) != BDRV_O_RDWR) {

Hmm, this is pretty much exactly what the bdrv_is_writable() helper function
does in block.c; too bad it's scope is limited to block.c.  Maybe worth it
to make it a more widely available helper and use it here?

> +        fatal = false;
> +    }
>  
>      if (s->signaled_corruption &&
>          (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
> -- 
> 2.17.0
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] qcow2: Do not mark inactive images corrupt

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

On 2018-06-04 22:06, Jeff Cody wrote:
> On Mon, Jun 04, 2018 at 04:14:36PM +0200, Max Reitz wrote:
>> When signaling a corruption on a read-only image, qcow2 already makes
>> fatal events non-fatal (i.e., they will not result in the image being
>> closed, and the image header's corrupt flag will not be set).  This is
>> necessary because we cannot set the corrupt flag on read-only images,
>> and it is possible because further corruption of read-only images is
>> impossible.
>>
>> Inactive images are effectively read-only, too, so we should do the same
>> for them.
>>
>> (Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in
>> bdrv_co_pwritev() will fail, crashing qemu.)
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 59a38b9cd3..8b5f7386f7 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -4402,7 +4402,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
>>      char *message;
>>      va_list ap;
>>  
>> -    fatal = fatal && !bs->read_only;
>> +    if ((bs->open_flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) != BDRV_O_RDWR) {
> 
> Hmm, this is pretty much exactly what the bdrv_is_writable() helper function
> does in block.c; too bad it's scope is limited to block.c.  Maybe worth it
> to make it a more widely available helper and use it here?

You know what, I copied it from there but for some reason never thought
of making it global.

Will do.

Max

>> +        fatal = false;
>> +    }
>>  
>>      if (s->signaled_corruption &&
>>          (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
>> -- 
>> 2.17.0
>>
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 2/2] iotests: Add case for a corrupted inactive image

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/060     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out | 14 ++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 6c7407f499..7bdf609f3f 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -440,6 +440,36 @@ echo "{'execute': 'qmp_capabilities'}
             -drive if=none,node-name=drive,file="$TEST_IMG",driver=qcow2 \
     | _filter_qmp | _filter_qemu_io
 
+echo
+echo "=== Testing incoming inactive corrupted image ==="
+echo
+
+_make_test_img 64M
+# Create an unaligned L1 entry, so qemu will signal a corruption when
+# reading from the covered area
+poke_file "$TEST_IMG" "$l1_offset" "\x00\x00\x00\x00\x2a\x2a\x2a\x2a"
+
+# Inactive images are effectively read-only images, so this should be a
+# non-fatal corruption (which does not modify the image)
+echo "{'execute': 'qmp_capabilities'}
+      {'execute': 'human-monitor-command',
+       'arguments': {'command-line': 'qemu-io drive \"read 0 512\"'}}
+      {'execute': 'quit'}" \
+    | $QEMU -qmp stdio -nographic -nodefaults \
+            -blockdev "{'node-name': 'drive',
+                        'driver': 'qcow2',
+                        'file': {
+                            'driver': 'file',
+                            'filename': '$TEST_IMG'
+                        }}" \
+            -incoming exec:'cat /dev/null' \
+            2>&1 \
+    | _filter_qmp | _filter_qemu_io
+
+echo
+# Image should not have been marked corrupt
+_img_info --format-specific | grep 'corrupt:'
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 5f4264cff6..bff023d889 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -420,4 +420,18 @@ write failed: Input/output error
 {"return": ""}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+=== Testing incoming inactive corrupted image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+QMP_VERSION
+{"return": {}}
+qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1 index: 0); further non-fatal corruption events will be suppressed
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
+read failed: Input/output error
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+    corrupt: false
 *** done
-- 
2.17.0

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: Add case for a corrupted inactive image

[Jeff Cody]

On Mon, Jun 04, 2018 at 04:14:37PM +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Aborts without patch 1, passes with it, so a twofer:

Tested-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>


> ---
>  tests/qemu-iotests/060     | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/060.out | 14 ++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index 6c7407f499..7bdf609f3f 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -440,6 +440,36 @@ echo "{'execute': 'qmp_capabilities'}
>              -drive if=none,node-name=drive,file="$TEST_IMG",driver=qcow2 \
>      | _filter_qmp | _filter_qemu_io
>  
> +echo
> +echo "=== Testing incoming inactive corrupted image ==="
> +echo
> +
> +_make_test_img 64M
> +# Create an unaligned L1 entry, so qemu will signal a corruption when
> +# reading from the covered area
> +poke_file "$TEST_IMG" "$l1_offset" "\x00\x00\x00\x00\x2a\x2a\x2a\x2a"
> +
> +# Inactive images are effectively read-only images, so this should be a
> +# non-fatal corruption (which does not modify the image)
> +echo "{'execute': 'qmp_capabilities'}
> +      {'execute': 'human-monitor-command',
> +       'arguments': {'command-line': 'qemu-io drive \"read 0 512\"'}}
> +      {'execute': 'quit'}" \
> +    | $QEMU -qmp stdio -nographic -nodefaults \
> +            -blockdev "{'node-name': 'drive',
> +                        'driver': 'qcow2',
> +                        'file': {
> +                            'driver': 'file',
> +                            'filename': '$TEST_IMG'
> +                        }}" \
> +            -incoming exec:'cat /dev/null' \
> +            2>&1 \
> +    | _filter_qmp | _filter_qemu_io
> +
> +echo
> +# Image should not have been marked corrupt
> +_img_info --format-specific | grep 'corrupt:'
> +
>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
> index 5f4264cff6..bff023d889 100644
> --- a/tests/qemu-iotests/060.out
> +++ b/tests/qemu-iotests/060.out
> @@ -420,4 +420,18 @@ write failed: Input/output error
>  {"return": ""}
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
> +
> +=== Testing incoming inactive corrupted image ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +QMP_VERSION
> +{"return": {}}
> +qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1 index: 0); further non-fatal corruption events will be suppressed
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
> +read failed: Input/output error
> +{"return": ""}
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
> +
> +    corrupt: false
>  *** done
> -- 
> 2.17.0
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qcow2: Do not mark inactive images corrupt

[John Snow]

On 06/04/2018 10:14 AM, Max Reitz wrote:
> The non-public logs in
> https://bugzilla.redhat.com/show_bug.cgi?id=1583346 (sorry...) reveal
> this problem:
> 
> $ (Create a qcow2 file "foo.qcow2" with a corrupted first L1 entry)
> $ echo 'qemu-io none0 "read 0 512"' \
>     | x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 \
>                                         -monitor stdio \
>                                         -incoming exec:'cat /dev/null'
> QEMU 2.12.50 monitor - type 'help' for more information
> (qemu) qemu-io none0 "read 0 512"
> qcow2: Marking image as corrupt: L2 table offset 0x44200 unaligned (L1 index: 0); further corruption events will be suppressed
> qemu-system-x86_64: block/io.c:1691: bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed.
> [1]    18444 done                 echo 'qemu-io none0 "read 0 512"' | 
>        18445 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 -monitor stdi
> 
> Oops.
> 
> 
> The first patch in this series fixes this by treating inactive images
> like read-only images in this regard (which most importantly means not
> trying to set the corrupt flag on them), the second one adds an iotest
> case.
> 
> 
> Max Reitz (2):
>   qcow2: Do not mark inactive images corrupt
>   iotests: Add case for a corrupted inactive image
> 
>  block/qcow2.c              |  4 +++-
>  tests/qemu-iotests/060     | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/060.out | 14 ++++++++++++++
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 

Makes sense to me, provided it's safe to check via BDRV_O_RDWR instead
of bs->read_only. (I assume it is.)

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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qcow2: Do not mark inactive images corrupt

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

On 2018-06-04 20:58, John Snow wrote:
> 
> 
> On 06/04/2018 10:14 AM, Max Reitz wrote:
>> The non-public logs in
>> https://bugzilla.redhat.com/show_bug.cgi?id=1583346 (sorry...) reveal
>> this problem:
>>
>> $ (Create a qcow2 file "foo.qcow2" with a corrupted first L1 entry)
>> $ echo 'qemu-io none0 "read 0 512"' \
>>     | x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 \
>>                                         -monitor stdio \
>>                                         -incoming exec:'cat /dev/null'
>> QEMU 2.12.50 monitor - type 'help' for more information
>> (qemu) qemu-io none0 "read 0 512"
>> qcow2: Marking image as corrupt: L2 table offset 0x44200 unaligned (L1 index: 0); further corruption events will be suppressed
>> qemu-system-x86_64: block/io.c:1691: bdrv_co_pwritev: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed.
>> [1]    18444 done                 echo 'qemu-io none0 "read 0 512"' | 
>>        18445 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 -monitor stdi
>>
>> Oops.
>>
>>
>> The first patch in this series fixes this by treating inactive images
>> like read-only images in this regard (which most importantly means not
>> trying to set the corrupt flag on them), the second one adds an iotest
>> case.
>>
>>
>> Max Reitz (2):
>>   qcow2: Do not mark inactive images corrupt
>>   iotests: Add case for a corrupted inactive image
>>
>>  block/qcow2.c              |  4 +++-
>>  tests/qemu-iotests/060     | 30 ++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/060.out | 14 ++++++++++++++
>>  3 files changed, 47 insertions(+), 1 deletion(-)
>>
> 
> Makes sense to me, provided it's safe to check via BDRV_O_RDWR instead
> of bs->read_only. (I assume it is.)

It's all a mess is all I can say.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]