| Message ID | 20171122101958.17065-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD server refactoring before BLOCK_STATUS | expand |
On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> The subject line says what and sort of implies why, but the commit message body is rather sparse. > --- > nbd/server.c | 34 ++++++++++++++++++++++------------ > 1 file changed, 22 insertions(+), 12 deletions(-) Not a net win in lines of code, so a bit more justification may be helpful. > > diff --git a/nbd/server.c b/nbd/server.c > index bccc0274e7..c9445a89e9 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -139,6 +139,19 @@ static void nbd_client_receive_next_request(NBDClient *client); > > */ > > +static inline int nbd_opt_read(NBDClient *client, void *buffer, size_t size, > + Error **errp) > +{ > + client->optlen -= size; > + return qio_channel_read_all(client->ioc, buffer, size, errp) < 0 ? -EIO : 0; Should this code check that client->optlen >= size, and issue the appropriate error message if not? That may centralize some of the error checking repeated elsewhere. > +} > + > +static inline int nbd_opt_drop(NBDClient *client, size_t size, Error **errp) > +{ > + client->optlen -= size; > + return nbd_drop(client->ioc, size, errp); Same question on size validation. > +} > + > /* Send a reply header, including length, but no payload. > * Return -errno on error, 0 on success. */ > static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type, > @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, > error_setg(errp, "Bad length received"); > return -EINVAL; > } > - if (nbd_read(client->ioc, name, client->optlen, errp) < 0) { > + if (nbd_opt_read(client, name, client->optlen, errp) < 0) { > error_prepend(errp, "read failed: "); > return -EINVAL; Here's an example caller that had a previous size validation. > } > @@ -383,40 +396,36 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, > msg = "overall request too short"; > goto invalid; > } > - if (nbd_read(client->ioc, &namelen, sizeof(namelen), errp) < 0) { > + if (nbd_opt_read(client, &namelen, sizeof(namelen), errp) < 0) { > return -EIO; And again, a size validation right before the call. > } > be32_to_cpus(&namelen); > - client->optlen -= sizeof(namelen); Okay, part of the refactoring means you don't have to remember to track remaining size separately from reading; that's a nice feature. I suspect it is also possible to assert that client->optlen is zero before reading the next opt (I'm reviewing the patch in order, we'll see if I come back here). [1] > if (namelen > client->optlen - sizeof(requests) || > (client->optlen - namelen) % 2) > { > msg = "name length is incorrect"; > goto invalid; > } > - if (nbd_read(client->ioc, name, namelen, errp) < 0) { > + if (nbd_opt_read(client, name, namelen, errp) < 0) { > return -EIO; > } Another size check prior to the call (actually checking multiple conditions)... > name[namelen] = '\0'; > - client->optlen -= namelen; > trace_nbd_negotiate_handle_export_name_request(name); > > - if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) { > + if (nbd_opt_read(client, &requests, sizeof(requests), errp) < 0) { > return -EIO; ...and no direct size check before this call, because the earlier size checks already covered it. Arguably, the in-place size check error messages may be slightly more precise, but any message at all about unexpected sizing is appropriate (given that only broken clients should be sending incorrect sizes) - so I'm still leaning towards a stronger refactoring that puts the size check in the helper function. > @@ -521,7 +530,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, > return rc; > > invalid: > - if (nbd_drop(client->ioc, client->optlen, errp) < 0) { > + if (nbd_opt_drop(client, client->optlen, errp) < 0) { > return -EIO; > } > return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, > @@ -715,7 +724,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, > return -EINVAL; > > default: > - if (nbd_drop(client->ioc, length, errp) < 0) { > + if (nbd_opt_drop(client, length, errp) < 0) { Isn't length == client->optlen here? If so, can we omit the length parameter to nbd_opt_drop(), and instead have it defined as dropping to the end of the current option? > @@ -821,6 +830,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, > if (ret < 0) { > return ret; > } > + assert(client->optlen == 0); [1] Ah, you did add the assertion. I'm going to propose a variant on this patch, to cover the points I made above about ease-of-use (and to hopefully show a net win in lines of code).
On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > nbd/server.c | 34 ++++++++++++++++++++++------------ > 1 file changed, 22 insertions(+), 12 deletions(-) > @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, > error_setg(errp, "Bad length received"); > return -EINVAL; > } > - if (nbd_read(client->ioc, name, client->optlen, errp) < 0) { > + if (nbd_opt_read(client, name, client->optlen, errp) < 0) { > error_prepend(errp, "read failed: "); > return -EINVAL; > } More context: name[client->optlen] = '\0'; Oops - that's broken, because client->optlen is now 0. Which means your code was only tested with empty-string default exports.
On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > nbd/server.c | 34 ++++++++++++++++++++++------------ > 1 file changed, 22 insertions(+), 12 deletions(-) > Hmm, revisiting my idea about moving more of the error checking into the helper function. As of this patch, we only have two clients of nbd_opt_read: > @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, > error_setg(errp, "Bad length received"); > return -EINVAL; > } > - if (nbd_read(client->ioc, name, client->optlen, errp) < 0) { > + if (nbd_opt_read(client, name, client->optlen, errp) < 0) { > error_prepend(errp, "read failed: "); > return -EINVAL; 1. NBD_OPT_EXPORT_NAME, where the length check is based on the maximum size name we're willing to accept (and NOT on comparison to the header size, as we request the entire client->optlen). This call cannot send an error reply (so if the length is bogus, we can just drop the connection by returning -EINVAL). Furthermore, once we handle this option, option processing is done; we do not reach the assert that client->optlen == 0. > } > @@ -383,40 +396,36 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, > msg = "overall request too short"; > goto invalid; > } > - if (nbd_read(client->ioc, &namelen, sizeof(namelen), errp) < 0) { > + if (nbd_opt_read(client, &namelen, sizeof(namelen), errp) < 0) { > return -EIO; > } 2. Multiple calls within NBD_OPT_INFO/NBD_OPT_GO. Here, the length check is based on our read being a subset of client->optlen, and our desired response on a failed check is whatever is at the invalid: label, namely: invalid: if (nbd_opt_drop(client, client->optlen, errp) < 0) { return -EIO; } return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, errp, "%s", msg); We want to drop all remaining data, reply to the client, and return 0 to keep the connection alive (or -EIO if the read or write failed). You are planning on adding more calls withing NBD_OPT_LIST_META_CONTEXT, which will have semantics more like 2 (if we detect an inconsistent size, we want to consume the rest of the input and return an EINVAL reply to the client, but keep the connection alive unless there is an I/O error in the meantime). I think that means we need a tri-state return from nbd_opt_read(): < 0 on I/O failure, 0 if the EINVAL message has been sent, and 1 if the read was successful; I also think it means that the handler for NBD_OPT_EXPORT_NAME does not really fit the bill for using the same handler. Hopefully this explanation will give you more insight into the counter-proposal patch I'm about to post.
On 11/22/2017 02:03 PM, Eric Blake wrote: > On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote: >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> nbd/server.c | 34 ++++++++++++++++++++++------------ >> 1 file changed, 22 insertions(+), 12 deletions(-) >> > > I think that means we need a tri-state return from nbd_opt_read(): < 0 > on I/O failure, 0 if the EINVAL message has been sent, and 1 if the read > was successful; I also think it means that the handler for > NBD_OPT_EXPORT_NAME does not really fit the bill for using the same > handler. Hopefully this explanation will give you more insight into the > counter-proposal patch I'm about to post. Hmm, I never posted the counter-proposal, because I got caught up in handling the CVEs that I spotted in the same area, then the 2.11 release. I'll get back to this series shortly.
diff --git a/nbd/server.c b/nbd/server.c index bccc0274e7..c9445a89e9 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -139,6 +139,19 @@ static void nbd_client_receive_next_request(NBDClient *client); */ +static inline int nbd_opt_read(NBDClient *client, void *buffer, size_t size, + Error **errp) +{ + client->optlen -= size; + return qio_channel_read_all(client->ioc, buffer, size, errp) < 0 ? -EIO : 0; +} + +static inline int nbd_opt_drop(NBDClient *client, size_t size, Error **errp) +{ + client->optlen -= size; + return nbd_drop(client->ioc, size, errp); +} + /* Send a reply header, including length, but no payload. * Return -errno on error, 0 on success. */ static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type, @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, error_setg(errp, "Bad length received"); return -EINVAL; } - if (nbd_read(client->ioc, name, client->optlen, errp) < 0) { + if (nbd_opt_read(client, name, client->optlen, errp) < 0) { error_prepend(errp, "read failed: "); return -EINVAL; } @@ -383,40 +396,36 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, msg = "overall request too short"; goto invalid; } - if (nbd_read(client->ioc, &namelen, sizeof(namelen), errp) < 0) { + if (nbd_opt_read(client, &namelen, sizeof(namelen), errp) < 0) { return -EIO; } be32_to_cpus(&namelen); - client->optlen -= sizeof(namelen); if (namelen > client->optlen - sizeof(requests) || (client->optlen - namelen) % 2) { msg = "name length is incorrect"; goto invalid; } - if (nbd_read(client->ioc, name, namelen, errp) < 0) { + if (nbd_opt_read(client, name, namelen, errp) < 0) { return -EIO; } name[namelen] = '\0'; - client->optlen -= namelen; trace_nbd_negotiate_handle_export_name_request(name); - if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) { + if (nbd_opt_read(client, &requests, sizeof(requests), errp) < 0) { return -EIO; } be16_to_cpus(&requests); - client->optlen -= sizeof(requests); trace_nbd_negotiate_handle_info_requests(requests); if (requests != client->optlen / sizeof(request)) { msg = "incorrect number of requests for overall length"; goto invalid; } while (requests--) { - if (nbd_read(client->ioc, &request, sizeof(request), errp) < 0) { + if (nbd_opt_read(client, &request, sizeof(request), errp) < 0) { return -EIO; } be16_to_cpus(&request); - client->optlen -= sizeof(request); trace_nbd_negotiate_handle_info_request(request, nbd_info_lookup(request)); /* We care about NBD_INFO_NAME and NBD_INFO_BLOCK_SIZE; @@ -521,7 +530,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, return rc; invalid: - if (nbd_drop(client->ioc, client->optlen, errp) < 0) { + if (nbd_opt_drop(client, client->optlen, errp) < 0) { return -EIO; } return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, @@ -715,7 +724,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, return -EINVAL; default: - if (nbd_drop(client->ioc, length, errp) < 0) { + if (nbd_opt_drop(client, length, errp) < 0) { return -EIO; } ret = nbd_negotiate_send_rep_err(client, @@ -791,7 +800,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, break; default: - if (nbd_drop(client->ioc, length, errp) < 0) { + if (nbd_opt_drop(client, length, errp) < 0) { return -EIO; } ret = nbd_negotiate_send_rep_err(client, @@ -821,6 +830,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, if (ret < 0) { return ret; } + assert(client->optlen == 0); } }
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- nbd/server.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)