[Qemu-devel] [PATCH] nbd/server: fix bitmap export

[Qemu-devel] [PATCH] nbd/server: fix bitmap export

[Vladimir Sementsov-Ogievskiy]

bitmap_to_extents function is broken: it switches dirty variable after
every iteration, however it can process only part of dirty (or zero)
area during one iteration in case when this area is too large for one
extent.

Fortunately, the bug don't produce wrong extents: it just inserts
zero-length extents between sequential extents representing large dirty
(or zero) area. However, zero-length extents are abandoned by NBD
protocol. So, careful client should consider such replay as server
fault and not-careful will likely ignore zero-length extents.

Fix this by more careful handling of dirty variable.

Bug was introduced in 3d068aff16
 "nbd/server: implement dirty bitmap export", with the whole function.
and presents in v3.0.0 release.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index ea5fe0eb33..12f721482d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1951,6 +1951,8 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
 
     assert(begin < overall_end && nb_extents);
     while (begin < overall_end && i < nb_extents) {
+        bool next_dirty = !dirty;
+
         if (dirty) {
             end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
         } else {
@@ -1962,6 +1964,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
             end = MIN(bdrv_dirty_bitmap_size(bitmap),
                       begin + UINT32_MAX + 1 -
                       bdrv_dirty_bitmap_granularity(bitmap));
+            next_dirty = dirty;
         }
         if (dont_fragment && end > overall_end) {
             end = overall_end;
@@ -1971,7 +1974,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
         extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);
         i++;
         begin = end;
-        dirty = !dirty;
+        dirty = next_dirty;
     }
 
     bdrv_dirty_iter_free(it);
-- 
2.18.0

Re: [Qemu-devel] [PATCH] nbd/server: fix bitmap export

[Eric Blake]

On 9/14/18 11:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> bitmap_to_extents function is broken: it switches dirty variable after
> every iteration, however it can process only part of dirty (or zero)
> area during one iteration in case when this area is too large for one
> extent.
> 
> Fortunately, the bug don't produce wrong extents: it just inserts
> zero-length extents between sequential extents representing large dirty
> (or zero) area. However, zero-length extents are abandoned by NBD

s/abandoned by/forbidden by the/

> protocol. So, careful client should consider such replay as server

s/replay/reply/

> fault and not-careful will likely ignore zero-length extents.

Which camp is qemu 3.0 as client in? Does it tolerate the zero-length 
extent, and still manage to see correct information overall, or does it 
crash?

Hmm - I think we're "safe" with qemu as client - right now, the only way 
qemu 3.0 accesses the qemu dirty bitmap over NBD is with my 
x-dirty-bitmap hack (commit 216ee3657), which uses 
block/nbd-client.c:nbd_client_co_block_status() to read the bitmap, and 
that always passes NBD_CMD_FLAG_REQ_ONE.  qemu will assert() if 
nbd_client_co_block_status() doesn't make any progress, but from what 
I'm reading of your bug report, qemu as client never permits the server 
to answer with more than one extent, and the bug of a zero-length extent 
is triggered only after the first extent has been sent.

Thus, the primary reason to accept this patch is not because of qemu 3.0 
as client, but for interoperability with other clients.  I'm planning on 
updating the commit message to add these additional details.

> 
> Fix this by more careful handling of dirty variable.
> 
> Bug was introduced in 3d068aff16
>   "nbd/server: implement dirty bitmap export", with the whole function.
> and presents in v3.0.0 release.

s/presents/present/

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

CC: qemu-stable@nongnu.org

> ---
>   nbd/server.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index ea5fe0eb33..12f721482d 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1951,6 +1951,8 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>   
>       assert(begin < overall_end && nb_extents);
>       while (begin < overall_end && i < nb_extents) {
> +        bool next_dirty = !dirty;
> +
>           if (dirty) {
>               end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
>           } else {
> @@ -1962,6 +1964,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>               end = MIN(bdrv_dirty_bitmap_size(bitmap),
>                         begin + UINT32_MAX + 1 -
>                         bdrv_dirty_bitmap_granularity(bitmap));
> +            next_dirty = dirty;
>           }

Rather than introducing next_dirty, couldn't you just make this:

if (end == -1 || end - begin > UINT32_MAX) {
     /* Cap ... */
     end = MIN(...);
} else {
     dirty = !dirty;
}

>           if (dont_fragment && end > overall_end) {
>               end = overall_end;
> @@ -1971,7 +1974,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>           extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);
>           i++;
>           begin = end;
> -        dirty = !dirty;
> +        dirty = next_dirty;
>       }

