[Qemu-devel] [PATCH for-2.10 0/2] Fix NBD client after server error

[Qemu-devel] [PATCH for-2.10 0/2] Fix NBD client after server error

[Eric Blake]

Patch 1 is a much smaller patch than Vladimir's attempt [1] at fixing
the client in the face of a malicious server.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01501.html

Patch 2 is not to be applied; it is a hack for easily creating a
malicious server; by setting NBD_SERVER_DEBUG to a positive integer,
the server will intentionally send bad magic when it reaches that
many replies.

I tested using:
 NBD_SERVER_DEBUG=1 ./qemu-nbd -f raw -x foo file
coupled with
 qemu-io -c 'r 0 1' -c 'r 0 1' -f raw nbd://localhost:10809/foo

Without the patch, the qemu-io client hangs; with the patch, the
client reports 'read failed: Input/output error' for the first read
(where the bad server was detected) and 'read failed: Broken pipe'
for the second (because the client has already dropped the
connection from the bad server).

I would like this to go in -rc3, but would definitely appreciate
review, as the manipulation of coroutines was tricky for me to
step through in the debugger, and I want to make sure I'm not
leaking any memory or stranding an incomplete coroutine.

Eric Blake (2):
  nbd: Drop connection if broken server is detected
  HACK: define NBD_SERVER_DEBUG to force malicious server

 block/nbd-client.c |  9 +++++++--
 nbd/server.c       | 11 +++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

-- 
2.13.4

[Qemu-devel] [PATCH 1/2] nbd: Drop connection if broken server is detected

[Eric Blake]

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.

Reported by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..802d50b636 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -68,7 +68,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)

 static coroutine_fn void nbd_read_reply_entry(void *opaque)
 {
-    NBDClientSession *s = opaque;
+    BlockDriverState *bs = opaque;
+    NBDClientSession *s = nbd_get_client_session(bs);
     uint64_t i;
     int ret;
     Error *local_err = NULL;
@@ -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);
+    }
 }

 static int nbd_co_send_request(BlockDriverState *bs,
@@ -416,7 +421,7 @@ int nbd_client_init(BlockDriverState *bs,
     /* Now that we're connected, set the socket to be non-blocking and
      * kick the reply mechanism.  */
     qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
-    client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, client);
+    client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, bs);
     nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));

     logout("Established connection with NBD server\n");
-- 
2.13.4

Re: [Qemu-devel] [PATCH 1/2] nbd: Drop connection if broken server is detected

[Vladimir Sementsov-Ogievskiy]

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.
>
> Reported by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   block/nbd-client.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 25dd28406b..802d50b636 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -68,7 +68,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>
>   static coroutine_fn void nbd_read_reply_entry(void *opaque)
>   {
> -    NBDClientSession *s = opaque;
> +    BlockDriverState *bs = opaque;
> +    NBDClientSession *s = nbd_get_client_session(bs);
>       uint64_t i;
>       int ret;
>       Error *local_err = NULL;
> @@ -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..

>   }
>
>   static int nbd_co_send_request(BlockDriverState *bs,
> @@ -416,7 +421,7 @@ int nbd_client_init(BlockDriverState *bs,
>       /* Now that we're connected, set the socket to be non-blocking and
>        * kick the reply mechanism.  */
>       qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
> -    client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, client);
> +    client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, bs);
>       nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>
>       logout("Established connection with NBD server\n");


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/2] nbd: Drop connection if broken server is detected

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3794 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/2] nbd: Drop connection if broken server is detected

[Vladimir Sementsov-Ogievskiy]

11.08.2017 17:15, Eric Blake wrote:
> 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.


Hmm, was it correct even before your patch? Is it safe to enter a coroutine
(which we've scheduled by nbd_recv_coroutines_enter_all()), which is 
actually
yielded inside nbd_rwv (not our yield in nbd_co_receive_reply)?

>
> 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

But there are no yields between two writes, so, if previous logic is 
correct,
if the read fails during first write it will return and error and we 
will not go
into the second write. If it fails during the second write, it should be 
OK too.

> 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.
>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/2] nbd: Drop connection if broken server is detected

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4253 bytes --]

On 08/11/2017 09:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.08.2017 17:15, Eric Blake wrote:
>> 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.

