From 0e97bcfd7ab3c7b3d489de3cf4c7c4977b73cd23 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 9 Jun 2017 22:04:12 +0200 Subject: [PATCH 5/6] nbd: Fully initialize client in case of failed negotiation RH-Author: Eric Blake Message-id: <20170609220413.28793-2-eblake@redhat.com> Patchwork-id: 75576 O-Subject: [RHEL-7.4 qemu-kvm PATCH 1/2] nbd: Fully initialize client in case of failed negotiation Bugzilla: 1451614 RH-Acked-by: Laszlo Ersek RH-Acked-by: Paolo Bonzini RH-Acked-by: Thomas Huth 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=1451614 Signed-off-by: Miroslav Rezanina Conflicts: nbd/server.c - code lives in nbd.c instead, with older handlers --- nbd.c | 6 ++---- qemu-nbd.c | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/nbd.c b/nbd.c index 8a32e18..b5cdc1b 100644 --- a/nbd.c +++ b/nbd.c @@ -1280,18 +1280,16 @@ static coroutine_fn void nbd_co_client_start(void *opaque) if (exp) { nbd_export_get(exp); + QTAILQ_INSERT_TAIL(&exp->clients, client, next); } qemu_set_nonblock(client->sock); + qemu_co_mutex_init(&client->send_lock); if (nbd_negotiate(data)) { nbd_client_close(client); goto out; } - qemu_co_mutex_init(&client->send_lock); qemu_set_fd_handler2(client->sock, nbd_can_read, nbd_read, NULL, client); - if (exp) { - QTAILQ_INSERT_TAIL(&exp->clients, client, next); - } out: g_free(data); } diff --git a/qemu-nbd.c b/qemu-nbd.c index 047dd49..cde7431 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -267,7 +267,7 @@ out: static int nbd_can_accept(void *opaque) { - return nb_fds < shared; + return state == RUNNING && nb_fds < shared; } static void nbd_export_closed(NBDExport *exp) -- 1.8.3.1