qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entr


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
Date: Mon, 7 Aug 2017 19:09:01 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

07.08.2017 18:33, Eric Blake wrote:
On 08/07/2017 10:13 AM, Eric Blake wrote:
On 08/07/2017 07:56 AM, Vladimir Sementsov-Ogievskiy wrote:
07.08.2017 14:52, Eric Blake wrote:
On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply.

      

      
...and the client is recognizing that the server sent garbage, but then
proceeds to handle the packet anyway.  The ideal reaction is that the
client should disconnect from the server, rather than handle the packet.
 But because it didn't do that, the client is now unable to receive any
future messages from the server.  Compare the traces of:


      
followed by a clean disconnect; vs. the buggy server:

address@hidden:nbd_opt_go_success Export is good to go
address@hidden:nbd_send_request Sending request to server: {
.from = 0, .len = 1, .handle = 94152262559840, .flags = 0x0, .type = 0
(read) }
address@hidden:nbd_receive_reply Got reply: { magic =
0x1446698, .error =  0, handle = 94152262559840 }
invalid magic (got 0x1446698)
read 1/1 bytes at offset 0
1 bytes, 1 ops; 0:00:12.10 (0.082581 bytes/sec and 0.0826 ops/sec)
address@hidden:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 94152262559840, .flags = 0x0, .type = 3
(flush) }

where the client is now hung.  Thankfully, the client is blocked in an
idle loop (not eating CPU), so I don't know if this counts as the
ability for a malicious server to cause a denial of service against qemu
as an NBD client (in general, being unable to read further data from the
server because client internal state is now botched is not that much
different from being unable to read further data from the server because
the client hung up on the invalid server), unless there is some way to
cause qemu to die from an assertion failure rather than just get stuck.
With just patch 6/17 applied, the client still hangs, but this time with
a different trace:

address@hidden:nbd_opt_go_success Export is good to go
address@hidden:nbd_send_request Sending request to server: {
.from = 0, .len = 1, .handle = 94716063746144, .flags = 0x0, .type = 0
(read) }
address@hidden:nbd_receive_reply Got reply: { magic =
0x1446698, .error =  0, handle = 94716063746144 }
invalid magic (got 0x1446698)
read failed: Input/output error
address@hidden:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 94716063746144, .flags = 0x0, .type = 3
(flush) }

The client still tried to send a flush request to the server, when it
should REALLY quit talking to the server at all and just disconnect,
because the moment the client recognizes that the server has sent
garbage is the moment that the client can no longer assume that the
server will react correctly to any further commands.

So I don't think your patch is quite correct, even if you have
identified a shortfall in our client code.

Why do you think so? It just does what it states in commit message.

Patch 17 should fix the case (but I doubt that it can be taken separately).


-- 
Best regards,
Vladimir

reply via email to

[Prev in Thread] Current Thread [Next in Thread]