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
© 2016 - 2024 Red Hat, Inc.