| Message ID | 1563529204-3368-8-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow Valgrind checking all QEMU processes | expand |
On 7/19/19 4:40 AM, Andrey Shinkevich wrote: > In case nbd_co_receive_one_chunk() fails in > nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in > the check nbd_reply_is_simple() without being initialized. The iotest > 083 does not pass under the Valgrind: $./check -nbd -valgrind 083. > The alternative solution is to swap the operands in the condition: > 'if (s->quit || nbd_reply_is_simple(reply))' > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/nbd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Huh. Very similar to https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but affects a different function. I can queue this one through my NBD tree to get both in my rc2 pull request. Reviewed-by: Eric Blake <eblake@redhat.com> > > diff --git a/block/nbd.c b/block/nbd.c > index 81edabb..8480ad4 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -786,7 +786,7 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle, > int *request_ret, Error **errp) > { > NBDReplyChunkIter iter; > - NBDReply reply; > + NBDReply reply = {}; > void *payload = NULL; > Error *local_err = NULL; > >
On 19/07/2019 17:34, Eric Blake wrote: > On 7/19/19 4:40 AM, Andrey Shinkevich wrote: >> In case nbd_co_receive_one_chunk() fails in >> nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in >> the check nbd_reply_is_simple() without being initialized. The iotest >> 083 does not pass under the Valgrind: $./check -nbd -valgrind 083. >> The alternative solution is to swap the operands in the condition: >> 'if (s->quit || nbd_reply_is_simple(reply))' >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> --- >> block/nbd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Huh. Very similar to > https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but > affects a different function. I can queue this one through my NBD tree > to get both in my rc2 pull request. > > Reviewed-by: Eric Blake <eblake@redhat.com> > Many thanks, Andrey >> >> diff --git a/block/nbd.c b/block/nbd.c >> index 81edabb..8480ad4 100644 >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -786,7 +786,7 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle, >> int *request_ret, Error **errp) >> { >> NBDReplyChunkIter iter; >> - NBDReply reply; >> + NBDReply reply = {}; >> void *payload = NULL; >> Error *local_err = NULL; >> >> >
On 7/19/19 9:34 AM, Eric Blake wrote: > On 7/19/19 4:40 AM, Andrey Shinkevich wrote: >> In case nbd_co_receive_one_chunk() fails in >> nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in >> the check nbd_reply_is_simple() without being initialized. The iotest >> 083 does not pass under the Valgrind: $./check -nbd -valgrind 083. >> The alternative solution is to swap the operands in the condition: >> 'if (s->quit || nbd_reply_is_simple(reply))' >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> --- >> block/nbd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Huh. Very similar to > https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but > affects a different function. I can queue this one through my NBD tree > to get both in my rc2 pull request. > > Reviewed-by: Eric Blake <eblake@redhat.com> Actually, since this is the second patch on the same topic, I'm wondering if it's better to use the following one-liner to fix BOTH issues and without relying on a gcc extension: diff --git i/block/nbd.c w/block/nbd.c index 8d565cc624ec..f751a8e633e5 100644 --- i/block/nbd.c +++ w/block/nbd.c @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( request_ret, qiov, payload, errp); if (ret < 0) { + memset(reply, 0, sizeof *reply); s->quit = true; } else { /* For assert at loop start in nbd_connection_entry */
On 19/07/2019 17:44, Eric Blake wrote: > On 7/19/19 9:34 AM, Eric Blake wrote: >> On 7/19/19 4:40 AM, Andrey Shinkevich wrote: >>> In case nbd_co_receive_one_chunk() fails in >>> nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in >>> the check nbd_reply_is_simple() without being initialized. The iotest >>> 083 does not pass under the Valgrind: $./check -nbd -valgrind 083. >>> The alternative solution is to swap the operands in the condition: >>> 'if (s->quit || nbd_reply_is_simple(reply))' >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> --- >>> block/nbd.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Huh. Very similar to >> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but >> affects a different function. I can queue this one through my NBD tree >> to get both in my rc2 pull request. >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Actually, since this is the second patch on the same topic, I'm > wondering if it's better to use the following one-liner to fix BOTH > issues and without relying on a gcc extension: > > diff --git i/block/nbd.c w/block/nbd.c > index 8d565cc624ec..f751a8e633e5 100644 > --- i/block/nbd.c > +++ w/block/nbd.c > @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( > request_ret, qiov, payload, > errp); > > if (ret < 0) { > + memset(reply, 0, sizeof *reply); The call to memset() consumes more CPU time. I don't know how frequent the "ret < 0" path is. The initialization ' = {0}' is cheaper. Is it safe to swap the operands in the condition in nbd_reply_chunk_iter_receive(): 'if (s->quit || nbd_reply_is_simple(reply))' ? Andrey > s->quit = true; > } else { > /* For assert at loop start in nbd_connection_entry */ >
On 7/19/19 10:00 AM, Andrey Shinkevich wrote: > > > On 19/07/2019 17:44, Eric Blake wrote: >> On 7/19/19 9:34 AM, Eric Blake wrote: >>> On 7/19/19 4:40 AM, Andrey Shinkevich wrote: >>>> In case nbd_co_receive_one_chunk() fails in >>>> nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in >>>> the check nbd_reply_is_simple() without being initialized. The iotest >>>> 083 does not pass under the Valgrind: $./check -nbd -valgrind 083. >>>> The alternative solution is to swap the operands in the condition: >>>> 'if (s->quit || nbd_reply_is_simple(reply))' >>>> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>> --- >>>> block/nbd.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> Huh. Very similar to >>> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but >>> affects a different function. I can queue this one through my NBD tree >>> to get both in my rc2 pull request. >>> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> Actually, since this is the second patch on the same topic, I'm >> wondering if it's better to use the following one-liner to fix BOTH >> issues and without relying on a gcc extension: >> >> diff --git i/block/nbd.c w/block/nbd.c >> index 8d565cc624ec..f751a8e633e5 100644 >> --- i/block/nbd.c >> +++ w/block/nbd.c >> @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( >> request_ret, qiov, payload, >> errp); >> >> if (ret < 0) { >> + memset(reply, 0, sizeof *reply); > > The call to memset() consumes more CPU time. I don't know how frequent > the "ret < 0" path is. The initialization ' = {0}' is cheaper. Wrong. The 'ret < 0' path only happens on error, while the '= {0}' path happens on ALL cases including success (if you'll look at the generated assembly code, gcc should emit the same assembly sequence for zero-initialization of the struct, whether that is a call to memset() or just inline assignments of zeros based on the small size of the struct, whether or not your code uses memset or '={}'). You don't need to optimize the error case, because on error, your NBD connection is dead, and you have worse problems. Pre-initialization that is going to be overwritten on success is worse (although probably immeasurably, because it is likely in the noise) than exactly one initialization on any control flow path. > Is it safe to swap the operands in the condition in > nbd_reply_chunk_iter_receive(): > 'if (s->quit || nbd_reply_is_simple(reply))' ? Yes, swapping the conditional appears to fix the only use of reply where it is used uninitialized, at least in the current state of the code (but it took me longer to audit that). So if we're going for a one-line fix for both observations of the problem, changing the conditional instead of a memset on error is also acceptable for now (and maybe makes the error case slightly faster, but that's not a big deal, because the error case already means the NBD connection has bigger problems) - but who knows what future uses of reply might creep in to an unsuspecting patch writer that doesn't see the (in)action at a distance? Which way is more maintainable, proving that the low-level code always initializes the variable (easy, since that can be checked at a single function), or proving that all high-level uses may pass in an uninitialized variable that is still left uninit on error but that they only use the variable on success (harder, since now you have to audit every single caller to see how they use reply on failure)?
On 19/07/2019 18:15, Eric Blake wrote: > On 7/19/19 10:00 AM, Andrey Shinkevich wrote: >> >> >> On 19/07/2019 17:44, Eric Blake wrote: >>> On 7/19/19 9:34 AM, Eric Blake wrote: >>>> On 7/19/19 4:40 AM, Andrey Shinkevich wrote: >>>>> In case nbd_co_receive_one_chunk() fails in >>>>> nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in >>>>> the check nbd_reply_is_simple() without being initialized. The iotest >>>>> 083 does not pass under the Valgrind: $./check -nbd -valgrind 083. >>>>> The alternative solution is to swap the operands in the condition: >>>>> 'if (s->quit || nbd_reply_is_simple(reply))' >>>>> >>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>>> --- >>>>> block/nbd.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> Huh. Very similar to >>>> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but >>>> affects a different function. I can queue this one through my NBD tree >>>> to get both in my rc2 pull request. >>>> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> >>> Actually, since this is the second patch on the same topic, I'm >>> wondering if it's better to use the following one-liner to fix BOTH >>> issues and without relying on a gcc extension: >>> >>> diff --git i/block/nbd.c w/block/nbd.c >>> index 8d565cc624ec..f751a8e633e5 100644 >>> --- i/block/nbd.c >>> +++ w/block/nbd.c >>> @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( >>> request_ret, qiov, payload, >>> errp); >>> >>> if (ret < 0) { >>> + memset(reply, 0, sizeof *reply); >> >> The call to memset() consumes more CPU time. I don't know how frequent >> the "ret < 0" path is. The initialization ' = {0}' is cheaper. > > Wrong. The 'ret < 0' path only happens on error, while the '= {0}' path > happens on ALL cases including success (if you'll look at the generated > assembly code, gcc should emit the same assembly sequence for > zero-initialization of the struct, whether that is a call to memset() or > just inline assignments of zeros based on the small size of the struct, > whether or not your code uses memset or '={}'). You don't need to > optimize the error case, because on error, your NBD connection is dead, > and you have worse problems. Pre-initialization that is going to be > overwritten on success is worse (although probably immeasurably, because > it is likely in the noise) than exactly one initialization on any > control flow path. > >> Is it safe to swap the operands in the condition in >> nbd_reply_chunk_iter_receive(): >> 'if (s->quit || nbd_reply_is_simple(reply))' ? > > Yes, swapping the conditional appears to fix the only use of reply where > it is used uninitialized, at least in the current state of the code (but > it took me longer to audit that). So if we're going for a one-line fix > for both observations of the problem, changing the conditional instead > of a memset on error is also acceptable for now (and maybe makes the > error case slightly faster, but that's not a big deal, because the error > case already means the NBD connection has bigger problems) - but who > knows what future uses of reply might creep in to an unsuspecting patch > writer that doesn't see the (in)action at a distance? Which way is more > maintainable, proving that the low-level code always initializes the > variable (easy, since that can be checked at a single function), or > proving that all high-level uses may pass in an uninitialized variable > that is still left uninit on error but that they only use the variable > on success (harder, since now you have to audit every single caller to > see how they use reply on failure)? > Sounds reasonable. Clear now. So, I will detach this patch from the series in the next v5 version. Andrey
diff --git a/block/nbd.c b/block/nbd.c index 81edabb..8480ad4 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -786,7 +786,7 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle, int *request_ret, Error **errp) { NBDReplyChunkIter iter; - NBDReply reply; + NBDReply reply = {}; void *payload = NULL; Error *local_err = NULL;
In case nbd_co_receive_one_chunk() fails in nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in the check nbd_reply_is_simple() without being initialized. The iotest 083 does not pass under the Valgrind: $./check -nbd -valgrind 083. The alternative solution is to swap the operands in the condition: 'if (s->quit || nbd_reply_is_simple(reply))' Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/nbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)