Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 14/06/2016 17:59, Alex Bligh wrote:
> 
>> On 14 Jun 2016, at 16:11, Paolo Bonzini  wrote:
>>
>>> To illustrate the problem, look consider what qemu itself would do as
>>> a server if it can't buffer the entire read issued to it.
>>
>> Return ENOMEM?
> 
> Well OK, qemu then 'works' on the basis it breaks another
> part of the spec, which is coping with long reads.

ENOMEM is a documented error code, and the limits extension will help
with that as well.

>> However, it looks like the
>> de facto status prior to structured replies is that the error is in the
>> spec, and this patch introduces a regression.
> 
> Well, I guess the patch makes it work the same as the
> reference server implementation and the spec, which I'd
> consider a fix. My view is that the error is in the
> kernel client.

... and QEMU and BSD.  What good is a server that doesn't interoperate
(albeit only in error cases) with any client?

Paolo

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake]

On 06/13/2016 06:19 AM, Paolo Bonzini wrote:
> 
> 
> On 12/05/2016 00:39, Eric Blake wrote:
>> We have a few bugs in how we handle invalid client commands:
>>
>> - A client can send an NBD_CMD_DISC where from + len overflows,
>> convincing us to reply with an error and stay connected, even
>> though the protocol requires us to silently disconnect. Fix by
>> hoisting the special case sooner.
>>

> It's simpler to always set req->complete.  Putting everything together:
> 
> diff --git a/nbd/server.c b/nbd/server.c

> @@ -1213,12 +1218,9 @@ static void nbd_trip(void *opaque)
>  LOG("invalid request type (%" PRIu32 ") received", request.type);
>  reply.error = EINVAL;
>  error_reply:
> -/* We must disconnect after replying with an error to
> - * NBD_CMD_READ, since we choose not to send bogus filler
> - * data; likewise after NBD_CMD_WRITE if we did not read the
> - * payload. */
> -if (nbd_co_send_reply(req, , 0) < 0 || command == NBD_CMD_READ 
> ||
> -(command == NBD_CMD_WRITE && !req->complete)) {
> +/* We must disconnect after NBD_CMD_WRITE if we did not
> + * read the payload. */
> +if (nbd_co_send_reply(req, , 0) < 0 || !req->complete)) {

This doesn't even compile (too many ')').  I assume you'll fix that
before your actual pull request goes out.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 14/06/2016 17:02, Alex Bligh wrote:
> 
> On 14 Jun 2016, at 14:32, Paolo Bonzini  wrote:
> 
>>
>> On 13/06/2016 23:41, Alex Bligh wrote:
>>> That's one of the reasons that there is a proposal to add
>>> STRUCTURED_READ to the spec (although I still haven't had time to
>>> implement that for qemu), so that we have a newer approach that allows
>>> for proper error handling without ambiguity on whether bogus bytes must
>>> be sent on a failed read.  But you'd have to convince me that ALL
>>> existing NBD server and client implementations expect to handle a read
>>> error without read payload, otherwise, I will stick with the notion that
>>> the current spec wording is correct, and that read errors CANNOT be
>>> gracefully recovered from unless BOTH sides transfer (possibly bogus)
>>> bytes along with the error message, and which is why BOTH sides of the
>>> protocol are warned that read errors usually result in a disconnection
>>> rather than clean continuation, without the addition of STRUCTURED_READ.
>>
>> I suspect that there are exactly two client implementations,
> 
> My understanding is that there are more than 2 client implementations.
> A quick google found at least one BSD client. I bet read error handling
> is a mess in all of them.

Found it, it is exactly the same as Linux and QEMU:

https://github.com/bitrig/bitrig/blob/418985278/sys/dev/nbd.c#L577

>> namely
>> Linux and QEMU's, and both do the right thing.
> 
> This depends what you mean by 'right'. Both appear to be non-compliant
> with the standard.

I mean "what makes sense".

