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 <[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 ping
