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 <[email protected]> Cc: Eric Blake <[email protected]> Cc: Vladimir Sementsov-Ogievskiy <[email protected]> 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