and then you merely delete the assignment to 'dirty' here.

>   
>       bdrv_dirty_iter_free(it);
> 

At any rate, the fix makes sense to me.  Should I go ahead and 
incorporate the changes I've suggested and turn it into a pull request 
through my NBD tree, or would you like to send a v2?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] nbd/server: fix bitmap export

[Vladimir Sementsov-Ogievskiy]

14.09.2018 20:24, Eric Blake wrote:
> On 9/14/18 11:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>> bitmap_to_extents function is broken: it switches dirty variable after
>> every iteration, however it can process only part of dirty (or zero)
>> area during one iteration in case when this area is too large for one
>> extent.
>>
>> Fortunately, the bug don't produce wrong extents: it just inserts
>> zero-length extents between sequential extents representing large dirty
>> (or zero) area. However, zero-length extents are abandoned by NBD
>
> s/abandoned by/forbidden by the/
>
>> protocol. So, careful client should consider such replay as server
>
> s/replay/reply/
>
>> fault and not-careful will likely ignore zero-length extents.
>
> Which camp is qemu 3.0 as client in? Does it tolerate the zero-length 
> extent, and still manage to see correct information overall, or does 
> it crash?
>
> Hmm - I think we're "safe" with qemu as client - right now, the only 
> way qemu 3.0 accesses the qemu dirty bitmap over NBD is with my 
> x-dirty-bitmap hack (commit 216ee3657), which uses 
> block/nbd-client.c:nbd_client_co_block_status() to read the bitmap, 
> and that always passes NBD_CMD_FLAG_REQ_ONE.  qemu will assert() if 
> nbd_client_co_block_status() doesn't make any progress, but from what 
> I'm reading of your bug report, qemu as client never permits the 
> server to answer with more than one extent, and the bug of a 
> zero-length extent is triggered only after the first extent has been 
> sent.
>
> Thus, the primary reason to accept this patch is not because of qemu 
> 3.0 as client, but for interoperability with other clients. I'm 
> planning on updating the commit message to add these additional details.
>
>>
>> Fix this by more careful handling of dirty variable.
>>
>> Bug was introduced in 3d068aff16
>>   "nbd/server: implement dirty bitmap export", with the whole function.
>> and presents in v3.0.0 release.
>
> s/presents/present/
>
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> CC: qemu-stable@nongnu.org
>
>> ---
>>   nbd/server.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index ea5fe0eb33..12f721482d 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1951,6 +1951,8 @@ static unsigned int 
>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>         assert(begin < overall_end && nb_extents);
>>       while (begin < overall_end && i < nb_extents) {
>> +        bool next_dirty = !dirty;
>> +
>>           if (dirty) {
>>               end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
>>           } else {
>> @@ -1962,6 +1964,7 @@ static unsigned int 
>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>               end = MIN(bdrv_dirty_bitmap_size(bitmap),
>>                         begin + UINT32_MAX + 1 -
>>                         bdrv_dirty_bitmap_granularity(bitmap));
>> +            next_dirty = dirty;
>>           }
>
> Rather than introducing next_dirty, couldn't you just make this:
>
> if (end == -1 || end - begin > UINT32_MAX) {
>     /* Cap ... */
>     end = MIN(...);
> } else {
>     dirty = !dirty;
> }

no, dirty variable is used after it, we can't change it here.

>
>>           if (dont_fragment && end > overall_end) {
>>               end = overall_end;
>> @@ -1971,7 +1974,7 @@ static unsigned int 
>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>           extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);
>>           i++;
>>           begin = end;
>> -        dirty = !dirty;
>> +        dirty = next_dirty;
>>       }
>
> and then you merely delete the assignment to 'dirty' here.
>
>>         bdrv_dirty_iter_free(it);
>>
>
> At any rate, the fix makes sense to me.  Should I go ahead and 
> incorporate the changes I've suggested and turn it into a pull request 
> through my NBD tree, or would you like to send a v2?
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] nbd/server: fix bitmap export

[Eric Blake]

On 9/14/18 12:30 PM, Vladimir Sementsov-Ogievskiy wrote:

