[PATCH 0/3] block/nbd: fix a race in reply processing

[PATCH 0/3] block/nbd: fix a race in reply processing

[Denis V. Lunev]

s->reply is documented as protected by s->receive_mutex, but the cookie
which owns it is cleared without that mutex. A request waiting for its
own reply reads the very same field under the mutex, and reads it twice
in a row, so the owner can clear it in between. The second read returns
0, COOKIE_TO_INDEX() turns that into an index of -1, and s->requests[]
is accessed in front of the array:

  Assertion `!s->requests[ind2].receiving' failed.

  (gdb) p cookie
  $1 = 8
  (gdb) p s->reply.cookie
  $2 = 0
  (gdb) p &((NBDClientRequest *)s->requests)[-1].receiving
  $3 = (_Bool *) 0x5555558416c0
  (gdb) p &s->in_flight
  $4 = (unsigned int *) 0x5555558416c0

requests[-1].receiving lands on in_flight, which is non-zero while
requests are outstanding, so the read comes back true and the assertion
fires. Without the assertion it is a plain out of bounds read.

This was hit in the field, on a virtio-blk disk whose backing chain ends
in an NBD node, with the virtqueues of that disk spread over three
iothreads. Two coroutines of one NBD node then run in different threads,
which is what the race needs: there is no yield point between the two
reads for the owner to squeeze into, so a single AioContext cannot
produce it.

Patch 3 is the fix, patches 1 and 2 are what I ran into on the way to
it. The order is dictated by patch 2: it routes every cookie to index
conversion through a helper which asserts the range, and for the cookie
of the reply in flight that assertion only holds once patch 1 stops the
error paths from leaving a value chosen by the server behind.

Reproduced with a scratch harness which drives one NBD client node from
two AioContexts against a real qemu-nbd. At -O2 gcc merges all three
reads of s->reply.cookie in nbd_receive_replies() into a single load, so
the race is not observable at all in such a build; the gdb output above
comes from an -O1 build of this branch with the two scratch commits on
top. The report itself came from a build with coverage instrumentation,
which is the kind of build that keeps the reads apart.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Eric Blake <eblake@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Denis V. Lunev (3):
  block/nbd: clear reply.cookie when the reply is rejected
  block/nbd: never index requests[] with an unchecked cookie
  block/nbd: clear reply.cookie under receive_mutex

 block/nbd.c | 53 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 18 deletions(-)


base-commit: e1705a25aff35635c360bbaba4c2731d019a422a
-- 
2.53.0

[PATCH 1/3] block/nbd: clear reply.cookie when the reply is rejected

[Denis V. Lunev]

nbd_receive_replies() reads a reply header into s->reply and, when the
header turns out to be unusable, reports a channel error and returns
without touching it. The cookie stays there until the request which
owns the reply clears it, and until then the waiters are explicitly
allowed to look at a cookie which is not theirs:

    if (s->reply.cookie != 0) {
        ind2 = COOKIE_TO_INDEX(s->reply.cookie);
        assert(!s->requests[ind2].receiving);

Two of the error paths leave a value chosen by the server behind: one
returns before the cookie is validated at all, the other returns
because that validation has failed. A waiter which picks such a cookie
up turns it into an index which is not in requests[] and accesses the
array out of bounds, at an offset the server controls.

The reply is of no use to anybody at this point, so clear the cookie
before the mutex is released and keep the invariant that a non-zero
s->reply.cookie is always an index of a live request.

Observing the stale cookie takes a second thread, which a multiqueue
configuration provides. Within one AioContext there is no yield point
between the failed read and the clearing done by the owner in
nbd_co_receive_one_chunk(), so nothing else of this node runs in
between. The parked waiters cannot see it either, as they are woken
only after the cookie has been cleared. What can get in is a request
entering nbd_receive_replies() afresh, one just sent or one back for
its next reply chunk, because that path takes the mutex without
looking at the state.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Eric Blake <eblake@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/nbd.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 5d231d5c4e..d9b776283f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -466,20 +466,19 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
             error_setg(errp, "server dropped connection");
         }
         if (ret < 0) {
-            nbd_channel_error(s, ret);
-            return ret;
+            goto err;
         }
         if (nbd_reply_is_structured(&s->reply) &&
             s->info.mode < NBD_MODE_STRUCTURED) {
-            nbd_channel_error(s, -EINVAL);
+            ret = -EINVAL;
             error_setg(errp, "unexpected structured reply");
-            return -EINVAL;
+            goto err;
         }
         ind2 = COOKIE_TO_INDEX(s->reply.cookie);
         if (ind2 >= MAX_NBD_REQUESTS || !s->requests[ind2].coroutine) {
-            nbd_channel_error(s, -EINVAL);
+            ret = -EINVAL;
             error_setg(errp, "unexpected cookie value");
-            return -EINVAL;
+            goto err;
         }
         if (s->reply.cookie == cookie) {
             /* We are done */
@@ -487,6 +486,13 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
         }
         nbd_recv_coroutine_wake_one(&s->requests[ind2]);
     }
+
+err:
+    /* Waiters look at this cookie, so do not leave a rejected one behind. */
+    s->reply.cookie = 0;
+    nbd_channel_error(s, ret);
+
+    return ret;
 }
 
 static int coroutine_fn GRAPH_RDLOCK
-- 
2.53.0

[PATCH 2/3] block/nbd: never index requests[] with an unchecked cookie

[Denis V. Lunev]

Cookies are converted into indices of s->requests[] in several places
and the result is used right away, without any check:

    int i = COOKIE_TO_INDEX(cookie);
    ...
    return nbd_co_receive_offset_data_payload(s, s->requests[i].offset,

COOKIE_TO_INDEX() subtracts one, so a zero cookie becomes an index of
-1 and the access lands in front of the array. This is undefined and,
depending on the type of the index and on what the compiler has put
there, it can as well pass silently: at the site above i is signed, so
even a plain i < MAX_NBD_REQUESTS check would happily let -1 through.

The only cookie which is checked today is the one taken from the wire,
in nbd_receive_replies(), where an invalid value is a protocol error
rather than an internal inconsistency and thus has to stay a channel
error. Route every other conversion through a helper which asserts the
range before it returns the slot, so that the check can not be
forgotten again.

The cookie of the reply in flight goes through the helper as well. It
is not an unchecked value: it has either passed the check above, or it
has been cleared by the previous commit, so a stale index there is an
internal inconsistency too.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Eric Blake <eblake@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/nbd.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index d9b776283f..21f0f9d40d 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -136,6 +136,16 @@ static void nbd_clear_bdrvstate(BlockDriverState *bs)
     s->x_dirty_bitmap = NULL;
 }
 
+/* Not for cookies coming from the wire, those are checked separately. */
+static NBDClientRequest *nbd_request_by_cookie(BDRVNBDState *s, uint64_t cookie)
+{
+    uint64_t ind = COOKIE_TO_INDEX(cookie);
+
+    assert(ind < MAX_NBD_REQUESTS);
+
+    return &s->requests[ind];
+}
+
 /* Called with s->receive_mutex taken.  */
 static bool coroutine_fn nbd_recv_coroutine_wake_one(NBDClientRequest *req)
 {
@@ -422,7 +432,8 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
                                             Error **errp)
 {
     int ret;
-    uint64_t ind = COOKIE_TO_INDEX(cookie), ind2;
+    NBDClientRequest *req = nbd_request_by_cookie(s, cookie);
+    uint64_t ind2;
     QEMU_LOCK_GUARD(&s->receive_mutex);
 
     while (true) {
@@ -437,10 +448,9 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
              * woken by whoever set s->reply.cookie (or never wait in this
              * yield). So, we should not wake it here.
              */
-            ind2 = COOKIE_TO_INDEX(s->reply.cookie);
-            assert(!s->requests[ind2].receiving);
+            assert(!nbd_request_by_cookie(s, s->reply.cookie)->receiving);
 
-            s->requests[ind].receiving = true;
+            req->receiving = true;
             qemu_co_mutex_unlock(&s->receive_mutex);
 
             qemu_coroutine_yield();
@@ -454,7 +464,7 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
              */
 
             qemu_co_mutex_lock(&s->receive_mutex);
-            assert(!s->requests[ind].receiving);
+            assert(!req->receiving);
             continue;
         }
 
@@ -861,7 +871,6 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
 {
     ERRP_GUARD();
     int ret;
-    int i = COOKIE_TO_INDEX(cookie);
     void *local_payload = NULL;
     NBDStructuredReplyChunk *chunk;
 
@@ -919,8 +928,8 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
             return -EINVAL;
         }
 
-        return nbd_co_receive_offset_data_payload(s, s->requests[i].offset,
-                                                  qiov, errp);
+        return nbd_co_receive_offset_data_payload(
+                s, nbd_request_by_cookie(s, cookie)->offset, qiov, errp);
     }
 
     if (nbd_reply_type_is_error(chunk->type)) {
@@ -1067,7 +1076,7 @@ static bool coroutine_fn nbd_reply_chunk_iter_receive(BDRVNBDState *s,
 
 break_loop:
     qemu_mutex_lock(&s->requests_lock);
-    s->requests[COOKIE_TO_INDEX(cookie)].coroutine = NULL;
+    nbd_request_by_cookie(s, cookie)->coroutine = NULL;
     s->in_flight--;
     qemu_co_queue_next(&s->free_sema);
     qemu_mutex_unlock(&s->requests_lock);
-- 
2.53.0

[PATCH 3/3] block/nbd: clear reply.cookie under receive_mutex

[Denis V. Lunev]

s->reply is documented as protected by s->receive_mutex, but the cookie
is cleared without it once the owning request has consumed its chunk.
A waiter in nbd_receive_replies() inspects the very same field under
the mutex, and does so with two separate loads:

    if (s->reply.cookie != 0) {
        ind2 = COOKIE_TO_INDEX(s->reply.cookie);
        assert(!s->requests[ind2].receiving);

Nothing keeps those two loads consistent. If the owner clears the
cookie in between, the second one reads 0, COOKIE_TO_INDEX() turns it
into an index of -1, and s->requests[] is accessed out of bounds:

  Assertion `!s->requests[ind2].receiving' failed.

  (gdb) p cookie
  $1 = 8
  (gdb) p s->reply.cookie
  $2 = 0
  (gdb) p &((NBDClientRequest *)s->requests)[-1].receiving
  $3 = (_Bool *) 0x5555558416c0
  (gdb) p &s->in_flight
  $4 = (unsigned int *) 0x5555558416c0
  (gdb) p s->in_flight
  $5 = 8

