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

Denis V. Lunev posted 3 patches 1 day, 8 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260820115228.587427-1-den@openvz.org
Maintainers: Eric Blake <eblake@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
block/nbd.c | 65 +++++++++++++++++++++++++++++++++++------------------
1 file changed, 43 insertions(+), 22 deletions(-)
[PATCH v2 0/3] block/nbd: fix a race in reply processing
Posted by Denis V. Lunev 1 day, 8 hours ago
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 checks the cookie, and for the cookie
of the reply in flight that check 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.

v1:
https://lore.kernel.org/qemu-devel/20260812102906.894063-1-den@openvz.org/

Changes in v2
-------------

 - patch 2: the helper takes an Error ** and checks every cookie, the
   one taken from the wire included, so it is no longer called with an
   exception; the callers which can only hand it a cookie of their own
   pass &error_abort. (Vladimir)

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 | 65 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 22 deletions(-)


base-commit: ae4f3443209ab154b48b706a146e5f557ab147cb
-- 
2.53.0