[PATCH for-5.1 v2 0/2] qemu-img convert: Fix abort with unaligned image size

[PATCH for-5.1 v2 0/2] qemu-img convert: Fix abort with unaligned image size

[Kevin Wolf]

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1834646

Patch 1 fixes the assertion failure by failing gracefully when opening
an image whose size isn't aligned to the required request alignment.

Patch 2 relaxes the restrictions for NFS, which actually supports byte
alignment, but incorrectly gets a 4k request alignment in the file-posix
block driver.

v2:
- Don't fail opening unaligned images, but requesting WRITE permission
  without RESIZE. This keeps qcow2 images with unaligned metadata at EOF
  working. [Max]

Kevin Wolf (2):
  block: Require aligned image size to avoid assertion failure
  file-posix: Allow byte-aligned O_DIRECT with NFS

 block.c            | 16 ++++++++++++++++
 block/file-posix.c | 26 +++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

-- 
2.25.4

[PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure

[Kevin Wolf]

Unaligned requests will automatically be aligned to bl.request_alignment
and we can't extend write requests to access space beyond the end of the
image without resizing the image, so if we have the WRITE permission,
but not the RESIZE one, it's required that the image size is aligned.

Failing to meet this requirement could cause assertion failures like
this if RESIZE permissions weren't requested:

qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.

This was e.g. triggered by qemu-img converting to a target image with 4k
request alignment when the image was only aligned to 512 bytes, but not
to 4k.

Turn this into a graceful error in bdrv_check_perm() so that WRITE
without RESIZE can only be taken if the image size is aligned. If a user
holds both permissions and drops only RESIZE, the function will return
an error, but bdrv_child_try_set_perm() will ignore the failure silently
if permissions are only requested to be relaxed and just keep both
permissions while returning success.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/block.c b/block.c
index 35a372df57..6371928edb 100644
--- a/block.c
+++ b/block.c
@@ -2025,6 +2025,22 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
         return -EPERM;
     }
 