>> 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.
> 
> 
> Hmm, was it correct even before your patch? Is it safe to enter a coroutine
> (which we've scheduled by nbd_recv_coroutines_enter_all()), which is
> actually
> yielded inside nbd_rwv (not our yield in nbd_co_receive_reply)?

I'm honestly not sure how to answer the question. In my testing, I was
unable to catch a coroutine yielding inside of nbd_rwv(); I was able to
easily provoke a situation where the client can send two or more
commands prior to the server getting a chance to reply to either:

./qemu-io -f raw nbd://localhost:10809/foo \
   -c 'aio_read 0 512' -c 'aio_write 1k 1m'

where tracing the server proves that the server received both commands
before sending a reply; when the client sends two aio_read commands, it
was even the case that I could observe the server replying to the second
read before the first.  So I'm definitely provoking parallel coroutines.
 But even without my tentative squash patch, I haven't been able to
observe s->ioc change from valid to NULL within the body of
nbd_co_send_request - either the entire request is skipped because ioc
was already cleared, or the entire request operates on a valid ioc
(although the request may still fail with EPIPE because the ioc has
started its efforts at shutdown).  I even tried varying the size of the
aio_write; with 1M, the client got the write request sent off before the
server's reply; but 2M was large enough that the server sent the read
reply before the client could send the write.  Since we are using a
mutex, we have at most one coroutine able to attempt a write at once;
but that still says nothing about how many other parallel coroutines can
wake up to do a read.  I also think the fact that we are using qio's
set_cork around the paired writes is helping: although we have two calls
to nbd_rwv(), the first one is for the NBD_CMD_WRITE header which is
small that the qio layer waits for the second nbd_rwv() call before
actually sending anything over the wire (if anything, we are more likely
to see s->ioc change before the final set_cork call, rather than between
the two writes - if that change can even happen).

>>
>> 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
> 
> But there are no yields between two writes, so, if previous logic is
> correct,
> if the read fails during first write it will return and error and we
> will not go
> into the second write. If it fails during the second write, it should be
> OK too.

If we can ever observe s->ioc changing to NULL, then my followup squash
patch is needed (if nothing else, calling qio_channel_set_cork(NULL,
false) will crash).  But I'm not familiar enough with coroutines to know
if it is possible, or just paranoia on my part.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/2] nbd: Drop connection if broken server is detected

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On 08/11/2017 02:41 PM, Eric Blake wrote:
>> Hmm, was it correct even before your patch? Is it safe to enter a coroutine
>> (which we've scheduled by nbd_recv_coroutines_enter_all()), which is
>> actually
>> yielded inside nbd_rwv (not our yield in nbd_co_receive_reply)?
> 
> I'm honestly not sure how to answer the question. In my testing, I was
> unable to catch a coroutine yielding inside of nbd_rwv();

Single stepping through nbd_rwv(), I see that I/O is performed by
sendmsg(), which either gets the message sent or, because of nonblocking
mode, fails with EAGAIN, which gets turned into QIO_CHANNEL_ERR_BLOCK
and indeed a call to qemu_channel_yield() within nbd_rwv() - but it's
timing sensitive, so I still haven't been able to provoke this scenario
using gdb.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/2] nbd: Drop connection if broken server is detected

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5049 bytes --]

On 08/11/2017 03:01 PM, Eric Blake wrote:
> On 08/11/2017 02:41 PM, Eric Blake wrote:
>>> Hmm, was it correct even before your patch? Is it safe to enter a coroutine
>>> (which we've scheduled by nbd_recv_coroutines_enter_all()), which is
>>> actually
>>> yielded inside nbd_rwv (not our yield in nbd_co_receive_reply)?
>>
>> I'm honestly not sure how to answer the question. In my testing, I was
>> unable to catch a coroutine yielding inside of nbd_rwv();
> 
> Single stepping through nbd_rwv(), I see that I/O is performed by
> sendmsg(), which either gets the message sent or, because of nonblocking
> mode, fails with EAGAIN, which gets turned into QIO_CHANNEL_ERR_BLOCK
> and indeed a call to qemu_channel_yield() within nbd_rwv() - but it's
> timing sensitive, so I still haven't been able to provoke this scenario
> using gdb.

With this compiled into the client:

diff --git i/nbd/common.c w/nbd/common.c
index a2f28f2eec..f10e991eed 100644
--- i/nbd/common.c
+++ w/nbd/common.c
@@ -36,6 +36,10 @@ ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov,
size_t niov, size_t length,

     while (nlocal_iov > 0) {
         ssize_t len;
+        static int hack;
+        if (hack) {
+            len = QIO_CHANNEL_ERR_BLOCK;
+        } else
         if (do_read) {
             len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
         } else {

I was able to set a breakpoint in gdb to temporarily manipulate 'hack'
at the right moment, in order to trigger what would happen if a
nbd_rwv() hit EAGAIN.  And sadly, I got a segfault using my patches,
because the last reference to ioc had been cleared in the meantime.


Program received signal SIGSEGV, Segmentation fault.
0x000055555562ee94 in object_class_dynamic_cast_assert
(class=0x555555d9d1b0,
    typename=0x5555556c856d "qio-channel", file=0x5555556c8560
"io/channel.c",
    line=75, func=0x5555556c8670 <__func__.21506> "qio_channel_writev_full")
    at qom/object.c:705
705	    trace_object_class_dynamic_cast_assert(class ? class->type->name
: "(null)",


#0  0x000055555562ee94 in object_class_dynamic_cast_assert (
    class=0x555555d9d1b0, typename=0x5555556c856d "qio-channel",
    file=0x5555556c8560 "io/channel.c", line=75,
    func=0x5555556c8670 <__func__.21506> "qio_channel_writev_full")
    at qom/object.c:705
#1  0x000055555562312d in qio_channel_writev_full (ioc=0x555555d9bde0,
    iov=0x555555d9ec90, niov=1, fds=0x0, nfds=0, errp=0x0) at
io/channel.c:75
#2  0x0000555555623233 in qio_channel_writev (ioc=0x555555d9bde0,
    iov=0x555555d9ec90, niov=1, errp=0x0) at io/channel.c:102
#3  0x0000555555603590 in nbd_rwv (ioc=0x555555d9bde0, iov=0x555555d9ecd0,
    niov=1, length=1048576, do_read=false, errp=0x0) at nbd/common.c:46