> Note the standard is not defined by the client implementation, but
> by the protocol document.
> 
> IMHO the 'right thing' is what is in the spec. Servers can't send an
> error in any other way if they don't buffer the entire read first, as the
> read may error towards the end.
> 
> To illustrate the problem, look consider what qemu itself would do as
> a server if it can't buffer the entire read issued to it.

Return ENOMEM?

> The spec originally was not clear on how errors on reads should be
> handled, leading to any read causing a protocol drop. The spec is
> now clear. Unfortunately it is not possible to make a back compatible
> fix. Hence the real fix here is to implement structured replies,
> which is what Eric and I have been working on.

I agree that structured replies are better.  However, it looks like the
de facto status prior to structured replies is that the error is in the
spec, and this patch introduces a regression.

Paolo

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Alex Bligh]

> On 14 Jun 2016, at 16:11, Paolo Bonzini  wrote:
> 
>> To illustrate the problem, look consider what qemu itself would do as
>> a server if it can't buffer the entire read issued to it.
> 
> Return ENOMEM?

Well OK, qemu then 'works' on the basis it breaks another
part of the spec, which is coping with long reads.

> However, it looks like the
> de facto status prior to structured replies is that the error is in the
> spec, and this patch introduces a regression.

Well, I guess the patch makes it work the same as the
reference server implementation and the spec, which I'd
consider a fix. My view is that the error is in the
kernel client. I think Erik CC'd in nbd-general
re the comment that the spec was broken; I don't think
it is, and don't propose to change it. Wouter might or
might not feel differently.

It's been reasonably well known (I wrote about it
at least 3 years ago), that the current implementation
(reference + kernel) does not cope well with errors
on reads, so I'm guessing one is just trading one
set of brokenness for another. So I'm pretty relaxed
about what goes in qemu.

-- 
Alex Bligh

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Alex Bligh]

On 14 Jun 2016, at 14:32, Paolo Bonzini  wrote:

> 
> On 13/06/2016 23:41, Alex Bligh wrote:
>> That's one of the reasons that there is a proposal to add
>> STRUCTURED_READ to the spec (although I still haven't had time to
>> implement that for qemu), so that we have a newer approach that allows
>> for proper error handling without ambiguity on whether bogus bytes must
>> be sent on a failed read.  But you'd have to convince me that ALL
>> existing NBD server and client implementations expect to handle a read
>> error without read payload, otherwise, I will stick with the notion that
>> the current spec wording is correct, and that read errors CANNOT be
>> gracefully recovered from unless BOTH sides transfer (possibly bogus)
>> bytes along with the error message, and which is why BOTH sides of the
>> protocol are warned that read errors usually result in a disconnection
>> rather than clean continuation, without the addition of STRUCTURED_READ.
> 
> I suspect that there are exactly two client implementations,

My understanding is that there are more than 2 client implementations.
A quick google found at least one BSD client. I bet read error handling
is a mess in all of them.

> namely
> Linux and QEMU's, and both do the right thing.

This depends what you mean by 'right'. Both appear to be non-compliant
with the standard.

Note the standard is not defined by the client implementation, but
by the protocol document.

IMHO the 'right thing' is what is in the spec. Servers can't send an
error in any other way if they don't buffer the entire read first, as the
read may error towards the end.

To illustrate the problem, look consider what qemu itself would do as
a server if it can't buffer the entire read issued to it.

> What servers do doesn't matter, if all the clients agree.

The spec originally was not clear on how errors on reads should be
handled, leading to any read causing a protocol drop. The spec is
now clear. Unfortunately it is not possible to make a back compatible
fix. Hence the real fix here is to implement structured replies,
which is what Eric and I have been working on.