>>>       while (begin < overall_end && i < nb_extents) {
>>> +        bool next_dirty = !dirty;
>>> +
>>>           if (dirty) {
>>>               end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
>>>           } else {
>>> @@ -1962,6 +1964,7 @@ static unsigned int 
>>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>>               end = MIN(bdrv_dirty_bitmap_size(bitmap),
>>>                         begin + UINT32_MAX + 1 -
>>>                         bdrv_dirty_bitmap_granularity(bitmap));
>>> +            next_dirty = dirty;
>>>           }
>>
>> Rather than introducing next_dirty, couldn't you just make this:
>>
>> if (end == -1 || end - begin > UINT32_MAX) {
>>     /* Cap ... */
>>     end = MIN(...);
>> } else {
>>     dirty = !dirty;
>> }
> 
> no, dirty variable is used after it, we can't change it here.

Ah, right. But we could also hoist the extents[i].flags = 
cpu_to_be32(dirty ? NBD_STATEE_DIRTY : 0) line to occur prior to the 
'if' doing the end capping calculation.  However, splitting the two 
assignments into extents[i].* to no longer be consecutive statements, 
just to avoid the use of a temporary variable, starts to get less 
aesthetically pleasing.

Thus, I'm fine with your version (with commit message improved), unless 
you want to send a v2.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] nbd/server: fix bitmap export

[Vladimir Sementsov-Ogievskiy]

14.09.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
> 14.09.2018 20:24, Eric Blake wrote:
>> On 9/14/18 11:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> bitmap_to_extents function is broken: it switches dirty variable after
>>> every iteration, however it can process only part of dirty (or zero)
>>> area during one iteration in case when this area is too large for one
>>> extent.
>>>
>>> Fortunately, the bug don't produce wrong extents: it just inserts
>>> zero-length extents between sequential extents representing large dirty
>>> (or zero) area. However, zero-length extents are abandoned by NBD
>>
>> s/abandoned by/forbidden by the/
>>
>>> protocol. So, careful client should consider such replay as server
>>
>> s/replay/reply/
>>
>>> fault and not-careful will likely ignore zero-length extents.
>>
>> Which camp is qemu 3.0 as client in? Does it tolerate the zero-length 
>> extent, and still manage to see correct information overall, or does 
>> it crash?
>>
>> Hmm - I think we're "safe" with qemu as client - right now, the only 
>> way qemu 3.0 accesses the qemu dirty bitmap over NBD is with my 
>> x-dirty-bitmap hack (commit 216ee3657), which uses 
>> block/nbd-client.c:nbd_client_co_block_status() to read the bitmap, 
>> and that always passes NBD_CMD_FLAG_REQ_ONE.  qemu will assert() if 
>> nbd_client_co_block_status() doesn't make any progress, but from what 
>> I'm reading of your bug report, qemu as client never permits the 
>> server to answer with more than one extent, and the bug of a 
>> zero-length extent is triggered only after the first extent has been 
>> sent.
>>
>> Thus, the primary reason to accept this patch is not because of qemu 
>> 3.0 as client, but for interoperability with other clients. I'm 
>> planning on updating the commit message to add these additional details.
>>
>>>
>>> Fix this by more careful handling of dirty variable.
>>>
>>> Bug was introduced in 3d068aff16
>>>   "nbd/server: implement dirty bitmap export", with the whole function.
>>> and presents in v3.0.0 release.
>>
>> s/presents/present/
>>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> CC: qemu-stable@nongnu.org
>>
>>> ---
>>>   nbd/server.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index ea5fe0eb33..12f721482d 100644
>>> --- a/nbd/server.c
>>> +++ b/nbd/server.c
>>> @@ -1951,6 +1951,8 @@ static unsigned int 
>>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>>         assert(begin < overall_end && nb_extents);
>>>       while (begin < overall_end && i < nb_extents) {
>>> +        bool next_dirty = !dirty;
>>> +
>>>           if (dirty) {
>>>               end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
>>>           } else {
>>> @@ -1962,6 +1964,7 @@ static unsigned int 
>>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>>               end = MIN(bdrv_dirty_bitmap_size(bitmap),
>>>                         begin + UINT32_MAX + 1 -
>>> bdrv_dirty_bitmap_granularity(bitmap));
>>> +            next_dirty = dirty;
>>>           }
>>
>> Rather than introducing next_dirty, couldn't you just make this:
>>
>> if (end == -1 || end - begin > UINT32_MAX) {
>>     /* Cap ... */
>>     end = MIN(...);
>> } else {
>>     dirty = !dirty;
>> }
>
> no, dirty variable is used after it, we can't change it here.

however, it can be done if we move

extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);