+    /*
+     * Unaligned requests will automatically be aligned to bl.request_alignment
+     * and without RESIZE we can't extend requests to write to space beyond the
+     * end of the image, so it's required that the image size is aligned.
+     */
+    if ((cumulative_perms & BLK_PERM_WRITE) &&
+        !(cumulative_perms & BLK_PERM_RESIZE))
+    {
+        if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
+            error_setg(errp, "Cannot get 'write' permission without 'resize': "
+                             "Image size is not a multiple of request "
+                             "alignment");
+            return -EPERM;
+        }
+    }
+
     /* Check this node */
     if (!drv) {
         return 0;
-- 
2.25.4

Re: [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2718 bytes --]

On 16.07.20 16:26, Kevin Wolf wrote:
> Unaligned requests will automatically be aligned to bl.request_alignment
> and we can't extend write requests to access space beyond the end of the
> image without resizing the image, so if we have the WRITE permission,
> but not the RESIZE one, it's required that the image size is aligned.
> 
> Failing to meet this requirement could cause assertion failures like
> this if RESIZE permissions weren't requested:
> 
> qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
> 
> This was e.g. triggered by qemu-img converting to a target image with 4k
> request alignment when the image was only aligned to 512 bytes, but not
> to 4k.
> 
> Turn this into a graceful error in bdrv_check_perm() so that WRITE
> without RESIZE can only be taken if the image size is aligned. If a user
> holds both permissions and drops only RESIZE, the function will return
> an error, but bdrv_child_try_set_perm() will ignore the failure silently
> if permissions are only requested to be relaxed and just keep both
> permissions while returning success.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 35a372df57..6371928edb 100644
> --- a/block.c
> +++ b/block.c
> @@ -2025,6 +2025,22 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>          return -EPERM;
>      }
>  
> +    /*
> +     * Unaligned requests will automatically be aligned to bl.request_alignment
> +     * and without RESIZE we can't extend requests to write to space beyond the
> +     * end of the image, so it's required that the image size is aligned.
> +     */
> +    if ((cumulative_perms & BLK_PERM_WRITE) &&

What about WRITE_UNCHANGED?  I think this would only matter with nodes
that can have backing files (i.e., qcow2 in practice) because
WRITE_UNCHANGED is only used by COR and block jobs doing something with
a backing chain, so it shouldn’t matter in practice, but, well.

So, either way:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +        !(cumulative_perms & BLK_PERM_RESIZE))
> +    {
> +        if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
> +            error_setg(errp, "Cannot get 'write' permission without 'resize': "
> +                             "Image size is not a multiple of request "
> +                             "alignment");
> +            return -EPERM;
> +        }
> +    }
> +
>      /* Check this node */
>      if (!drv) {
>          return 0;
> 



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

Re: [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure

[Kevin Wolf]

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

Am 17.07.2020 um 13:02 hat Max Reitz geschrieben:
> On 16.07.20 16:26, Kevin Wolf wrote:
> > Unaligned requests will automatically be aligned to bl.request_alignment
> > and we can't extend write requests to access space beyond the end of the
> > image without resizing the image, so if we have the WRITE permission,
> > but not the RESIZE one, it's required that the image size is aligned.
> > 
> > Failing to meet this requirement could cause assertion failures like
> > this if RESIZE permissions weren't requested:
> > 
> > qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
> > 
> > This was e.g. triggered by qemu-img converting to a target image with 4k
> > request alignment when the image was only aligned to 512 bytes, but not
> > to 4k.
> > 
> > Turn this into a graceful error in bdrv_check_perm() so that WRITE
> > without RESIZE can only be taken if the image size is aligned. If a user
> > holds both permissions and drops only RESIZE, the function will return
> > an error, but bdrv_child_try_set_perm() will ignore the failure silently
> > if permissions are only requested to be relaxed and just keep both
> > permissions while returning success.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 35a372df57..6371928edb 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2025,6 +2025,22 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
> >          return -EPERM;
> >      }
> >  
> > +    /*
> > +     * Unaligned requests will automatically be aligned to bl.request_alignment
> > +     * and without RESIZE we can't extend requests to write to space beyond the
> > +     * end of the image, so it's required that the image size is aligned.
> > +     */
> > +    if ((cumulative_perms & BLK_PERM_WRITE) &&
> 
> What about WRITE_UNCHANGED?  I think this would only matter with nodes
> that can have backing files (i.e., qcow2 in practice) because
> WRITE_UNCHANGED is only used by COR and block jobs doing something with
> a backing chain, so it shouldn’t matter in practice, but, well.

So basically just replacing the line with this?

    if ((cumulative_perms & (BLK_PERM_WRITE | BDRV_PERM_WRITE_UNCHANGED)) &&

I can do that while applying if it is what you mean.

> So, either way:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

Kevin

> > +        !(cumulative_perms & BLK_PERM_RESIZE))
> > +    {
> > +        if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
> > +            error_setg(errp, "Cannot get 'write' permission without 'resize': "
> > +                             "Image size is not a multiple of request "
> > +                             "alignment");
> > +            return -EPERM;
> > +        }
> > +    }
> > +
> >      /* Check this node */
> >      if (!drv) {
> >          return 0;
> > 
> 
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-5.1 v2 1/2] block: Require aligned image size to avoid assertion failure

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3195 bytes --]

