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

[Qemu-devel] [PATCH v2 0/3] 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 makes a function public that the second
patch uses to fix the issue by treating all non-writable images like
read-only images (yes, there is a difference...) in this regard (which
most importantly means not trying to set the corrupt flag on them).
Inactive images count as non-writable images, but not as read-only
images, so that fixes it.

The third patch adds an iotest case.


v2:
- Use bdrv_is_writable() instead of copying its functionality [Jeff]


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/3:[down] 'block: Make bdrv_is_writable() public'
002/3:[0004] [FC] 'qcow2: Do not mark inactive images corrupt'
003/3:[----] [--] 'iotests: Add case for a corrupted inactive image'


Max Reitz (3):
  block: Make bdrv_is_writable() public
  qcow2: Do not mark inactive images corrupt
  iotests: Add case for a corrupted inactive image

 include/block/block.h      |  1 +
 block.c                    | 17 ++++++++++++++---
 block/qcow2.c              |  2 +-
 tests/qemu-iotests/060     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out | 14 ++++++++++++++
 5 files changed, 60 insertions(+), 4 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v2 1/3] block: Make bdrv_is_writable() public

[Max Reitz]

This is a useful function for the whole block layer, so make it public.
At the same time, users outside of block.c probably do not need to make
use of the reopen functionality, so rename the current function to
bdrv_is_writable_after_reopen() create a new bdrv_is_writable() function
that just passes NULL to it for the reopen queue.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h |  1 +
 block.c               | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 4dd4f1eab2..e677080c4e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -408,6 +408,7 @@ bool bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
                            bool ignore_allow_rdw, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+bool bdrv_is_writable(BlockDriverState *bs);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
 void bdrv_lock_medium(BlockDriverState *bs, bool locked);
diff --git a/block.c b/block.c
index 9d577f65bb..50887087f3 100644
--- a/block.c
+++ b/block.c
@@ -1620,13 +1620,24 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q, BlockDriverState *bs)
 
 /* Returns whether the image file can be written to after the reopen queue @q
  * has been successfully applied, or right now if @q is NULL. */
-static bool bdrv_is_writable(BlockDriverState *bs, BlockReopenQueue *q)
+static bool bdrv_is_writable_after_reopen(BlockDriverState *bs,
+                                          BlockReopenQueue *q)
 {
     int flags = bdrv_reopen_get_flags(q, bs);
 
     return (flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) == BDRV_O_RDWR;
 }
 
+/*
+ * Return whether the BDS can be written to.  This is not necessarily
+ * the same as !bdrv_is_read_only(bs), as inactivated images may not
+ * be written to but do not count as read-only images.
+ */
+bool bdrv_is_writable(BlockDriverState *bs)
+{
+    return bdrv_is_writable_after_reopen(bs, NULL);
+}
+
 static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
                             BdrvChild *c, const BdrvChildRole *role,
                             BlockReopenQueue *reopen_queue,
@@ -1664,7 +1675,7 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
 
     /* Write permissions never work with read-only images */
     if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
-        !bdrv_is_writable(bs, q))
+        !bdrv_is_writable_after_reopen(bs, q))
     {
         error_setg(errp, "Block node is read-only");
         return -EPERM;
@@ -1956,7 +1967,7 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
                                   &perm, &shared);
 
         /* Format drivers may touch metadata even if the guest doesn't write */
