diff mbox series

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

Message ID 20200716142601.111237-2-kwolf@redhat.com
State New, archived
Headers show
Series qemu-img convert: Fix abort with unaligned image size | expand

Commit Message

Kevin Wolf July 16, 2020, 2:26 p.m. UTC
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(+)

Comments

Max Reitz July 17, 2020, 11:02 a.m. UTC | #1
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;
>
Kevin Wolf July 17, 2020, 11:32 a.m. UTC | #2
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;
> > 
> 
>
Max Reitz July 17, 2020, 11:36 a.m. UTC | #3
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;
>>>
>>
>>
> 
> 
>
diff mbox series

Patch

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;