to be the first line of the loop body, so, with this movement, I'm ok 
with your changes and turning it into a pull.

>
>>
>>>           if (dont_fragment && end > overall_end) {
>>>               end = overall_end;
>>> @@ -1971,7 +1974,7 @@ static unsigned int 
>>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>>           extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);
>>>           i++;
>>>           begin = end;
>>> -        dirty = !dirty;
>>> +        dirty = next_dirty;
>>>       }
>>
>> and then you merely delete the assignment to 'dirty' here.
>>
>>>         bdrv_dirty_iter_free(it);
>>>
>>
>> At any rate, the fix makes sense to me.  Should I go ahead and 
>> incorporate the changes I've suggested and turn it into a pull 
>> request through my NBD tree, or would you like to send a v2?
>>
>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] nbd/server: fix bitmap export

[Vladimir Sementsov-Ogievskiy]

14.09.2018 20:35, Eric Blake wrote:
> On 9/14/18 12:30 PM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>>       while (begin < overall_end && i < nb_extents) {
>>>> +        bool next_dirty = !dirty;
>>>> +
>>>>           if (dirty) {
>>>>               end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
>>>>           } else {
>>>> @@ -1962,6 +1964,7 @@ static unsigned int 
>>>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>>>               end = MIN(bdrv_dirty_bitmap_size(bitmap),
>>>>                         begin + UINT32_MAX + 1 -
>>>> bdrv_dirty_bitmap_granularity(bitmap));
>>>> +            next_dirty = dirty;
>>>>           }
>>>
>>> Rather than introducing next_dirty, couldn't you just make this:
>>>
>>> if (end == -1 || end - begin > UINT32_MAX) {
>>>     /* Cap ... */
>>>     end = MIN(...);
>>> } else {
>>>     dirty = !dirty;
>>> }
>>
>> no, dirty variable is used after it, we can't change it here.
>
> Ah, right. But we could also hoist the extents[i].flags = 
> cpu_to_be32(dirty ? NBD_STATEE_DIRTY : 0) line to occur prior to the 
> 'if' doing the end capping calculation.  However, splitting the two 
> assignments into extents[i].* to no longer be consecutive statements, 
> just to avoid the use of a temporary variable, starts to get less 
> aesthetically pleasing.
>
> Thus, I'm fine with your version (with commit message improved), 
> unless you want to send a v2.

Ok, I don't want:) Be free to update commit message and pull it.

>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] nbd/server: fix bitmap export

[Eric Blake]

On 9/14/18 12:24 PM, Eric Blake wrote:
> On 9/14/18 11:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>> bitmap_to_extents function is broken: it switches dirty variable after
>> every iteration, however it can process only part of dirty (or zero)
>> area during one iteration in case when this area is too large for one
>> extent.
>>
>> Fortunately, the bug don't produce wrong extents: it just inserts
>> zero-length extents between sequential extents representing large dirty
>> (or zero) area. However, zero-length extents are abandoned by NBD
> 
> s/abandoned by/forbidden by the/
> 
>> protocol. So, careful client should consider such replay as server
> 
> s/replay/reply/
> 
>> fault and not-careful will likely ignore zero-length extents.
> 
> Which camp is qemu 3.0 as client in? Does it tolerate the zero-length 
> extent, and still manage to see correct information overall, or does it 
> crash?
> 
> Hmm - I think we're "safe" with qemu as client - right now, the only way 
> qemu 3.0 accesses the qemu dirty bitmap over NBD is with my 
> x-dirty-bitmap hack (commit 216ee3657), which uses 
> block/nbd-client.c:nbd_client_co_block_status() to read the bitmap, and 
> that always passes NBD_CMD_FLAG_REQ_ONE.  qemu will assert() if 
> nbd_client_co_block_status() doesn't make any progress, but from what 
> I'm reading of your bug report, qemu as client never permits the server 
> to answer with more than one extent, and the bug of a zero-length extent 
> is triggered only after the first extent has been sent.

A further mitigation - the bug can only occur for a client that requests 
block status with a length in the range (4G-bitmap_granularity, 4G). 
Otherwise, the loop terminates after the extent that got truncated to 
4G-bitmap_granularity, preventing the next iteration of the loop from 
detecting a 0-length extent.

> 
> Thus, the primary reason to accept this patch is not because of qemu 3.0 
> as client, but for interoperability with other clients.  I'm planning on 
> updating the commit message to add these additional details.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org