The cookie we wait for is 8, yet reply.cookie reads 0 one line after it
was found non-zero, so the index is -1. requests[-1].receiving lands on
in_flight, which is non-zero while requests are outstanding, and that is
what the assertion trips over.

Hitting this requires two coroutines of one NBD node to run in
different threads, as there is no yield point between the two loads
for the owner to squeeze into. A multiqueue configuration provides
exactly that, with the virtqueues of one disk spread over several
iothreads. Note that a compiler is free to merge the two loads into
one, in which case the race is invisible, so builds with reduced
optimization are much more likely to trip over it.

Accessing s->reply without the mutex is fine for the coroutine that
owns the reply: a non-zero cookie makes the field private to it.
Releasing that ownership is not, as it races with the waiters which
are explicitly allowed to look at the cookie. Clear it under the
mutex, in the same critical section as the wakeup, and make
nbd_recv_coroutines_wake() caller-locked, as CoMutex is not
recursive. It has a single caller.

The added acquisition cannot block behind the header read in
nbd_receive_replies(), because that path is only reachable with
reply.cookie == 0 while we still own a non-zero cookie. Merging the
clear with the wakeup also keeps a newcomer from starting a header
read in between, which would stall this already completed request for
the duration of that read.

There is no cookie to own when we get here after an error, and then a
newcomer can indeed be inside that read. It does not hold us for long
either, as the channel has been shut down before the error was
reported, so the read it sits in returns right away.

