Re: [Qemu-devel] [PATCH 0/2] block/vdi: Fix bmap writing error

[Max Reitz <mreitz@redhat.com>]

On 2014-10-23 at 09:07, Max Reitz wrote:
> On 2014-10-22 at 13:56, Kevin Wolf wrote:
>> Am 21.10.2014 um 10:51 hat Max Reitz geschrieben:
>>> The bmap size in block/vdi.c may exceed INT_MAX. Using
>>> bdrv_pwrite_sync() (which takes an int byte count) is therefore not a
>>> good idea. The second patch of this series fixes this by replacing
>>> bdrv_pwrite_sync() by bdrv_write()+bdrv_flush() (we don't need the p in
>>> pwrite here).
>>>
>>> The first patch employs ROUND_UP() and DIV_ROUND_UP() in 
>>> block/vdi.c, so
>>> you are reminded that bmap_size is aligned to BDRV_SECTOR_SIZE for the
>>> second patch.
>>>
>>> See https://bugzilla.redhat.com/show_bug.cgi?id=1154940 for a bug
>>> report.
>>>
>>> I will not include an iotest in this series because this would require
>>> qemu to allocate and then write about 2G of data; yes, test 1 in 084
>>> fails for me because qemu cannot allocate 4G for the bmap.
>>>
>>> In fact, I can only test this once I'm home where I have more RAM
>>> available (I made the mistake of activating swap space to test this 
>>> only
>>> once).
>> Thanks, applied to the block branch.
>
> So I tested it yesterday and it turns out it does not fix the bug. I'm 
> sorry, could you unapply patch 2?

I see you haven't dropped it yet. Well, I just thought about it and of 
course we can keep it in. It just doesn't fix the error its commit 
message pretends to, but it definitely doesn't make matters worse.

Max

> The reason it doesn't work is, as you personally said to me yesterday, 
> that bdrv_write() goes through the bdrv_pwrite() path as well. Well, 
> it does not really, but it does test whether nb_sectors > INT_MAX / 
> BDRV_SECTOR_SIZE. Therefore, writing the bitmap still fails with 
> EINVAL (for images >= 512 TB).
>
> Now, we could either actually fix the VDI code; or we simply limit the 
> maximum image size to half what it is now. I don't see a problem in 
> doing the latter, since qemu already does not support all image sizes, 
> so there's no reason why we should not limit the size even further. 
> Also, 512 TB seems plenty today.
>
> So unless someone is against this, I'll adjust VDI_BLOCKS_IN_IMAGE to 
> be INT_MAX / sizeof(uint32_t) (it's currently UINT32_MAX / 
> sizeof(uint32_t) == 0x3fffffff). This will be problematic if 
> sizeof(int) > sizeof(uint32_t), but I doubt that'll happen soon, if at 
> all.
>
> Max