On 17.07.20 13:32, Kevin Wolf wrote:
> Am 17.07.2020 um 13:02 hat Max Reitz geschrieben:
>> On 16.07.20 16:26, Kevin Wolf wrote:
>>> Unaligned requests will automatically be aligned to bl.request_alignment
>>> and we can't extend write requests to access space beyond the end of the
>>> image without resizing the image, so if we have the WRITE permission,
>>> but not the RESIZE one, it's required that the image size is aligned.
>>>
>>> Failing to meet this requirement could cause assertion failures like
>>> this if RESIZE permissions weren't requested:
>>>
>>> qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
>>>
>>> This was e.g. triggered by qemu-img converting to a target image with 4k
>>> request alignment when the image was only aligned to 512 bytes, but not
>>> to 4k.
>>>
>>> Turn this into a graceful error in bdrv_check_perm() so that WRITE
>>> without RESIZE can only be taken if the image size is aligned. If a user
>>> holds both permissions and drops only RESIZE, the function will return
>>> an error, but bdrv_child_try_set_perm() will ignore the failure silently
>>> if permissions are only requested to be relaxed and just keep both
>>> permissions while returning success.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 35a372df57..6371928edb 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2025,6 +2025,22 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>>>          return -EPERM;
>>>      }
>>>  
>>> +    /*
>>> +     * Unaligned requests will automatically be aligned to bl.request_alignment
>>> +     * and without RESIZE we can't extend requests to write to space beyond the
>>> +     * end of the image, so it's required that the image size is aligned.
>>> +     */
>>> +    if ((cumulative_perms & BLK_PERM_WRITE) &&
>>
>> What about WRITE_UNCHANGED?  I think this would only matter with nodes
>> that can have backing files (i.e., qcow2 in practice) because
>> WRITE_UNCHANGED is only used by COR and block jobs doing something with
>> a backing chain, so it shouldn’t matter in practice, but, well.
> 
> So basically just replacing the line with this?
> 
>     if ((cumulative_perms & (BLK_PERM_WRITE | BDRV_PERM_WRITE_UNCHANGED)) &&
> 
> I can do that while applying if it is what you mean.

Yes. :)

>> So, either way:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> Thanks!
> 
> Kevin
> 
>>> +        !(cumulative_perms & BLK_PERM_RESIZE))
>>> +    {
>>> +        if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
>>> +            error_setg(errp, "Cannot get 'write' permission without 'resize': "
>>> +                             "Image size is not a multiple of request "
>>> +                             "alignment");
>>> +            return -EPERM;
>>> +        }
>>> +    }
>>> +
>>>      /* Check this node */
>>>      if (!drv) {
>>>          return 0;
>>>
>>
>>
> 
> 
> 



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

[PATCH for-5.1 v2 2/2] file-posix: Allow byte-aligned O_DIRECT with NFS

[Kevin Wolf]

Since commit a6b257a08e3 ('file-posix: Handle undetectable alignment'),
we assume that if we open a file with O_DIRECT and alignment probing
returns 1, we just couldn't find out the real alignment requirement
because some filesystems make the requirement only for allocated blocks.
In this case, a safe default of 4k is used.

This is too strict for NFS, which does actually allow byte-aligned
requests even with O_DIRECT. Because we can't distinguish both cases
with generic code, let's just look at the file system magic and disable
s->needs_alignment for NFS. This way, O_DIRECT can still be used on NFS
for images that are not aligned to 4k.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/file-posix.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 8067e238cb..ae8190edab 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -62,10 +62,12 @@
 #include <sys/ioctl.h>
 #include <sys/param.h>
 #include <sys/syscall.h>
+#include <sys/vfs.h>
 #include <linux/cdrom.h>
 #include <linux/fd.h>
 #include <linux/fs.h>
 #include <linux/hdreg.h>
+#include <linux/magic.h>
 #include <scsi/sg.h>
 #ifdef __s390__
 #include <asm/dasd.h>
@@ -300,6 +302,28 @@ static int probe_physical_blocksize(int fd, unsigned int *blk_size)
 #endif
 }
 
+/*
+ * Returns true if no alignment restrictions are necessary even for files
+ * opened with O_DIRECT.
+ *
+ * raw_probe_alignment() probes the required alignment and assume that 1 means
+ * the probing failed, so it falls back to a safe default of 4k. This can be
+ * avoided if we know that byte alignment is okay for the file.
+ */
+static bool dio_byte_aligned(int fd)
+{
+#ifdef __linux__
+    struct statfs buf;
+    int ret;
+
+    ret = fstatfs(fd, &buf);
+    if (ret == 0 && buf.f_type == NFS_SUPER_MAGIC) {
+        return true;
+    }
+#endif
+    return false;
+}
+
 /* Check if read is allowed with given memory buffer and length.
  *
  * This function is used to check O_DIRECT memory buffer and request alignment.
@@ -629,7 +653,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 
     s->has_discard = true;
     s->has_write_zeroes = true;
-    if ((bs->open_flags & BDRV_O_NOCACHE) != 0) {
+    if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
         s->needs_alignment = true;
     }
 
-- 
2.25.4

Re: [PATCH for-5.1 v2 2/2] file-posix: Allow byte-aligned O_DIRECT with NFS

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 978 bytes --]

On 16.07.20 16:26, Kevin Wolf wrote:
> Since commit a6b257a08e3 ('file-posix: Handle undetectable alignment'),
> we assume that if we open a file with O_DIRECT and alignment probing
> returns 1, we just couldn't find out the real alignment requirement
> because some filesystems make the requirement only for allocated blocks.
> In this case, a safe default of 4k is used.
> 
> This is too strict for NFS, which does actually allow byte-aligned
> requests even with O_DIRECT. Because we can't distinguish both cases
> with generic code, let's just look at the file system magic and disable
> s->needs_alignment for NFS. This way, O_DIRECT can still be used on NFS
> for images that are not aligned to 4k.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/file-posix.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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