#4  0x00005555555ebcca in nbd_co_send_request (bs=0x555555d94260,
    request=0x7fffda2819f0, qiov=0x555555d9ee88) at block/nbd-client.c:152


My next test is whether incrementing the ref-count to s->ioc for the
duration of nbd_co_send_request() is adequate to protect against this
problem:

diff --git i/block/nbd-client.c w/block/nbd-client.c
index 28b10f3fa2..cb0c4ebedf 100644
--- i/block/nbd-client.c
+++ w/block/nbd-client.c
@@ -147,6 +147,11 @@ static int nbd_co_send_request(BlockDriverState *bs,
         return -EPIPE;
     }

+    /*
+     * Make sure ioc stays live, even if another coroutine decides to
+     * kill the connection because of a server error.
+     */
+    object_ref(OBJECT(ioc));
     if (qiov) {
         qio_channel_set_cork(ioc, true);
         rc = nbd_send_request(ioc, request);
@@ -161,6 +166,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
     } else {
         rc = nbd_send_request(ioc, request);
     }
+    object_unref(OBJECT(ioc));
     qemu_co_mutex_unlock(&s->send_mutex);
     return rc;
 }

and that got further, only to crash at:

(gdb) c
Continuing.
readv failed: Input/output error
aio_write failed: Input/output error
qemu-io: block/block-backend.c:1211: blk_aio_write_entry: Assertion
`!rwco->qiov || rwco->qiov->size == acb->bytes' failed.

where rwco->qiov->size was garbage.

At this point, while I still think it might be possible to come up with
a less-invasive  solution than your v2 patch, I'm also at the point
where I want the bug fixed rather than me wallowing around trying to
debug coroutine interactions; and thus I'm leaning towards your v2 patch
as being more likely to be robust in the face of concurrency (it's not
killing ioc while other coroutines still exist, so much as just making
sure that every yield point checks if the kill switch has been triggered
for a short-circuit exit).  So I will probably be taking your version
and creating a pull request for -rc3 on Monday.  (Before I fully ack
your version, though, I _will_ be hammering at it under gdb the same way
I hammered at mine)


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH 2/2] HACK: define NBD_SERVER_DEBUG to force malicious server

[Eric Blake]

---
 nbd/server.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index 82a78bf439..d6fbd46370 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -919,6 +919,17 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
     stl_be_p(buf + 4, reply->error);
     stq_be_p(buf + 8, reply->handle);

+    static int debug;
+    static int count;
+    if (!count++) {
+        const char *str = getenv("NBD_SERVER_DEBUG");
+        if (str) {
+            debug = atoi(str);
+        }
+    }
+    if (debug && !(count % debug)) {
+        buf[0] = 0;
+    }
     return nbd_write(ioc, buf, sizeof(buf), errp);
 }

-- 
2.13.4

Re: [Qemu-devel] [PATCH for-2.10 0/2] Fix NBD client after server error

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

On 08/10/2017 09:37 PM, Eric Blake wrote:
> Patch 1 is a much smaller patch than Vladimir's attempt [1] at fixing
> the client in the face of a malicious server.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01501.html
> 
> Patch 2 is not to be applied; it is a hack for easily creating a
> malicious server; by setting NBD_SERVER_DEBUG to a positive integer,
> the server will intentionally send bad magic when it reaches that
> many replies.
> 
> I tested using:
>  NBD_SERVER_DEBUG=1 ./qemu-nbd -f raw -x foo file
> coupled with
>  qemu-io -c 'r 0 1' -c 'r 0 1' -f raw nbd://localhost:10809/foo
> 
> Without the patch, the qemu-io client hangs; with the patch, the
> client reports 'read failed: Input/output error' for the first read
> (where the bad server was detected) and 'read failed: Broken pipe'
> for the second (because the client has already dropped the
> connection from the bad server).

I've also confirmed that this is a regression from our 2.8 release
(introduced when 2.9 switched NBD to use coroutines).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]