From 7c2ce59d5622187490728e29a91710a470d1c5b4 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 9 Jun 2017 21:56:07 +0200 Subject: [PATCH 02/13] nbd: Fully initialize client in case of failed negotiation RH-Author: Eric Blake Message-id: <20170609215608.27053-2-eblake@redhat.com> Patchwork-id: 75571 O-Subject: [RHEV-7.4 qemu-kvm-rhev PATCH 1/2] nbd: Fully initialize client in case of failed negotiation Bugzilla: 1447948 RH-Acked-by: Paolo Bonzini RH-Acked-by: Thomas Huth RH-Acked-by: Laszlo Ersek If a non-NBD client connects to qemu-nbd, we would end up with a SIGSEGV in nbd_client_put() because we were trying to unregister the client's association to the export, even though we skipped inserting the client into that list. Easy trigger in two terminals: $ qemu-nbd -p 30001 --format=raw file $ nmap 127.0.0.1 -p 30001 nmap claims that it thinks it connected to a pago-services1 server (which probably means nmap could be updated to learn the NBD protocol and give a more accurate diagnosis of the open port - but that's not our problem), then terminates immediately, so our call to nbd_negotiate() fails. The fix is to reorder nbd_co_client_start() to ensure that all initialization occurs before we ever try talking to a client in nbd_negotiate(), so that the teardown sequence on negotiation failure doesn't fault while dereferencing a half-initialized object. While debugging this, I also noticed that nbd_update_server_watch() called by nbd_client_closed() was still adding a channel to accept the next client, even when the state was no longer RUNNING. That is fixed by making nbd_can_accept() pay attention to the current state. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1451614 Signed-off-by: Eric Blake Message-Id: <20170527030421.28366-1-eblake@redhat.com> Signed-off-by: Paolo Bonzini (cherry picked from commit ???) https://bugzilla.redhat.com/show_bug.cgi?id=1447948 Signed-off-by: Miroslav Rezanina --- nbd/server.c | 8 +++----- qemu-nbd.c | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 924a1fe..edfda84 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1376,16 +1376,14 @@ static coroutine_fn void nbd_co_client_start(void *opaque) if (exp) { nbd_export_get(exp); + QTAILQ_INSERT_TAIL(&exp->clients, client, next); } + qemu_co_mutex_init(&client->send_lock); + if (nbd_negotiate(data)) { client_close(client); goto out; } - qemu_co_mutex_init(&client->send_lock); - - if (exp) { - QTAILQ_INSERT_TAIL(&exp->clients, client, next); - } nbd_client_receive_next_request(client); diff --git a/qemu-nbd.c b/qemu-nbd.c index e080fb7..b44764e 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -324,7 +324,7 @@ out: static int nbd_can_accept(void) { - return nb_fds < shared; + return state == RUNNING && nb_fds < shared; } static void nbd_export_closed(NBDExport *exp) -- 1.8.3.1