-- 
Alex Bligh

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 13/06/2016 23:41, Alex Bligh wrote:
> That's one of the reasons that there is a proposal to add
> STRUCTURED_READ to the spec (although I still haven't had time to
> implement that for qemu), so that we have a newer approach that allows
> for proper error handling without ambiguity on whether bogus bytes must
> be sent on a failed read.  But you'd have to convince me that ALL
> existing NBD server and client implementations expect to handle a read
> error without read payload, otherwise, I will stick with the notion that
> the current spec wording is correct, and that read errors CANNOT be
> gracefully recovered from unless BOTH sides transfer (possibly bogus)
> bytes along with the error message, and which is why BOTH sides of the
> protocol are warned that read errors usually result in a disconnection
> rather than clean continuation, without the addition of STRUCTURED_READ.

I suspect that there are exactly two client implementations, namely
Linux and QEMU's, and both do the right thing.

What servers do doesn't matter, if all the clients agree.

Paolo

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Alex Bligh]

On 13 Jun 2016, at 13:25, Eric Blake  wrote:

> On 06/13/2016 06:10 AM, Paolo Bonzini wrote:
>> 
>> 
>> On 12/05/2016 00:39, Eric Blake wrote:
>>> - If we report an error to NBD_CMD_READ, we are not writing out
>>> any data payload; but the protocol says that a client can expect
>>> to read the payload no matter what (and must instead ignore it),
>>> which means the client will start reading our next replies as
>>> its data payload. Fix by disconnecting (an alternative fix of
>>> sending bogus payload would be trickier to implement).
>> 
>> This is an error in the spec.  The Linux driver doesn't expect to read
>> the payload here, and neither does block/nbd-client.c.
> 
> That's one of the reasons that there is a proposal to add
> STRUCTURED_READ to the spec (although I still haven't had time to
> implement that for qemu), so that we have a newer approach that allows
> for proper error handling without ambiguity on whether bogus bytes must
> be sent on a failed read.  But you'd have to convince me that ALL
> existing NBD server and client implementations expect to handle a read
> error without read payload, otherwise, I will stick with the notion that
> the current spec wording is correct, and that read errors CANNOT be
> gracefully recovered from unless BOTH sides transfer (possibly bogus)
> bytes along with the error message, and which is why BOTH sides of the
> protocol are warned that read errors usually result in a disconnection
> rather than clean continuation, without the addition of STRUCTURED_READ.

To back up what Eric said:

Unfortunately the design is pretty much broken for reporting errors
on reads (at least in part as there is no way to signal errors that
occur after some of the reply has been written).

The spec specifies that on a read, no matter whether or not there
is an error, the data is all sent. This was after some mailing
list conversations on the subject which indicated this was the
least broken way to do things (IIRC).

This is actually what nbd-server.c does in the threaded handler:
 https://github.com/yoe/nbd/blob/master/nbd-server.c#L1468

For amusement value, the non-threaded handler (which is not used
any more) does not send any payload on an error:
 https://github.com/yoe/nbd/blob/master/nbd-server.c#L1734

In essence read error handling is a horrible mess in NBD, and
I would not expect it to work in general :-(

--
Alex Bligh






signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake]

On 06/13/2016 06:19 AM, Paolo Bonzini wrote:

>> +/* Sanity checks, part 2. */
>> +if (request->from + request->len > client->exp->size) {
>> +LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
>> +", Size: %" PRIu64, request->from, request->len,
>> +(uint64_t)client->exp->size);
>> +rc = -EINVAL;
> 
> For writes, this should be ENOSPC according to the spec.

Good call.