Fixes: 4ddb5d2fde ("block/nbd: drop connection_co")
Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Eric Blake <eblake@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/nbd.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 21f0f9d40d..1659008784 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -158,11 +158,11 @@ static bool coroutine_fn nbd_recv_coroutine_wake_one(NBDClientRequest *req)
     return false;
 }
 
+/* Called with s->receive_mutex taken. */
 static void coroutine_fn nbd_recv_coroutines_wake(BDRVNBDState *s)
 {
     int i;
 
-    QEMU_LOCK_GUARD(&s->receive_mutex);
     for (i = 0; i < MAX_NBD_REQUESTS; i++) {
         if (nbd_recv_coroutine_wake_one(&s->requests[i])) {
             return;
@@ -970,9 +970,11 @@ static coroutine_fn int nbd_co_receive_one_chunk(
         /* For assert at loop start in nbd_connection_entry */
         *reply = s->reply;
     }
-    s->reply.cookie = 0;
 
-    nbd_recv_coroutines_wake(s);
+    WITH_QEMU_LOCK_GUARD(&s->receive_mutex) {
+        s->reply.cookie = 0;
+        nbd_recv_coroutines_wake(s);
+    }
 
     return ret;
 }
-- 
2.53.0

Re: [PATCH 0/3] block/nbd: fix a race in reply processing

[Denis V. Lunev]

On 8/12/26 12:29, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain it was sent from.
> Do not click links or open attachments unless it is an email you expected to receive.
> s->reply is documented as protected by s->receive_mutex, but the cookie
> which owns it is cleared without that mutex. A request waiting for its
> own reply reads the very same field under the mutex, and reads it twice
> in a row, so the owner can clear it in between. The second read returns
> 0, COOKIE_TO_INDEX() turns that into an index of -1, and s->requests[]
> is accessed in front of the array:
>
>   Assertion `!s->requests[ind2].receiving' failed.
>
>   (gdb) p cookie
>   $1 = 8
>   (gdb) p s->reply.cookie
>   $2 = 0
>   (gdb) p &((NBDClientRequest *)s->requests)[-1].receiving
>   $3 = (_Bool *) 0x5555558416c0
>   (gdb) p &s->in_flight
>   $4 = (unsigned int *) 0x5555558416c0
>
> requests[-1].receiving lands on in_flight, which is non-zero while
> requests are outstanding, so the read comes back true and the assertion
> fires. Without the assertion it is a plain out of bounds read.
>
> This was hit in the field, on a virtio-blk disk whose backing chain ends
> in an NBD node, with the virtqueues of that disk spread over three
> iothreads. Two coroutines of one NBD node then run in different threads,
> which is what the race needs: there is no yield point between the two
> reads for the owner to squeeze into, so a single AioContext cannot
> produce it.
>
> Patch 3 is the fix, patches 1 and 2 are what I ran into on the way to
> it. The order is dictated by patch 2: it routes every cookie to index
> conversion through a helper which asserts the range, and for the cookie
> of the reply in flight that assertion only holds once patch 1 stops the
> error paths from leaving a value chosen by the server behind.
>
> Reproduced with a scratch harness which drives one NBD client node from
> two AioContexts against a real qemu-nbd. At -O2 gcc merges all three
> reads of s->reply.cookie in nbd_receive_replies() into a single load, so
> the race is not observable at all in such a build; the gdb output above
> comes from an -O1 build of this branch with the two scratch commits on
> top. The report itself came from a build with coverage instrumentation,
> which is the kind of build that keeps the reads apart.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Denis V. Lunev (3):
>   block/nbd: clear reply.cookie when the reply is rejected
>   block/nbd: never index requests[] with an unchecked cookie
>   block/nbd: clear reply.cookie under receive_mutex
>
>  block/nbd.c | 53 +++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 18 deletions(-)
>
>
> base-commit: e1705a25aff35635c360bbaba4c2731d019a422a
ping

Re: [PATCH 1/3] block/nbd: clear reply.cookie when the reply is rejected

[Vladimir Sementsov-Ogievskiy]

On 12.08.26 13:29, Denis V. Lunev wrote:
> nbd_receive_replies() reads a reply header into s->reply and, when the
> header turns out to be unusable, reports a channel error and returns
> without touching it. The cookie stays there until the request which
> owns the reply clears it, and until then the waiters are explicitly
> allowed to look at a cookie which is not theirs:
> 
>      if (s->reply.cookie != 0) {
>          ind2 = COOKIE_TO_INDEX(s->reply.cookie);
>          assert(!s->requests[ind2].receiving);
> 
> Two of the error paths leave a value chosen by the server behind: one
> returns before the cookie is validated at all, the other returns
> because that validation has failed. A waiter which picks such a cookie
> up turns it into an index which is not in requests[] and accesses the
> array out of bounds, at an offset the server controls.
> 
> The reply is of no use to anybody at this point, so clear the cookie
> before the mutex is released and keep the invariant that a non-zero
> s->reply.cookie is always an index of a live request.
> 
> Observing the stale cookie takes a second thread, which a multiqueue
> configuration provides. Within one AioContext there is no yield point
> between the failed read and the clearing done by the owner in
> nbd_co_receive_one_chunk(), so nothing else of this node runs in
> between. The parked waiters cannot see it either, as they are woken
> only after the cookie has been cleared. What can get in is a request
> entering nbd_receive_replies() afresh, one just sent or one back for
> its next reply chunk, because that path takes the mutex without
> looking at the state.
> 
> Signed-off-by: Denis V. Lunev<den@openvz.org>
> Cc: Eric Blake<eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH 2/3] block/nbd: never index requests[] with an unchecked cookie

[Vladimir Sementsov-Ogievskiy]

On 12.08.26 13:29, Denis V. Lunev wrote:
> Cookies are converted into indices of s->requests[] in several places
> and the result is used right away, without any check:
> 
>      int i = COOKIE_TO_INDEX(cookie);
>      ...
>      return nbd_co_receive_offset_data_payload(s, s->requests[i].offset,
> 
> COOKIE_TO_INDEX() subtracts one, so a zero cookie becomes an index of
> -1 and the access lands in front of the array. This is undefined and,
> depending on the type of the index and on what the compiler has put
> there, it can as well pass silently: at the site above i is signed, so
> even a plain i < MAX_NBD_REQUESTS check would happily let -1 through.
> 
> The only cookie which is checked today is the one taken from the wire,
> in nbd_receive_replies(), where an invalid value is a protocol error
> rather than an internal inconsistency and thus has to stay a channel
> error. Route every other conversion through a helper which asserts the
> range before it returns the slot, so that the check can not be
> forgotten again.
> 
> The cookie of the reply in flight goes through the helper as well. It
> is not an unchecked value: it has either passed the check above, or it
> has been cleared by the previous commit, so a stale index there is an
> internal inconsistency too.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   block/nbd.c | 27 ++++++++++++++++++---------
>   1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index d9b776283f..21f0f9d40d 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -136,6 +136,16 @@ static void nbd_clear_bdrvstate(BlockDriverState *bs)
>       s->x_dirty_bitmap = NULL;
>   }
>   
> +/* Not for cookies coming from the wire, those are checked separately. */

Still, I think, if we do such a wrapper, better to call it always with no
exclusions.

For example, add errp, and pass &error_abort when don't expect an error.

> +static NBDClientRequest *nbd_request_by_cookie(BDRVNBDState *s, uint64_t cookie)
> +{
> +    uint64_t ind = COOKIE_TO_INDEX(cookie);
> +
> +    assert(ind < MAX_NBD_REQUESTS);
> +
> +    return &s->requests[ind];
> +}
> +
>   /* Called with s->receive_mutex taken.  */
>   static bool coroutine_fn nbd_recv_coroutine_wake_one(NBDClientRequest *req)
>   {
> @@ -422,7 +432,8 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
>                                               Error **errp)
>   {
>       int ret;
> -    uint64_t ind = COOKIE_TO_INDEX(cookie), ind2;
> +    NBDClientRequest *req = nbd_request_by_cookie(s, cookie);
> +    uint64_t ind2;
>       QEMU_LOCK_GUARD(&s->receive_mutex);
>   
>       while (true) {
> @@ -437,10 +448,9 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
>                * woken by whoever set s->reply.cookie (or never wait in this
>                * yield). So, we should not wake it here.
>                */
> -            ind2 = COOKIE_TO_INDEX(s->reply.cookie);
> -            assert(!s->requests[ind2].receiving);
> +            assert(!nbd_request_by_cookie(s, s->reply.cookie)->receiving);
>   
> -            s->requests[ind].receiving = true;
> +            req->receiving = true;
>               qemu_co_mutex_unlock(&s->receive_mutex);
>   
>               qemu_coroutine_yield();
> @@ -454,7 +464,7 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
>                */
>   
>               qemu_co_mutex_lock(&s->receive_mutex);
> -            assert(!s->requests[ind].receiving);
> +            assert(!req->receiving);
>               continue;
>           }
>   
> @@ -861,7 +871,6 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
>   {
>       ERRP_GUARD();
>       int ret;
> -    int i = COOKIE_TO_INDEX(cookie);
>       void *local_payload = NULL;
>       NBDStructuredReplyChunk *chunk;
>   
> @@ -919,8 +928,8 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
>               return -EINVAL;
>           }
>   
> -        return nbd_co_receive_offset_data_payload(s, s->requests[i].offset,
> -                                                  qiov, errp);
> +        return nbd_co_receive_offset_data_payload(
> +                s, nbd_request_by_cookie(s, cookie)->offset, qiov, errp);
>       }
>   
>       if (nbd_reply_type_is_error(chunk->type)) {
> @@ -1067,7 +1076,7 @@ static bool coroutine_fn nbd_reply_chunk_iter_receive(BDRVNBDState *s,
>   
>   break_loop:
>       qemu_mutex_lock(&s->requests_lock);
> -    s->requests[COOKIE_TO_INDEX(cookie)].coroutine = NULL;
> +    nbd_request_by_cookie(s, cookie)->coroutine = NULL;
>       s->in_flight--;
>       qemu_co_queue_next(&s->free_sema);
>       qemu_mutex_unlock(&s->requests_lock);


-- 
Best regards,
Vladimir

Re: [PATCH 3/3] block/nbd: clear reply.cookie under receive_mutex

[Vladimir Sementsov-Ogievskiy]

On 12.08.26 13:29, Denis V. Lunev wrote:
> s->reply is documented as protected by s->receive_mutex, but the cookie
> is cleared without it once the owning request has consumed its chunk.
> A waiter in nbd_receive_replies() inspects the very same field under
> the mutex, and does so with two separate loads:
> 
>      if (s->reply.cookie != 0) {
>          ind2 = COOKIE_TO_INDEX(s->reply.cookie);
>          assert(!s->requests[ind2].receiving);
> 
> Nothing keeps those two loads consistent. If the owner clears the
> cookie in between, the second one reads 0, COOKIE_TO_INDEX() turns it
> into an index of -1, and s->requests[] is accessed out of bounds:
> 
>    Assertion `!s->requests[ind2].receiving' failed.
> 
>    (gdb) p cookie
>    $1 = 8
>    (gdb) p s->reply.cookie
>    $2 = 0
>    (gdb) p &((NBDClientRequest *)s->requests)[-1].receiving
>    $3 = (_Bool *) 0x5555558416c0
>    (gdb) p &s->in_flight
>    $4 = (unsigned int *) 0x5555558416c0
>    (gdb) p s->in_flight
>    $5 = 8
> 
> The cookie we wait for is 8, yet reply.cookie reads 0 one line after it
> was found non-zero, so the index is -1. requests[-1].receiving lands on
> in_flight, which is non-zero while requests are outstanding, and that is
> what the assertion trips over.
> 
> Hitting this requires two coroutines of one NBD node to run in
> different threads, as there is no yield point between the two loads
> for the owner to squeeze into. A multiqueue configuration provides
> exactly that, with the virtqueues of one disk spread over several
> iothreads. Note that a compiler is free to merge the two loads into
> one, in which case the race is invisible, so builds with reduced
> optimization are much more likely to trip over it.
> 
> Accessing s->reply without the mutex is fine for the coroutine that
> owns the reply: a non-zero cookie makes the field private to it.
> Releasing that ownership is not, as it races with the waiters which
> are explicitly allowed to look at the cookie. Clear it under the
> mutex, in the same critical section as the wakeup, and make
> nbd_recv_coroutines_wake() caller-locked, as CoMutex is not
> recursive. It has a single caller.
> 
> The added acquisition cannot block behind the header read in
> nbd_receive_replies(), because that path is only reachable with
> reply.cookie == 0 while we still own a non-zero cookie. Merging the
> clear with the wakeup also keeps a newcomer from starting a header
> read in between, which would stall this already completed request for
> the duration of that read.
> 
> There is no cookie to own when we get here after an error, and then a
> newcomer can indeed be inside that read. It does not hold us for long
> either, as the channel has been shut down before the error was
> reported, so the read it sits in returns right away.
> 
> Fixes: 4ddb5d2fde ("block/nbd: drop connection_co")
> Signed-off-by: Denis V. Lunev<den@openvz.org>
> Cc: Eric Blake<eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir