Commit | Line | Data |
---|---|---|
26eee146 SR |
1 | From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 |
2 | From: Stefan Reiter <s.reiter@proxmox.com> | |
3 | Date: Mon, 23 Aug 2021 11:28:32 +0200 | |
4 | Subject: [PATCH] monitor/qmp: fix race with clients disconnecting early | |
5 | ||
6 | The following sequence can produce a race condition that results in | |
7 | responses meant for different clients being sent to the wrong one: | |
8 | ||
9 | (QMP, no OOB) | |
10 | 1) client A connects | |
11 | 2) client A sends 'qmp_capabilities' | |
12 | 3) 'qmp_dispatch' runs in coroutine, schedules out to | |
13 | 'do_qmp_dispatch_bh' and yields | |
14 | 4) client A disconnects (i.e. aborts, crashes, etc...) | |
15 | 5) client B connects | |
16 | 6) 'do_qmp_dispatch_bh' runs 'qmp_capabilities' and wakes calling coroutine | |
17 | 7) capabilities are now set and 'mon->commands' is set to '&qmp_commands' | |
18 | 8) 'qmp_dispatch' returns to 'monitor_qmp_dispatch' | |
19 | 9) success message is sent to client B *without it ever having sent | |
20 | 'qmp_capabilities' itself* | |
21 | 9a) even if client B ignores it, it will now presumably send it's own | |
22 | greeting, which will error because caps are already set | |
23 | ||
24 | The fix proposed here uses an atomic, sequential connection number | |
25 | stored in the MonitorQMP struct, which is incremented everytime a new | |
26 | client connects. Since it is not changed on CHR_EVENT_CLOSED, the | |
27 | behaviour of allowing a client to disconnect only one side of the | |
28 | connection is retained. | |
29 | ||
30 | The connection_nr needs to be exposed outside of the monitor subsystem, | |
31 | since qmp_dispatch lives in qapi code. It needs to be checked twice, | |
32 | once for actually running the command in the BH (fixes 7), and once for | |
33 | sending back a response (fixes 9). | |
34 | ||
35 | This satisfies my local reproducer - using multiple clients constantly | |
36 | looping to open a connection, send the greeting, then exiting no longer | |
37 | crashes other, normally behaving clients with unrelated responses. | |
38 | ||
39 | Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> | |
ddbf7a87 | 40 | Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> |
26eee146 SR |
41 | --- |
42 | include/monitor/monitor.h | 1 + | |
43 | monitor/monitor-internal.h | 7 +++++++ | |
44 | monitor/monitor.c | 15 +++++++++++++++ | |
45 | monitor/qmp.c | 15 ++++++++++++++- | |
46 | qapi/qmp-dispatch.c | 21 +++++++++++++++++---- | |
47 | stubs/monitor-core.c | 5 +++++ | |
48 | 6 files changed, 59 insertions(+), 5 deletions(-) | |
49 | ||
50 | diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h | |
5b15e2ec | 51 | index a4b40e8391..d64ae8f34e 100644 |
26eee146 SR |
52 | --- a/include/monitor/monitor.h |
53 | +++ b/include/monitor/monitor.h | |
54 | @@ -16,6 +16,7 @@ extern QemuOptsList qemu_mon_opts; | |
55 | Monitor *monitor_cur(void); | |
56 | Monitor *monitor_set_cur(Coroutine *co, Monitor *mon); | |
57 | bool monitor_cur_is_qmp(void); | |
58 | +int monitor_get_connection_nr(const Monitor *mon); | |
59 | ||
60 | void monitor_init_globals(void); | |
61 | void monitor_init_globals_core(void); | |
62 | diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h | |
dc9827a6 | 63 | index caa2e90ef2..e1596f79ab 100644 |
26eee146 SR |
64 | --- a/monitor/monitor-internal.h |
65 | +++ b/monitor/monitor-internal.h | |
dc9827a6 | 66 | @@ -152,6 +152,13 @@ typedef struct { |
26eee146 SR |
67 | QemuMutex qmp_queue_lock; |
68 | /* Input queue that holds all the parsed QMP requests */ | |
69 | GQueue *qmp_requests; | |
70 | + | |
71 | + /* | |
72 | + * A sequential number that gets incremented on every new CHR_EVENT_OPENED. | |
73 | + * Used to avoid leftover responses in BHs from being sent to the wrong | |
74 | + * client. Access with atomics. | |
75 | + */ | |
76 | + int connection_nr; | |
77 | } MonitorQMP; | |
78 | ||
79 | /** | |
80 | diff --git a/monitor/monitor.c b/monitor/monitor.c | |
5b15e2ec | 81 | index 86949024f6..c306cadcf4 100644 |
26eee146 SR |
82 | --- a/monitor/monitor.c |
83 | +++ b/monitor/monitor.c | |
f376b2b9 | 84 | @@ -135,6 +135,21 @@ bool monitor_cur_is_qmp(void) |
26eee146 SR |
85 | return cur_mon && monitor_is_qmp(cur_mon); |
86 | } | |
87 | ||
88 | +/** | |
89 | + * If @mon is a QMP monitor, return the connection_nr, otherwise -1. | |
90 | + */ | |
91 | +int monitor_get_connection_nr(const Monitor *mon) | |
92 | +{ | |
93 | + MonitorQMP *qmp_mon; | |
94 | + | |
95 | + if (!monitor_is_qmp(mon)) { | |
96 | + return -1; | |
97 | + } | |
98 | + | |
99 | + qmp_mon = container_of(mon, MonitorQMP, common); | |
100 | + return qatomic_read(&qmp_mon->connection_nr); | |
101 | +} | |
102 | + | |
103 | /** | |
104 | * Is @mon is using readline? | |
105 | * Note: not all HMP monitors use readline, e.g., gdbserver has a | |
106 | diff --git a/monitor/qmp.c b/monitor/qmp.c | |
f376b2b9 | 107 | index 092c527b6f..6b8cfcf6d8 100644 |
26eee146 SR |
108 | --- a/monitor/qmp.c |
109 | +++ b/monitor/qmp.c | |
110 | @@ -141,6 +141,8 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req) | |
111 | QDict *rsp; | |
112 | QDict *error; | |
113 | ||
114 | + int conn_nr_before = qatomic_read(&mon->connection_nr); | |
115 | + | |
116 | rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), | |
117 | &mon->common); | |
118 | ||
119 | @@ -156,7 +158,17 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req) | |
120 | } | |
121 | } | |
122 | ||
123 | - monitor_qmp_respond(mon, rsp); | |
124 | + /* | |
125 | + * qmp_dispatch might have yielded and waited for a BH, in which case there | |
126 | + * is a chance a new client connected in the meantime - if this happened, | |
127 | + * the command will not have been executed, but we also need to ensure that | |
128 | + * we don't send back a corresponding response on a line that no longer | |
129 | + * belongs to this request. | |
130 | + */ | |
131 | + if (conn_nr_before == qatomic_read(&mon->connection_nr)) { | |
132 | + monitor_qmp_respond(mon, rsp); | |
133 | + } | |
134 | + | |
135 | qobject_unref(rsp); | |
136 | } | |
137 | ||
138 | @@ -444,6 +456,7 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event) | |
139 | ||
140 | switch (event) { | |
141 | case CHR_EVENT_OPENED: | |
142 | + qatomic_inc_fetch(&mon->connection_nr); | |
143 | mon->commands = &qmp_cap_negotiation_commands; | |
144 | monitor_qmp_caps_reset(mon); | |
145 | data = qmp_greeting(mon); | |
146 | diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c | |
dc9827a6 | 147 | index 0990873ec8..e605003771 100644 |
26eee146 SR |
148 | --- a/qapi/qmp-dispatch.c |
149 | +++ b/qapi/qmp-dispatch.c | |
dc9827a6 | 150 | @@ -117,16 +117,28 @@ typedef struct QmpDispatchBH { |
26eee146 SR |
151 | QObject **ret; |
152 | Error **errp; | |
153 | Coroutine *co; | |
154 | + int conn_nr; | |
155 | } QmpDispatchBH; | |
156 | ||
157 | static void do_qmp_dispatch_bh(void *opaque) | |
158 | { | |
159 | QmpDispatchBH *data = opaque; | |
160 | ||
161 | - assert(monitor_cur() == NULL); | |
162 | - monitor_set_cur(qemu_coroutine_self(), data->cur_mon); | |
163 | - data->cmd->fn(data->args, data->ret, data->errp); | |
164 | - monitor_set_cur(qemu_coroutine_self(), NULL); | |
165 | + /* | |
166 | + * A QMP monitor tracks it's client with a connection number, if this | |
167 | + * changes during the scheduling delay of this BH, we must not execute the | |
168 | + * command. Otherwise a badly placed 'qmp_capabilities' might affect the | |
169 | + * connection state of a client it was never meant for. | |
170 | + */ | |
171 | + if (data->conn_nr == monitor_get_connection_nr(data->cur_mon)) { | |
172 | + assert(monitor_cur() == NULL); | |
173 | + monitor_set_cur(qemu_coroutine_self(), data->cur_mon); | |
174 | + data->cmd->fn(data->args, data->ret, data->errp); | |
175 | + monitor_set_cur(qemu_coroutine_self(), NULL); | |
176 | + } else { | |
177 | + error_setg(data->errp, "active monitor connection changed"); | |
178 | + } | |
179 | + | |
180 | aio_co_wake(data->co); | |
181 | } | |
182 | ||
dc9827a6 | 183 | @@ -231,6 +243,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, |
26eee146 SR |
184 | .ret = &ret, |
185 | .errp = &err, | |
186 | .co = qemu_coroutine_self(), | |
187 | + .conn_nr = monitor_get_connection_nr(cur_mon), | |
188 | }; | |
189 | aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh, | |
190 | &data); | |
191 | diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c | |
5b15e2ec | 192 | index afa477aae6..d3ff124bf3 100644 |
26eee146 SR |
193 | --- a/stubs/monitor-core.c |
194 | +++ b/stubs/monitor-core.c | |
5b15e2ec | 195 | @@ -12,6 +12,11 @@ Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) |
26eee146 SR |
196 | return NULL; |
197 | } | |
198 | ||
199 | +int monitor_get_connection_nr(const Monitor *mon) | |
200 | +{ | |
201 | + return -1; | |
202 | +} | |
203 | + | |
204 | void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp) | |
205 | { | |
206 | } |