-        if (bdrv_is_writable(bs, reopen_queue)) {
+        if (bdrv_is_writable_after_reopen(bs, reopen_queue)) {
             perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
         }
 
-- 
2.17.0

[Qemu-devel] [PATCH v2 2/3] 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.  bdrv_is_writable() can tell us whether an image can actually
be written to, so use its result instead of !bs->read_only.

(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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6b2d88759d..6fa5e1d71a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4569,7 +4569,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
     char *message;
     va_list ap;
 
-    fatal = fatal && !bs->read_only;
+    fatal = fatal && bdrv_is_writable(bs);
 
     if (s->signaled_corruption &&
         (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
-- 
2.17.0

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

[Max Reitz]

Reviewed-by: John Snow <jsnow@redhat.com>
Tested-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
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] [PATCH v2 0/3] qcow2: Do not mark inactive images corrupt

[Max Reitz]

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

On 2018-06-06 21:36, 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.

And I forgot to CC qemu-stable this time.

Oops ×2


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

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

[John Snow]

On 06/06/2018 03:36 PM, 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 makes a function public that the second
> patch uses to fix the issue by treating all non-writable images like
> read-only images (yes, there is a difference...) in this regard (which
> most importantly means not trying to set the corrupt flag on them).
> Inactive images count as non-writable images, but not as read-only
> images, so that fixes it.
> 
> The third patch adds an iotest case.
> 
> 
> v2:
> - Use bdrv_is_writable() instead of copying its functionality [Jeff]
> 
> 
> git-backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/3:[down] 'block: Make bdrv_is_writable() public'
> 002/3:[0004] [FC] 'qcow2: Do not mark inactive images corrupt'
> 003/3:[----] [--] 'iotests: Add case for a corrupted inactive image'
> 
> 
> Max Reitz (3):
>   block: Make bdrv_is_writable() public
>   qcow2: Do not mark inactive images corrupt
>   iotests: Add case for a corrupted inactive image
> 
>  include/block/block.h      |  1 +
>  block.c                    | 17 ++++++++++++++---
>  block/qcow2.c              |  2 +-
>  tests/qemu-iotests/060     | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/060.out | 14 ++++++++++++++
>  5 files changed, 60 insertions(+), 4 deletions(-)
> 

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

Re: [Qemu-devel] [PATCH v2 1/3] block: Make bdrv_is_writable() public

[Jeff Cody]

On Wed, Jun 06, 2018 at 09:37:00PM +0200, Max Reitz wrote:
> This is a useful function for the whole block layer, so make it public.
> At the same time, users outside of block.c probably do not need to make
> use of the reopen functionality, so rename the current function to
> bdrv_is_writable_after_reopen() create a new bdrv_is_writable() function
> that just passes NULL to it for the reopen queue.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Thanks!

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

> ---
>  include/block/block.h |  1 +
>  block.c               | 17 ++++++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 4dd4f1eab2..e677080c4e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -408,6 +408,7 @@ bool bdrv_is_read_only(BlockDriverState *bs);
>  int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
>                             bool ignore_allow_rdw, Error **errp);
>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> +bool bdrv_is_writable(BlockDriverState *bs);
>  bool bdrv_is_sg(BlockDriverState *bs);
>  bool bdrv_is_inserted(BlockDriverState *bs);
>  void bdrv_lock_medium(BlockDriverState *bs, bool locked);
> diff --git a/block.c b/block.c
> index 9d577f65bb..50887087f3 100644
> --- a/block.c
> +++ b/block.c
> @@ -1620,13 +1620,24 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q, BlockDriverState *bs)
>  
>  /* Returns whether the image file can be written to after the reopen queue @q
>   * has been successfully applied, or right now if @q is NULL. */
> -static bool bdrv_is_writable(BlockDriverState *bs, BlockReopenQueue *q)
> +static bool bdrv_is_writable_after_reopen(BlockDriverState *bs,
> +                                          BlockReopenQueue *q)
>  {
>      int flags = bdrv_reopen_get_flags(q, bs);
>  
>      return (flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) == BDRV_O_RDWR;
>  }
>  
> +/*
> + * Return whether the BDS can be written to.  This is not necessarily
> + * the same as !bdrv_is_read_only(bs), as inactivated images may not
> + * be written to but do not count as read-only images.
> + */
> +bool bdrv_is_writable(BlockDriverState *bs)
> +{
> +    return bdrv_is_writable_after_reopen(bs, NULL);
> +}
> +
>  static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
>                              BdrvChild *c, const BdrvChildRole *role,
>                              BlockReopenQueue *reopen_queue,
> @@ -1664,7 +1675,7 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>  
>      /* Write permissions never work with read-only images */
>      if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
> -        !bdrv_is_writable(bs, q))
> +        !bdrv_is_writable_after_reopen(bs, q))
>      {
>          error_setg(errp, "Block node is read-only");
>          return -EPERM;
> @@ -1956,7 +1967,7 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>                                    &perm, &shared);
>  
>          /* Format drivers may touch metadata even if the guest doesn't write */
> -        if (bdrv_is_writable(bs, reopen_queue)) {
> +        if (bdrv_is_writable_after_reopen(bs, reopen_queue)) {
>              perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
>          }
>  
> -- 
> 2.17.0
> 

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

[Jeff Cody]

On Wed, Jun 06, 2018 at 09:37:01PM +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.  bdrv_is_writable() can tell us whether an image can actually
> be written to, so use its result instead of !bs->read_only.
> 
> (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>

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

> ---
>  block/qcow2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6b2d88759d..6fa5e1d71a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4569,7 +4569,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
>      char *message;
>      va_list ap;
>  
> -    fatal = fatal && !bs->read_only;
> +    fatal = fatal && bdrv_is_writable(bs);
>  
>      if (s->signaled_corruption &&
>          (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
> -- 
> 2.17.0
> 

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

[Max Reitz]

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

On 2018-06-06 21:36, 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 makes a function public that the second
> patch uses to fix the issue by treating all non-writable images like
> read-only images (yes, there is a difference...) in this regard (which
> most importantly means not trying to set the corrupt flag on them).
> Inactive images count as non-writable images, but not as read-only
> images, so that fixes it.
> 
> The third patch adds an iotest case.

Thanks for the reviews, applied to my block branch.

Max


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