>> +if (nbd_co_send_reply(req, , 0) < 0 || command == 
>> NBD_CMD_READ ||
>> +(command == NBD_CMD_WRITE && !req->complete)) {
> 
> It's simpler to always set req->complete.  Putting everything together:
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 4743316..73505dc 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1017,6 +1017,11 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
>  TRACE("Decoding type");
>  
>  command = request->type & NBD_CMD_MASK_COMMAND;
> +if (command != NBD_CMD_WRITE) {
> +/* No payload, we are ready to read the next request.  */
> +req->complete = true;
> +}
> +

Nice.

> @@ -1213,12 +1218,9 @@ static void nbd_trip(void *opaque)
>  LOG("invalid request type (%" PRIu32 ") received", request.type);
>  reply.error = EINVAL;
>  error_reply:
> -/* We must disconnect after replying with an error to
> - * NBD_CMD_READ, since we choose not to send bogus filler
> - * data; likewise after NBD_CMD_WRITE if we did not read the
> - * payload. */
> -if (nbd_co_send_reply(req, , 0) < 0 || command == NBD_CMD_READ 
> ||
> -(command == NBD_CMD_WRITE && !req->complete)) {
> +/* We must disconnect after NBD_CMD_WRITE if we did not
> + * read the payload. */
> +if (nbd_co_send_reply(req, , 0) < 0 || !req->complete)) {

I'm not sure I agree with your change on NBD_CMD_READ, but we can hash
that out with upstream NBD list on the correct protocol, and make any
further changes as a followup.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake]

[adding nbd list]

On 06/13/2016 06:10 AM, Paolo Bonzini wrote:
> 
> 
> On 12/05/2016 00:39, Eric Blake wrote:
>> - If we report an error to NBD_CMD_READ, we are not writing out
>> any data payload; but the protocol says that a client can expect
>> to read the payload no matter what (and must instead ignore it),
>> which means the client will start reading our next replies as
>> its data payload. Fix by disconnecting (an alternative fix of
>> sending bogus payload would be trickier to implement).
> 
> This is an error in the spec.  The Linux driver doesn't expect to read
> the payload here, and neither does block/nbd-client.c.

That's one of the reasons that there is a proposal to add
STRUCTURED_READ to the spec (although I still haven't had time to
implement that for qemu), so that we have a newer approach that allows
for proper error handling without ambiguity on whether bogus bytes must
be sent on a failed read.  But you'd have to convince me that ALL
existing NBD server and client implementations expect to handle a read
error without read payload, otherwise, I will stick with the notion that
the current spec wording is correct, and that read errors CANNOT be
gracefully recovered from unless BOTH sides transfer (possibly bogus)
bytes along with the error message, and which is why BOTH sides of the
protocol are warned that read errors usually result in a disconnection
rather than clean continuation, without the addition of STRUCTURED_READ.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 12/05/2016 00:39, Eric Blake wrote:
> We have a few bugs in how we handle invalid client commands:
> 
> - A client can send an NBD_CMD_DISC where from + len overflows,
> convincing us to reply with an error and stay connected, even
> though the protocol requires us to silently disconnect. Fix by
> hoisting the special case sooner.
> 
> - A client can send an NBD_CMD_WRITE with bogus from and len,
> where we reply to the client with EINVAL without consuming the
> payload; this will normally cause us to fail if the next thing
> read is not the right magic, but in rare cases, could cause us
> to interpret the data payload as valid commands and do things
> not requested by the client. Fix by adding a complete flag to
> track whether we are in sync or must disconnect.
> 
> - If we report an error to NBD_CMD_READ, we are not writing out
> any data payload; but the protocol says that a client can expect
> to read the payload no matter what (and must instead ignore it),
> which means the client will start reading our next replies as
> its data payload. Fix by disconnecting (an alternative fix of
> sending bogus payload would be trickier to implement).
> 
> Furthermore, we have split the checks for bogus from/len across
> two functions, when it is easier to do it all at once.
> 
> Signed-off-by: Eric Blake 
> ---
>  nbd/server.c | 67 
> +---
>  1 file changed, 46 insertions(+), 21 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 53507c5..9ac7e01 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -52,6 +52,7 @@ struct NBDRequest {
>  QSIMPLEQ_ENTRY(NBDRequest) entry;
>  NBDClient *client;
>  uint8_t *data;
> +bool complete;
>  };
> 
>  struct NBDExport {
> @@ -989,7 +990,13 @@ static ssize_t nbd_co_send_reply(NBDRequest *req, struct 
> nbd_reply *reply,
>  return rc;
>  }
> 
> -static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request 
> *request)
> +/* Collect a client request.  Return 0 if request looks valid, -EAGAIN
> + * to keep trying the collection, -EIO to drop connection right away,
> + * and any other negative value to report an error to the client
> + * (although the caller may still need to disconnect after reporting
> + * the error).  */
> +static ssize_t nbd_co_receive_request(NBDRequest *req,
> +  struct nbd_request *request)
>  {
>  NBDClient *client = req->client;
>  uint32_t command;
> @@ -1007,16 +1014,26 @@ static ssize_t nbd_co_receive_request(NBDRequest 
> *req, struct nbd_request *reque
>  goto out;
>  }
> 
> -if ((request->from + request->len) < request->from) {
> -LOG("integer overflow detected! "
> -"you're probably being attacked");
> -rc = -EINVAL;
> -goto out;
> -}
> -
>  TRACE("Decoding type");
> 
>  command = request->type & NBD_CMD_MASK_COMMAND;
> +if (command == NBD_CMD_DISC) {
> +/* Special case: we're going to disconnect without a reply,
> + * whether or not flags, from, or len are bogus */
> +TRACE("Request type is DISCONNECT");
> +rc = -EIO;
> +goto out;
> +}
> +
> +/* Check for sanity in the parameters, part 1.  Defer as many
> + * checks as possible until after reading any NBD_CMD_WRITE
> + * payload, so we can try and keep the connection alive.  */
> +if ((request->from + request->len) < request->from) {
> +LOG("integer overflow detected, you're probably being attacked");
> +rc = -EINVAL;
> +goto out;
> +}
> +
>  if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
>  if (request->len > NBD_MAX_BUFFER_SIZE) {
>  LOG("len (%" PRIu32" ) is larger than max len (%u)",
> @@ -1039,7 +1056,18 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, 
> struct nbd_request *reque
>  rc = -EIO;
>  goto out;
>  }
> +req->complete = true;
>  }
> +
> +/* Sanity checks, part 2. */
> +if (request->from + request->len > client->exp->size) {
> +LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
> +", Size: %" PRIu64, request->from, request->len,
> +(uint64_t)client->exp->size);
> +rc = -EINVAL;

For writes, this should be ENOSPC according to the spec.

> +goto out;
> +}
> +
>  rc = 0;
> 
>  out:
> @@ -1082,14 +1110,6 @@ static void nbd_trip(void *opaque)
>  goto error_reply;
>  }
>  command = request.type & NBD_CMD_MASK_COMMAND;
> -if (command != NBD_CMD_DISC && (request.from + request.len) > exp->size) 
> {
> -LOG("From: %" PRIu64 ", Len: %" PRIu32", Size: %" PRIu64
> -", Offset: %" PRIu64 "\n",
> -request.from, request.len,
> -(uint64_t)exp->size, (uint64_t)exp->dev_offset);
> -LOG("requested operation past EOF--bad client?");
> - 

Re: [Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 12/05/2016 00:39, Eric Blake wrote:
> - If we report an error to NBD_CMD_READ, we are not writing out
> any data payload; but the protocol says that a client can expect
> to read the payload no matter what (and must instead ignore it),
> which means the client will start reading our next replies as
> its data payload. Fix by disconnecting (an alternative fix of
> sending bogus payload would be trickier to implement).

This is an error in the spec.  The Linux driver doesn't expect to read
the payload here, and neither does block/nbd-client.c.

Paolo

[Qemu-block] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake]

We have a few bugs in how we handle invalid client commands:

- A client can send an NBD_CMD_DISC where from + len overflows,
convincing us to reply with an error and stay connected, even
though the protocol requires us to silently disconnect. Fix by
hoisting the special case sooner.

- A client can send an NBD_CMD_WRITE with bogus from and len,
where we reply to the client with EINVAL without consuming the
payload; this will normally cause us to fail if the next thing
read is not the right magic, but in rare cases, could cause us
to interpret the data payload as valid commands and do things
not requested by the client. Fix by adding a complete flag to
track whether we are in sync or must disconnect.

- If we report an error to NBD_CMD_READ, we are not writing out
any data payload; but the protocol says that a client can expect
to read the payload no matter what (and must instead ignore it),
which means the client will start reading our next replies as
its data payload. Fix by disconnecting (an alternative fix of
sending bogus payload would be trickier to implement).

Furthermore, we have split the checks for bogus from/len across
two functions, when it is easier to do it all at once.

Signed-off-by: Eric Blake 
---
 nbd/server.c | 67 +---
 1 file changed, 46 insertions(+), 21 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 53507c5..9ac7e01 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -52,6 +52,7 @@ struct NBDRequest {
 QSIMPLEQ_ENTRY(NBDRequest) entry;
 NBDClient *client;
 uint8_t *data;
+bool complete;
 };

 struct NBDExport {
@@ -989,7 +990,13 @@ static ssize_t nbd_co_send_reply(NBDRequest *req, struct 
nbd_reply *reply,
 return rc;
 }

-static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request 
*request)
+/* Collect a client request.  Return 0 if request looks valid, -EAGAIN
+ * to keep trying the collection, -EIO to drop connection right away,
+ * and any other negative value to report an error to the client
+ * (although the caller may still need to disconnect after reporting
+ * the error).  */
+static ssize_t nbd_co_receive_request(NBDRequest *req,
+  struct nbd_request *request)
 {
 NBDClient *client = req->client;
 uint32_t command;
@@ -1007,16 +1014,26 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, 
struct nbd_request *reque
 goto out;
 }

-if ((request->from + request->len) < request->from) {
-LOG("integer overflow detected! "
-"you're probably being attacked");
-rc = -EINVAL;
-goto out;
-}
-
 TRACE("Decoding type");

 command = request->type & NBD_CMD_MASK_COMMAND;
+if (command == NBD_CMD_DISC) {
+/* Special case: we're going to disconnect without a reply,
+ * whether or not flags, from, or len are bogus */
+TRACE("Request type is DISCONNECT");
+rc = -EIO;
+goto out;
+}
+
+/* Check for sanity in the parameters, part 1.  Defer as many
+ * checks as possible until after reading any NBD_CMD_WRITE
+ * payload, so we can try and keep the connection alive.  */
+if ((request->from + request->len) < request->from) {
+LOG("integer overflow detected, you're probably being attacked");
+rc = -EINVAL;
+goto out;
+}
+
 if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
 if (request->len > NBD_MAX_BUFFER_SIZE) {
 LOG("len (%" PRIu32" ) is larger than max len (%u)",
@@ -1039,7 +1056,18 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, 
struct nbd_request *reque
 rc = -EIO;
 goto out;
 }
+req->complete = true;
 }
+
+/* Sanity checks, part 2. */
+if (request->from + request->len > client->exp->size) {
+LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
+", Size: %" PRIu64, request->from, request->len,
+(uint64_t)client->exp->size);
+rc = -EINVAL;
+goto out;
+}
+
 rc = 0;

 out:
@@ -1082,14 +1110,6 @@ static void nbd_trip(void *opaque)
 goto error_reply;
 }
 command = request.type & NBD_CMD_MASK_COMMAND;
-if (command != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
-LOG("From: %" PRIu64 ", Len: %" PRIu32", Size: %" PRIu64
-", Offset: %" PRIu64 "\n",
-request.from, request.len,
-(uint64_t)exp->size, (uint64_t)exp->dev_offset);
-LOG("requested operation past EOF--bad client?");
-goto invalid_request;
-}

 if (client->closing) {
 /*
@@ -1151,10 +1171,11 @@ static void nbd_trip(void *opaque)
 goto out;
 }
 break;
+
 case NBD_CMD_DISC:
-TRACE("Request type is DISCONNECT");
-errno = 0;
-goto out;
+/* unreachable, thanks to special case in