]>
Commit | Line | Data |
---|---|---|
b45e13fe AD |
1 | From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 |
2 | From: Eric Blake <eblake@redhat.com> | |
3 | Date: Wed, 27 Sep 2017 17:57:19 +0200 | |
4 | Subject: [PATCH] nbd-client: Fix regression when server sends garbage | |
5 | ||
6 | RH-Author: Eric Blake <eblake@redhat.com> | |
7 | Message-id: <20170927175725.20023-2-eblake@redhat.com> | |
8 | Patchwork-id: 76672 | |
9 | O-Subject: [RHEV-7.4.z qemu-kvm-rhev PATCH 1/7] nbd-client: Fix regression when server sends garbage | |
10 | Bugzilla: 1495474 | |
11 | RH-Acked-by: Max Reitz <mreitz@redhat.com> | |
12 | RH-Acked-by: Jeffrey Cody <jcody@redhat.com> | |
13 | RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com> | |
14 | ||
15 | When we switched NBD to use coroutines for qemu 2.9 (in particular, | |
16 | commit a12a712a), we introduced a regression: if a server sends us | |
17 | garbage (such as a corrupted magic number), we quit the read loop | |
18 | but do not stop sending further queued commands, resulting in the | |
19 | client hanging when it never reads the response to those additional | |
20 | commands. In qemu 2.8, we properly detected that the server is no | |
21 | longer reliable, and cancelled all existing pending commands with | |
22 | EIO, then tore down the socket so that all further command attempts | |
23 | get EPIPE. | |
24 | ||
25 | Restore the proper behavior of quitting (almost) all communication | |
26 | with a broken server: Once we know we are out of sync or otherwise | |
27 | can't trust the server, we must assume that any further incoming | |
28 | data is unreliable and therefore end all pending commands with EIO, | |
29 | and quit trying to send any further commands. As an exception, we | |
30 | still (try to) send NBD_CMD_DISC to let the server know we are going | |
31 | away (in part, because it is easier to do that than to further | |
32 | refactor nbd_teardown_connection, and in part because it is the | |
33 | only command where we do not have to wait for a reply). | |
34 | ||
35 | Based on a patch by Vladimir Sementsov-Ogievskiy. | |
36 | ||
37 | A malicious server can be created with the following hack, | |
38 | followed by setting NBD_SERVER_DEBUG to a non-zero value in the | |
39 | environment when running qemu-nbd: | |
40 | ||
41 | | --- a/nbd/server.c | |
42 | | +++ b/nbd/server.c | |
43 | | @@ -919,6 +919,17 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply, Error **errp) | |
44 | | stl_be_p(buf + 4, reply->error); | |
45 | | stq_be_p(buf + 8, reply->handle); | |
46 | | | |
47 | | + static int debug; | |
48 | | + static int count; | |
49 | | + if (!count++) { | |
50 | | + const char *str = getenv("NBD_SERVER_DEBUG"); | |
51 | | + if (str) { | |
52 | | + debug = atoi(str); | |
53 | | + } | |
54 | | + } | |
55 | | + if (debug && !(count % debug)) { | |
56 | | + buf[0] = 0; | |
57 | | + } | |
58 | | return nbd_write(ioc, buf, sizeof(buf), errp); | |
59 | | } | |
60 | ||
61 | Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | |
62 | Signed-off-by: Eric Blake <eblake@redhat.com> | |
63 | Message-Id: <20170814213426.24681-1-eblake@redhat.com> | |
64 | Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> | |
65 | (cherry picked from commit 72b6ffc76653214b69a94a7b1643ff80df134486) | |
66 | Signed-off-by: Eric Blake <eblake@redhat.com> | |
67 | Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> | |
68 | ||
69 | Conflicts: | |
70 | block/nbd-client.c | |
71 | --- | |
72 | block/nbd-client.c | 17 +++++++++++++---- | |
73 | block/nbd-client.h | 1 + | |
74 | 2 files changed, 14 insertions(+), 4 deletions(-) | |
75 | ||
76 | diff --git a/block/nbd-client.c b/block/nbd-client.c | |
507c6de3 | 77 | index 282679b4f8..701b4ce2eb 100644 |
b45e13fe AD |
78 | --- a/block/nbd-client.c |
79 | +++ b/block/nbd-client.c | |
80 | @@ -71,7 +71,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) | |
81 | uint64_t i; | |
82 | int ret; | |
83 | ||
84 | - for (;;) { | |
85 | + while (!s->quit) { | |
86 | assert(s->reply.handle == 0); | |
87 | ret = nbd_receive_reply(s->ioc, &s->reply); | |
88 | if (ret <= 0) { | |
89 | @@ -102,6 +102,9 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) | |
90 | qemu_coroutine_yield(); | |
91 | } | |
92 | ||
93 | + if (ret < 0) { | |
94 | + s->quit = true; | |
95 | + } | |
96 | nbd_recv_coroutines_enter_all(s); | |
97 | s->read_reply_co = NULL; | |
98 | } | |
99 | @@ -130,6 +133,10 @@ static int nbd_co_send_request(BlockDriverState *bs, | |
100 | assert(i < MAX_NBD_REQUESTS); | |
101 | request->handle = INDEX_TO_HANDLE(s, i); | |
102 | ||
103 | + if (s->quit) { | |
104 | + qemu_co_mutex_unlock(&s->send_mutex); | |
105 | + return -EIO; | |
106 | + } | |
107 | if (!s->ioc) { | |
108 | qemu_co_mutex_unlock(&s->send_mutex); | |
109 | return -EPIPE; | |
110 | @@ -138,7 +145,7 @@ static int nbd_co_send_request(BlockDriverState *bs, | |
111 | if (qiov) { | |
112 | qio_channel_set_cork(s->ioc, true); | |
113 | rc = nbd_send_request(s->ioc, request); | |
114 | - if (rc >= 0) { | |
115 | + if (rc >= 0 && !s->quit) { | |
116 | ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov, request->len, | |
117 | false); | |
118 | if (ret != request->len) { | |
119 | @@ -149,6 +156,9 @@ static int nbd_co_send_request(BlockDriverState *bs, | |
120 | } else { | |
121 | rc = nbd_send_request(s->ioc, request); | |
122 | } | |
123 | + if (rc < 0) { | |
124 | + s->quit = true; | |
125 | + } | |
126 | qemu_co_mutex_unlock(&s->send_mutex); | |
127 | return rc; | |
128 | } | |
129 | @@ -163,8 +173,7 @@ static void nbd_co_receive_reply(NBDClientSession *s, | |
130 | /* Wait until we're woken up by nbd_read_reply_entry. */ | |
131 | qemu_coroutine_yield(); | |
132 | *reply = s->reply; | |
133 | - if (reply->handle != request->handle || | |
134 | - !s->ioc) { | |
135 | + if (reply->handle != request->handle || !s->ioc || s->quit) { | |
136 | reply->error = EIO; | |
137 | } else { | |
138 | if (qiov && reply->error == 0) { | |
139 | diff --git a/block/nbd-client.h b/block/nbd-client.h | |
507c6de3 | 140 | index 891ba44a20..9774a8ebbb 100644 |
b45e13fe AD |
141 | --- a/block/nbd-client.h |
142 | +++ b/block/nbd-client.h | |
143 | @@ -30,6 +30,7 @@ typedef struct NBDClientSession { | |
144 | ||
145 | Coroutine *recv_coroutine[MAX_NBD_REQUESTS]; | |
146 | NBDReply reply; | |
147 | + bool quit; | |
148 | } NBDClientSession; | |
149 | ||
150 | NBDClientSession *nbd_get_client_session(BlockDriverState *bs); | |
151 | -- | |
507c6de3 | 152 | 2.11.0 |
b45e13fe | 153 |