On 08/11/2017 02:48 AM, Vladimir Sementsov-Ogievskiy wrote: > 11.08.2017 05:37, Eric Blake wrote: >> As soon as the server is sending us garbage, we should quit >> trying to send further messages to the server, and allow all >> pending coroutines for any remaining replies to error out. >> Failure to do so can let a malicious server cause the client >> to hang, for example, if the server sends an invalid magic >> number in its response.
>> @@ -107,8 +108,12 @@ static coroutine_fn void
>> nbd_read_reply_entry(void *opaque)
>> qemu_coroutine_yield();
>> }
>>
>> + s->reply.handle = 0;
>> nbd_recv_coroutines_enter_all(s);
>> s->read_reply_co = NULL;
>> + if (ret < 0) {
>> + nbd_teardown_connection(bs);
>> + }
>
> what if it happens in parallel with nbd_co_send_request?
> nbd_teardown_connectin destroys s->ioc, nbd_co_send_requests
> checks s->ioc only once and then calls nbd_send_request (which is
> finally nbd_rwv and may yield). I think nbd_rwv is not
> prepared to sudden destruction of ioc..
The nbd_recv_coroutines_enter_all() call schedules all pending
nbd_co_send_request coroutines to fire as soon as the current coroutine
reaches a yield point. The next yield point is during the
BDRV_POLL_WHILE of nbd_teardown_connection - but this is AFTER we've
called qio_channel_shutdown() - so as long as nbd_rwv() is called with a
valid ioc, the qio code should recognize that we are shutting down the
connection and gracefully give an error on each write attempt.
I see your point about the fact that coroutines can change hands in
between our two writes for an NBD_CMD_WRITE in nbd_co_send_request()
(the first write is nbd_send_request() for the header, the second is
nbd_rwv() for the data) - if between those two writes we process a
failing read, I see your point about us risking re-reading s->ioc as
NULL for the second write call. But maybe this is an appropriate fix -
hanging on to the ioc that we learned when grabbing the send_mutex:
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 802d50b636..28b10f3fa2 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -122,6 +122,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
{
NBDClientSession *s = nbd_get_client_session(bs);
int rc, ret, i;
+ QIOChannel *ioc;
qemu_co_mutex_lock(&s->send_mutex);
while (s->in_flight == MAX_NBD_REQUESTS) {
@@ -139,25 +140,26 @@ static int nbd_co_send_request(BlockDriverState *bs,
g_assert(qemu_in_coroutine());
assert(i < MAX_NBD_REQUESTS);
request->handle = INDEX_TO_HANDLE(s, i);
+ ioc = s->ioc;
- if (!s->ioc) {
+ if (!ioc) {
qemu_co_mutex_unlock(&s->send_mutex);
return -EPIPE;
}
if (qiov) {
- qio_channel_set_cork(s->ioc, true);
- rc = nbd_send_request(s->ioc, request);
+ qio_channel_set_cork(ioc, true);
+ rc = nbd_send_request(ioc, request);
if (rc >= 0) {
- ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len,
false,
+ ret = nbd_rwv(ioc, qiov->iov, qiov->niov, request->len, false,
NULL);
if (ret != request->len) {
rc = -EIO;
}
}
- qio_channel_set_cork(s->ioc, false);
+ qio_channel_set_cork(ioc, false);
} else {
- rc = nbd_send_request(s->ioc, request);
+ rc = nbd_send_request(ioc, request);
}
qemu_co_mutex_unlock(&s->send_mutex);
return rc;
But I'm really hoping Paolo will chime in on this thread.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
