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

[Max Reitz]

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;
>>>
>>
>>
> 
> 
>

signature.asc
Description: OpenPGP digital signature