A bdrv_getlength() call can fail and return a negative value. This
is not being handled in quorum_co_flush(), which can result in a
QUORUM_REPORT_BAD event with an arbitrary value on the 'sectors-count'
field.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/quorum.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/quorum.c b/block/quorum.c
index 55ba916655..d77991d680 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -785,8 +785,9 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
     for (i = 0; i < s->num_children; i++) {
         result = bdrv_co_flush(s->children[i]->bs);
         if (result) {
+            int64_t length = bdrv_getlength(s->children[i]->bs);
             quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
-                              bdrv_getlength(s->children[i]->bs),
+                              length > 0 ? length : 0,
                               s->children[i]->bs->node_name, result);
             result_value.l = result;
             quorum_count_vote(&error_votes, &result_value, i);
-- 
2.11.0

On 08/04/2017 09:08 AM, Alberto Garcia wrote:
> A bdrv_getlength() call can fail and return a negative value. This
> is not being handled in quorum_co_flush(), which can result in a
> QUORUM_REPORT_BAD event with an arbitrary value on the 'sectors-count'
> field.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/quorum.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 55ba916655..d77991d680 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -785,8 +785,9 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
>      for (i = 0; i < s->num_children; i++) {
>          result = bdrv_co_flush(s->children[i]->bs);
>          if (result) {
> +            int64_t length = bdrv_getlength(s->children[i]->bs);
>              quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
> -                              bdrv_getlength(s->children[i]->bs),
> +                              length > 0 ? length : 0,

In the fallback case, is always picking 0 good enough?  Then again, this
is in the error path, so it is unlikely in practice, and I don't see any
better way to handle it.

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

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

On Fri 04 Aug 2017 05:44:00 PM CEST, Eric Blake wrote:
>> --- a/block/quorum.c
>> +++ b/block/quorum.c
>> @@ -785,8 +785,9 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
>>      for (i = 0; i < s->num_children; i++) {
>>          result = bdrv_co_flush(s->children[i]->bs);
>>          if (result) {
>> +            int64_t length = bdrv_getlength(s->children[i]->bs);
>>              quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
>> -                              bdrv_getlength(s->children[i]->bs),
>> +                              length > 0 ? length : 0,
>
> In the fallback case, is always picking 0 good enough?  Then again,
> this is in the error path, so it is unlikely in practice, and I don't
> see any better way to handle it.

I don't see any better way to handle it either, and I'm not sure that it
matters much: this is a flush error event, the 'sectors-count' field
doesn't even mean anything, but we have to put something there.

Berto

On 08/07/2017 03:43 AM, Alberto Garcia wrote:
> On Fri 04 Aug 2017 05:44:00 PM CEST, Eric Blake wrote:
>>> --- a/block/quorum.c
>>> +++ b/block/quorum.c
>>> @@ -785,8 +785,9 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
>>>      for (i = 0; i < s->num_children; i++) {
>>>          result = bdrv_co_flush(s->children[i]->bs);
>>>          if (result) {
>>> +            int64_t length = bdrv_getlength(s->children[i]->bs);
>>>              quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
>>> -                              bdrv_getlength(s->children[i]->bs),
>>> +                              length > 0 ? length : 0,
>>
>> In the fallback case, is always picking 0 good enough?  Then again,
>> this is in the error path, so it is unlikely in practice, and I don't
>> see any better way to handle it.
> 
> I don't see any better way to handle it either, and I'm not sure that it
> matters much: this is a flush error event, the 'sectors-count' field
> doesn't even mean anything, but we have to put something there.

If that's the case, can we ALWAYS report 0, instead of usually the
length and sometimes 0?

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

On Mon 07 Aug 2017 01:29:09 PM CEST, Eric Blake wrote:
> On 08/07/2017 03:43 AM, Alberto Garcia wrote:
>> On Fri 04 Aug 2017 05:44:00 PM CEST, Eric Blake wrote:
>>>> --- a/block/quorum.c
>>>> +++ b/block/quorum.c
>>>> @@ -785,8 +785,9 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
>>>>      for (i = 0; i < s->num_children; i++) {
>>>>          result = bdrv_co_flush(s->children[i]->bs);
>>>>          if (result) {
>>>> +            int64_t length = bdrv_getlength(s->children[i]->bs);
>>>>              quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
>>>> -                              bdrv_getlength(s->children[i]->bs),
>>>> +                              length > 0 ? length : 0,
>>>
>>> In the fallback case, is always picking 0 good enough?  Then again,
>>> this is in the error path, so it is unlikely in practice, and I don't
>>> see any better way to handle it.
>> 
>> I don't see any better way to handle it either, and I'm not sure that it
>> matters much: this is a flush error event, the 'sectors-count' field
>> doesn't even mean anything, but we have to put something there.
>
> If that's the case, can we ALWAYS report 0, instead of usually the
> length and sometimes 0?

That's actually not a bad idea, I'll send the patch now.

Berto