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

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