| Message ID | 1463006384-7734-5-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
[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.
On 13 Jun 2016, at 13:25, Eric Blake <eblake@redhat.com> 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
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
On 14 Jun 2016, at 14:32, Paolo Bonzini <pbonzini@redhat.com> 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.
On 14/06/2016 17:02, Alex Bligh wrote: > > On 14 Jun 2016, at 14:32, Paolo Bonzini <pbonzini@redhat.com> 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
> On 14 Jun 2016, at 16:11, Paolo Bonzini <pbonzini@redhat.com> 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.
On 14/06/2016 17:59, Alex Bligh wrote: > >> On 14 Jun 2016, at 16:11, Paolo Bonzini <pbonzini@redhat.com> 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
On Mon, Jun 13, 2016 at 10:41:05PM +0100, Alex Bligh wrote: > 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 nbd-server used to just drop the connection on read error. > In essence read error handling is a horrible mess in NBD, and > I would not expect it to work in general :-(
On Tue, Jun 14, 2016 at 04:02:15PM +0100, Alex Bligh wrote: > > On 14 Jun 2016, at 14:32, Paolo Bonzini <pbonzini@redhat.com> 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. No, it isn't. At least not yet. The standard document has only become formal a few months ago. It's certainly possible that we made a mistake formalizing things, and if so, we should fix the document rather than saying "whatever you've been doing these years, even though it worked, is wrong". There are more clients than the Linux and qemu ones, but I think it's fair to say that those two are the most important ones. If they agree that a read reply which errors should come without payload, then I think we should update the standard to say that, too. > > 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. That much, at any rate, is true.
On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote: > There are more clients than the Linux and qemu ones, but I think it's > fair to say that those two are the most important ones. If they agree > that a read reply which errors should come without payload, then I think > we should update the standard to say that, too. I've just pushed a commit that changes the spec (and the implementation) so that if a server encounters a read error, it does not send a payload. In other words, the current behaviour of qemu is correct, is now documented to be correct, and should not be changed. Regards,
> On 15 Jun 2016, at 09:03, Wouter Verhelst <w@uter.be> wrote: > > On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote: >> There are more clients than the Linux and qemu ones, but I think it's >> fair to say that those two are the most important ones. If they agree >> that a read reply which errors should come without payload, then I think >> we should update the standard to say that, too. > > I've just pushed a commit that changes the spec (and the implementation) > so that if a server encounters a read error, it does not send a payload. > > In other words, the current behaviour of qemu is correct, is now > documented to be correct, and should not be changed. So what should those servers do (like 2 of mine) which don't buffer the entire read, if they get an error having already sent some data? -- Alex Bligh
----- Original Message ----- > From: "Alex Bligh" <alex@alex.org.uk> > To: "Wouter Verhelst" <w@uter.be> > Cc: "Alex Bligh" <alex@alex.org.uk>, nbd-general@lists.sourceforge.net, "Paolo Bonzini" <pbonzini@redhat.com>, > qemu-devel@nongnu.org, "qemu block" <qemu-block@nongnu.org> > Sent: Wednesday, June 15, 2016 10:52:21 AM > Subject: Re: [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands > > > > On 15 Jun 2016, at 09:03, Wouter Verhelst <w@uter.be> wrote: > > > > On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote: > >> There are more clients than the Linux and qemu ones, but I think it's > >> fair to say that those two are the most important ones. If they agree > >> that a read reply which errors should come without payload, then I think > >> we should update the standard to say that, too. > > > > I've just pushed a commit that changes the spec (and the implementation) > > so that if a server encounters a read error, it does not send a payload. > > > > In other words, the current behaviour of qemu is correct, is now > > documented to be correct, and should not be changed. > > So what should those servers do (like 2 of mine) which don't buffer > the entire read, if they get an error having already sent some data? They have sent an error code of zero, and it turned out to be wrong. So the only thing they can do safely is disconnect. Paolo
On 15 Jun 2016, at 10:18, Paolo Bonzini <pbonzini@redhat.com> wrote: >> So what should those servers do (like 2 of mine) which don't buffer >> the entire read, if they get an error having already sent some data? > > They have sent an error code of zero, and it turned out to be wrong. So > the only thing they can do safely is disconnect. Right, but that is not what Wouter's change says: + If an error occurs, the server SHOULD set the appropriate error code + in the error field. The server MAY then initiate a hard disconnect. + If it chooses not to, it MUST NOT send any payload for this request. I read this as either a) the server can issue a hard disconnect without sending any reply; or b) it must send the reply header with no payload It also seems to permit not setting the error code (it's only a 'SHOULD'), not disconnecting (it's a MAY), then not sending any payload, which is a nonsense. Perhaps this should read "If an error occurs, the server MUST either initiate a hard disconnect before the entire payload has been sent or set the appropriate code in the error field and send the response header without any payload." if we want to go down this route.
On 15/06/2016 12:27, Alex Bligh wrote: > > On 15 Jun 2016, at 10:18, Paolo Bonzini <pbonzini@redhat.com> wrote: > >>> So what should those servers do (like 2 of mine) which don't buffer >>> the entire read, if they get an error having already sent some data? >> >> They have sent an error code of zero, and it turned out to be wrong. So >> the only thing they can do safely is disconnect. > > Right, but that is not what Wouter's change says: > > + If an error occurs, the server SHOULD set the appropriate error code > + in the error field. The server MAY then initiate a hard disconnect. > + If it chooses not to, it MUST NOT send any payload for this request. > > I read this as either > > a) the server can issue a hard disconnect without sending any reply; or > > b) it must send the reply header with no payload > > It also seems to permit not setting the error code (it's only a 'SHOULD'), > not disconnecting (it's a MAY), then not sending any payload, which is a > nonsense. Right. > Perhaps this should read "If an error occurs, the server MUST either initiate > a hard disconnect before the entire payload has been sent or > set the appropriate code in the error field and send the response header > without any payload." if we want to go down this route. Yes, I agree. I do believe we want to go down this route. I think we agree that partial buffering may always require the server to disconnect after an error. Therefore I don't see any benefit at all in sending a payload after an error message. Paolo
On Wed, Jun 15, 2016 at 11:27:21AM +0100, Alex Bligh wrote: > Perhaps this should read "If an error occurs, the server MUST either initiate > a hard disconnect before the entire payload has been sent or > set the appropriate code in the error field and send the response header > without any payload." if we want to go down this route. Something along those lines is what I meant, yes.
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 nbd_co_receive_request() */ + abort(); + case NBD_CMD_FLUSH: TRACE("Request type is FLUSH"); @@ -1182,10 +1203,14 @@ static void nbd_trip(void *opaque) break; default: LOG("invalid request type (%" PRIu32 ") received", request.type); - invalid_request: reply.error = EINVAL; error_reply: - if (nbd_co_send_reply(req, &reply, 0) < 0) { + /* 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, &reply, 0) < 0 || command == NBD_CMD_READ || + (command == NBD_CMD_WRITE && !req->complete)) { goto out; } break;
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 <eblake@redhat.com> --- nbd/server.c | 67 +++++++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 21 deletions(-)