From a16a7907707e08c09843297a6f707f5bfb8beb22 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 10 Jan 2018 17:08:20 -0600 Subject: [PATCH 1/6] nbd/server: Hoist nbd_reject_length() earlier No semantic change, but will make it easier for an upcoming patch to refactor code without having to add forward declarations. Fix a poor comment while at it. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20180110230825.18321-2-eblake@redhat.com> --- nbd/server.c | 58 ++++++++++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 6cf2eeb2c1..5ba12f3c0b 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016-2017 Red Hat, Inc. + * Copyright (C) 2016-2018 Red Hat, Inc. * Copyright (C) 2005 Anthony Liguori * * Network Block Device Server Side @@ -348,6 +348,34 @@ static int nbd_negotiate_send_info(NBDClient *client, uint32_t opt, return 0; } +/* nbd_reject_length: Handle any unexpected payload. + * @fatal requests that we quit talking to the client, even if we are able + * to successfully send an error reply. + * Return: + * -errno transmission error occurred or @fatal was requested, errp is set + * 0 error message successfully sent to client, errp is not set + */ +static int nbd_reject_length(NBDClient *client, uint32_t length, + uint32_t option, bool fatal, Error **errp) +{ + int ret; + + assert(length); + if (nbd_drop(client->ioc, length, errp) < 0) { + return -EIO; + } + ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID, + option, errp, + "option '%s' should have zero length", + nbd_opt_lookup(option)); + if (fatal && !ret) { + error_setg(errp, "option '%s' should have zero length", + nbd_opt_lookup(option)); + return -EINVAL; + } + return ret; +} + /* Handle NBD_OPT_INFO and NBD_OPT_GO. * Return -errno on error, 0 if ready for next option, and 1 to move * into transmission phase. */ @@ -570,34 +598,6 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client, return QIO_CHANNEL(tioc); } -/* nbd_reject_length: Handle any unexpected payload. - * @fatal requests that we quit talking to the client, even if we are able - * to successfully send an error to the guest. - * Return: - * -errno transmission error occurred or @fatal was requested, errp is set - * 0 error message successfully sent to client, errp is not set - */ -static int nbd_reject_length(NBDClient *client, uint32_t length, - uint32_t option, bool fatal, Error **errp) -{ - int ret; - - assert(length); - if (nbd_drop(client->ioc, length, errp) < 0) { - return -EIO; - } - ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID, - option, errp, - "option '%s' should have zero length", - nbd_opt_lookup(option)); - if (fatal && !ret) { - error_setg(errp, "option '%s' should have zero length", - nbd_opt_lookup(option)); - return -EINVAL; - } - return ret; -} - /* nbd_negotiate_options * Process all NBD_OPT_* client option commands, during fixed newstyle * negotiation. From 0cfae925d2e31a7d7613a6fb60aa58b38ec2b61e Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 10 Jan 2018 17:08:21 -0600 Subject: [PATCH 2/6] nbd/server: refactor negotiation functions parameters Instead of passing currently negotiating option and its length to many of negotiation functions let's just store them on NBDClient struct to be state-variables of negotiation phase. This unifies semantics of negotiation functions and allows tracking changes of remaining option length in future patches. Asssert that optlen is back to 0 after negotiation (including old-style connections which don't negotiate), although we need more patches before we can assert optlen is 0 between options during negotiation. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20171122101958.17065-2-vsementsov@virtuozzo.com> [eblake: rebase, commit message tweak, assert !optlen after negotiation completes] Signed-off-by: Eric Blake --- nbd/server.c | 168 +++++++++++++++++++++++++-------------------------- 1 file changed, 84 insertions(+), 84 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 5ba12f3c0b..b9efdcc605 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -102,9 +102,11 @@ struct NBDClient { bool closing; bool structured_reply; -}; -/* That's all folks */ + uint32_t opt; /* Current option being negotiated */ + uint32_t optlen; /* remaining length of data in ioc for the option being + negotiated now */ +}; static void nbd_client_receive_next_request(NBDClient *client); @@ -137,10 +139,12 @@ static void nbd_client_receive_next_request(NBDClient *client); /* Send a reply header, including length, but no payload. * Return -errno on error, 0 on success. */ -static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type, - uint32_t opt, uint32_t len, Error **errp) +static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type, + uint32_t len, Error **errp) { uint64_t magic; + QIOChannel *ioc = client->ioc; + uint32_t opt = client->opt; trace_nbd_negotiate_send_rep_len(opt, nbd_opt_lookup(opt), type, nbd_rep_lookup(type), len); @@ -174,17 +178,17 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type, /* Send a reply header with default 0 length. * Return -errno on error, 0 on success. */ -static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt, +static int nbd_negotiate_send_rep(NBDClient *client, uint32_t type, Error **errp) { - return nbd_negotiate_send_rep_len(ioc, type, opt, 0, errp); + return nbd_negotiate_send_rep_len(client, type, 0, errp); } /* Send an error reply. * Return -errno on error, 0 on success. */ -static int GCC_FMT_ATTR(5, 6) -nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type, - uint32_t opt, Error **errp, const char *fmt, ...) +static int GCC_FMT_ATTR(4, 5) +nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type, + Error **errp, const char *fmt, ...) { va_list va; char *msg; @@ -197,11 +201,11 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type, len = strlen(msg); assert(len < 4096); trace_nbd_negotiate_send_rep_err(msg); - ret = nbd_negotiate_send_rep_len(ioc, type, opt, len, errp); + ret = nbd_negotiate_send_rep_len(client, type, len, errp); if (ret < 0) { goto out; } - if (nbd_write(ioc, msg, len, errp) < 0) { + if (nbd_write(client->ioc, msg, len, errp) < 0) { error_prepend(errp, "write failed (error message): "); ret = -EIO; } else { @@ -215,21 +219,21 @@ out: /* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload. * Return -errno on error, 0 on success. */ -static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp, +static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp, Error **errp) { size_t name_len, desc_len; uint32_t len; const char *name = exp->name ? exp->name : ""; const char *desc = exp->description ? exp->description : ""; + QIOChannel *ioc = client->ioc; int ret; trace_nbd_negotiate_send_rep_list(name, desc); name_len = strlen(name); desc_len = strlen(desc); len = name_len + desc_len + sizeof(len); - ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len, - errp); + ret = nbd_negotiate_send_rep_len(client, NBD_REP_SERVER, len, errp); if (ret < 0) { return ret; } @@ -258,20 +262,21 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp, static int nbd_negotiate_handle_list(NBDClient *client, Error **errp) { NBDExport *exp; + assert(client->opt == NBD_OPT_LIST); /* For each export, send a NBD_REP_SERVER reply. */ QTAILQ_FOREACH(exp, &exports, next) { - if (nbd_negotiate_send_rep_list(client->ioc, exp, errp)) { + if (nbd_negotiate_send_rep_list(client, exp, errp)) { return -EINVAL; } } /* Finish with a NBD_REP_ACK. */ - return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST, errp); + return nbd_negotiate_send_rep(client, NBD_REP_ACK, errp); } /* Send a reply to NBD_OPT_EXPORT_NAME. * Return -errno on error, 0 on success. */ -static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length, +static int nbd_negotiate_handle_export_name(NBDClient *client, uint16_t myflags, bool no_zeroes, Error **errp) { @@ -288,15 +293,16 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length, [10 .. 133] reserved (0) [unless no_zeroes] */ trace_nbd_negotiate_handle_export_name(); - if (length >= sizeof(name)) { + if (client->optlen >= sizeof(name)) { error_setg(errp, "Bad length received"); return -EINVAL; } - if (nbd_read(client->ioc, name, length, errp) < 0) { + if (nbd_read(client->ioc, name, client->optlen, errp) < 0) { error_prepend(errp, "read failed: "); return -EINVAL; } - name[length] = '\0'; + name[client->optlen] = '\0'; + client->optlen = 0; trace_nbd_negotiate_handle_export_name_request(name); @@ -326,14 +332,14 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length, /* Send a single NBD_REP_INFO, with a buffer @buf of @length bytes. * The buffer does NOT include the info type prefix. * Return -errno on error, 0 if ready to send more. */ -static int nbd_negotiate_send_info(NBDClient *client, uint32_t opt, +static int nbd_negotiate_send_info(NBDClient *client, uint16_t info, uint32_t length, void *buf, Error **errp) { int rc; trace_nbd_negotiate_send_info(info, nbd_info_lookup(info), length); - rc = nbd_negotiate_send_rep_len(client->ioc, NBD_REP_INFO, opt, + rc = nbd_negotiate_send_rep_len(client, NBD_REP_INFO, sizeof(info) + length, errp); if (rc < 0) { return rc; @@ -355,22 +361,20 @@ static int nbd_negotiate_send_info(NBDClient *client, uint32_t opt, * -errno transmission error occurred or @fatal was requested, errp is set * 0 error message successfully sent to client, errp is not set */ -static int nbd_reject_length(NBDClient *client, uint32_t length, - uint32_t option, bool fatal, Error **errp) +static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp) { int ret; - assert(length); - if (nbd_drop(client->ioc, length, errp) < 0) { + assert(client->optlen); + if (nbd_drop(client->ioc, client->optlen, errp) < 0) { return -EIO; } - ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID, - option, errp, + ret = nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, errp, "option '%s' should have zero length", - nbd_opt_lookup(option)); + nbd_opt_lookup(client->opt)); if (fatal && !ret) { error_setg(errp, "option '%s' should have zero length", - nbd_opt_lookup(option)); + nbd_opt_lookup(client->opt)); return -EINVAL; } return ret; @@ -379,8 +383,7 @@ static int nbd_reject_length(NBDClient *client, uint32_t length, /* Handle NBD_OPT_INFO and NBD_OPT_GO. * Return -errno on error, 0 if ready for next option, and 1 to move * into transmission phase. */ -static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, - uint32_t opt, uint16_t myflags, +static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, Error **errp) { int rc; @@ -401,7 +404,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, 2 bytes: N, number of requests (can be 0) N * 2 bytes: N requests */ - if (length < sizeof(namelen) + sizeof(requests)) { + if (client->optlen < sizeof(namelen) + sizeof(requests)) { msg = "overall request too short"; goto invalid; } @@ -409,8 +412,10 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, return -EIO; } be32_to_cpus(&namelen); - length -= sizeof(namelen); - if (namelen > length - sizeof(requests) || (length - namelen) % 2) { + client->optlen -= sizeof(namelen); + if (namelen > client->optlen - sizeof(requests) || + (client->optlen - namelen) % 2) + { msg = "name length is incorrect"; goto invalid; } @@ -422,16 +427,16 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, return -EIO; } name[namelen] = '\0'; - length -= namelen; + client->optlen -= namelen; trace_nbd_negotiate_handle_export_name_request(name); if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) { return -EIO; } be16_to_cpus(&requests); - length -= sizeof(requests); + client->optlen -= sizeof(requests); trace_nbd_negotiate_handle_info_requests(requests); - if (requests != length / sizeof(request)) { + if (requests != client->optlen / sizeof(request)) { msg = "incorrect number of requests for overall length"; goto invalid; } @@ -440,7 +445,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, return -EIO; } be16_to_cpus(&request); - length -= sizeof(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; @@ -455,18 +460,18 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, break; } } - assert(length == 0); + assert(client->optlen == 0); exp = nbd_export_find(name); if (!exp) { - return nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_UNKNOWN, - opt, errp, "export '%s' not present", + return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_UNKNOWN, + errp, "export '%s' not present", name); } /* Don't bother sending NBD_INFO_NAME unless client requested it */ if (sendname) { - rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, namelen, name, + rc = nbd_negotiate_send_info(client, NBD_INFO_NAME, namelen, name, errp); if (rc < 0) { return rc; @@ -478,7 +483,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, if (exp->description) { size_t len = strlen(exp->description); - rc = nbd_negotiate_send_info(client, opt, NBD_INFO_DESCRIPTION, + rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION, len, exp->description, errp); if (rc < 0) { return rc; @@ -490,7 +495,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, * whether this is OPT_INFO or OPT_GO. */ /* minimum - 1 for back-compat, or 512 if client is new enough. * TODO: consult blk_bs(blk)->bl.request_alignment? */ - sizes[0] = (opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1; + sizes[0] = + (client->opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1; /* preferred - Hard-code to 4096 for now. * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */ sizes[1] = 4096; @@ -500,7 +506,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, cpu_to_be32s(&sizes[0]); cpu_to_be32s(&sizes[1]); cpu_to_be32s(&sizes[2]); - rc = nbd_negotiate_send_info(client, opt, NBD_INFO_BLOCK_SIZE, + rc = nbd_negotiate_send_info(client, NBD_INFO_BLOCK_SIZE, sizeof(sizes), sizes, errp); if (rc < 0) { return rc; @@ -511,7 +517,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, exp->nbdflags | myflags); stq_be_p(buf, exp->size); stw_be_p(buf + 8, exp->nbdflags | myflags); - rc = nbd_negotiate_send_info(client, opt, NBD_INFO_EXPORT, + rc = nbd_negotiate_send_info(client, NBD_INFO_EXPORT, sizeof(buf), buf, errp); if (rc < 0) { return rc; @@ -521,21 +527,21 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, * request block sizes, return an error. * TODO: consult blk_bs(blk)->request_align, and only error if it * is not 1? */ - if (opt == NBD_OPT_INFO && !blocksize) { - return nbd_negotiate_send_rep_err(client->ioc, - NBD_REP_ERR_BLOCK_SIZE_REQD, opt, + if (client->opt == NBD_OPT_INFO && !blocksize) { + return nbd_negotiate_send_rep_err(client, + NBD_REP_ERR_BLOCK_SIZE_REQD, errp, "request NBD_INFO_BLOCK_SIZE to " "use this export"); } /* Final reply */ - rc = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, opt, errp); + rc = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp); if (rc < 0) { return rc; } - if (opt == NBD_OPT_GO) { + if (client->opt == NBD_OPT_GO) { client->exp = exp; QTAILQ_INSERT_TAIL(&client->exp->clients, client, next); nbd_export_get(client->exp); @@ -544,10 +550,10 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length, return rc; invalid: - if (nbd_drop(client->ioc, length, errp) < 0) { + if (nbd_drop(client->ioc, client->optlen, errp) < 0) { return -EIO; } - return nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID, opt, + return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, errp, "%s", msg); } @@ -561,11 +567,12 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client, QIOChannelTLS *tioc; struct NBDTLSHandshakeData data = { 0 }; + assert(client->opt == NBD_OPT_STARTTLS); + trace_nbd_negotiate_handle_starttls(); ioc = client->ioc; - if (nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, - NBD_OPT_STARTTLS, errp) < 0) { + if (nbd_negotiate_send_rep(client, NBD_REP_ACK, errp) < 0) { return NULL; } @@ -670,12 +677,14 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, return -EINVAL; } option = be32_to_cpu(option); + client->opt = option; if (nbd_read(client->ioc, &length, sizeof(length), errp) < 0) { error_prepend(errp, "read failed: "); return -EINVAL; } length = be32_to_cpu(length); + client->optlen = length; if (length > NBD_MAX_BUFFER_SIZE) { error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)", @@ -697,8 +706,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, if (length) { /* Unconditionally drop the connection if the client * can't start a TLS negotiation correctly */ - return nbd_reject_length(client, length, option, true, - errp); + return nbd_reject_length(client, true, errp); } tioc = nbd_negotiate_handle_starttls(client, errp); if (!tioc) { @@ -719,9 +727,8 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, if (nbd_drop(client->ioc, length, errp) < 0) { return -EIO; } - ret = nbd_negotiate_send_rep_err(client->ioc, - NBD_REP_ERR_TLS_REQD, - option, errp, + ret = nbd_negotiate_send_rep_err(client, + NBD_REP_ERR_TLS_REQD, errp, "Option 0x%" PRIx32 "not permitted before TLS", option); @@ -737,8 +744,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, switch (option) { case NBD_OPT_LIST: if (length) { - ret = nbd_reject_length(client, length, option, false, - errp); + ret = nbd_reject_length(client, false, errp); } else { ret = nbd_negotiate_handle_list(client, errp); } @@ -748,18 +754,17 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, /* NBD spec says we must try to reply before * disconnecting, but that we must also tolerate * guests that don't wait for our reply. */ - nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, option, NULL); + nbd_negotiate_send_rep(client, NBD_REP_ACK, NULL); return 1; case NBD_OPT_EXPORT_NAME: - return nbd_negotiate_handle_export_name(client, length, + return nbd_negotiate_handle_export_name(client, myflags, no_zeroes, errp); case NBD_OPT_INFO: case NBD_OPT_GO: - ret = nbd_negotiate_handle_info(client, length, option, - myflags, errp); + ret = nbd_negotiate_handle_info(client, myflags, errp); if (ret == 1) { assert(option == NBD_OPT_GO); return 0; @@ -768,32 +773,27 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, case NBD_OPT_STARTTLS: if (length) { - ret = nbd_reject_length(client, length, option, false, - errp); + ret = nbd_reject_length(client, false, errp); } else if (client->tlscreds) { - ret = nbd_negotiate_send_rep_err(client->ioc, - NBD_REP_ERR_INVALID, - option, errp, + ret = nbd_negotiate_send_rep_err(client, + NBD_REP_ERR_INVALID, errp, "TLS already enabled"); } else { - ret = nbd_negotiate_send_rep_err(client->ioc, - NBD_REP_ERR_POLICY, - option, errp, + ret = nbd_negotiate_send_rep_err(client, + NBD_REP_ERR_POLICY, errp, "TLS not configured"); } break; case NBD_OPT_STRUCTURED_REPLY: if (length) { - ret = nbd_reject_length(client, length, option, false, - errp); + ret = nbd_reject_length(client, false, errp); } else if (client->structured_reply) { ret = nbd_negotiate_send_rep_err( - client->ioc, NBD_REP_ERR_INVALID, option, errp, + client, NBD_REP_ERR_INVALID, errp, "structured reply already negotiated"); } else { - ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, - option, errp); + ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp); client->structured_reply = true; myflags |= NBD_FLAG_SEND_DF; } @@ -803,9 +803,8 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, if (nbd_drop(client->ioc, length, errp) < 0) { return -EIO; } - ret = nbd_negotiate_send_rep_err(client->ioc, - NBD_REP_ERR_UNSUP, - option, errp, + ret = nbd_negotiate_send_rep_err(client, + NBD_REP_ERR_UNSUP, errp, "Unsupported option 0x%" PRIx32 " (%s)", option, nbd_opt_lookup(option)); @@ -818,7 +817,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, */ switch (option) { case NBD_OPT_EXPORT_NAME: - return nbd_negotiate_handle_export_name(client, length, + return nbd_negotiate_handle_export_name(client, myflags, no_zeroes, errp); @@ -898,6 +897,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp) } } + assert(!client->optlen); trace_nbd_negotiate_success(); return 0; From 32f158a63509656f95922ce28d1b4a4e06a2cbf2 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 10 Jan 2018 17:08:22 -0600 Subject: [PATCH 3/6] nbd/server: Better error for NBD_OPT_EXPORT_NAME failure When a client abruptly disconnects before we've finished reading the name sent with NBD_OPT_EXPORT_NAME, we are better off logging the failure as EIO (we can't communicate with the client), rather than EINVAL (the client sent bogus data). Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20180110230825.18321-4-eblake@redhat.com> --- nbd/server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nbd/server.c b/nbd/server.c index b9efdcc605..31c1d32429 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -299,7 +299,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, } if (nbd_read(client->ioc, name, client->optlen, errp) < 0) { error_prepend(errp, "read failed: "); - return -EINVAL; + return -EIO; } name[client->optlen] = '\0'; client->optlen = 0; From 41f5dfafbb158d2bed0b6abcb7d16b77c56ea5a1 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 10 Jan 2018 17:08:23 -0600 Subject: [PATCH 4/6] nbd/server: Add va_list form of nbd_negotiate_send_rep_err() This will be useful for the next patch. Based on a patch by Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20180110230825.18321-5-eblake@redhat.com> --- nbd/server.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 31c1d32429..04da1c2f1e 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -186,18 +186,15 @@ static int nbd_negotiate_send_rep(NBDClient *client, uint32_t type, /* Send an error reply. * Return -errno on error, 0 on success. */ -static int GCC_FMT_ATTR(4, 5) -nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type, - Error **errp, const char *fmt, ...) +static int GCC_FMT_ATTR(4, 0) +nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type, + Error **errp, const char *fmt, va_list va) { - va_list va; char *msg; int ret; size_t len; - va_start(va, fmt); msg = g_strdup_vprintf(fmt, va); - va_end(va); len = strlen(msg); assert(len < 4096); trace_nbd_negotiate_send_rep_err(msg); @@ -217,6 +214,21 @@ out: return ret; } +/* Send an error reply. + * Return -errno on error, 0 on success. */ +static int GCC_FMT_ATTR(4, 5) +nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type, + Error **errp, const char *fmt, ...) +{ + va_list va; + int ret; + + va_start(va, fmt); + ret = nbd_negotiate_send_rep_verr(client, type, errp, fmt, va); + va_end(va); + return ret; +} + /* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload. * Return -errno on error, 0 on success. */ static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp, From 894e02804c862c6940b43a0a488164655d3fb3f0 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 10 Jan 2018 17:08:24 -0600 Subject: [PATCH 5/6] nbd/server: Add helper functions for parsing option payload Rather than making every callsite perform length sanity checks and error reporting, add the helper functions nbd_opt_read() and nbd_opt_drop() that use the length stored in the client struct; also add an assertion that optlen is 0 before any option (ie. any previous option was fully handled), complementing the assertion added in an earlier patch that optlen is 0 after all negotiation completes. Note that the call in nbd_negotiate_handle_export_name() does not use the new helper (in part because the server cannot reply to NBD_OPT_EXPORT_NAME - it either succeeds or the connection drops). Based on patches by Vladimir Sementsov-Ogievskiy. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20180110230825.18321-6-eblake@redhat.com> --- nbd/server.c | 122 ++++++++++++++++++++++++++------------------------- 1 file changed, 62 insertions(+), 60 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 04da1c2f1e..78b08f5891 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -229,6 +229,40 @@ nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type, return ret; } +/* Drop remainder of the current option, and send a reply with the + * given error type and message. Return -errno on read or write + * failure; or 0 if connection is still live. */ +static int GCC_FMT_ATTR(4, 5) +nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp, + const char *fmt, ...) +{ + int ret = nbd_drop(client->ioc, client->optlen, errp); + va_list va; + + client->optlen = 0; + if (!ret) { + va_start(va, fmt); + ret = nbd_negotiate_send_rep_verr(client, type, errp, fmt, va); + va_end(va); + } + return ret; +} + +/* Read size bytes from the unparsed payload of the current option. + * Return -errno on I/O error, 0 if option was completely handled by + * sending a reply about inconsistent lengths, or 1 on success. */ +static int nbd_opt_read(NBDClient *client, void *buffer, size_t size, + Error **errp) +{ + if (size > client->optlen) { + return nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp, + "Inconsistent lengths in option %s", + nbd_opt_lookup(client->opt)); + } + client->optlen -= size; + return qio_channel_read_all(client->ioc, buffer, size, errp) < 0 ? -EIO : 1; +} + /* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload. * Return -errno on error, 0 on success. */ static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp, @@ -378,14 +412,11 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp) int ret; assert(client->optlen); - if (nbd_drop(client->ioc, client->optlen, errp) < 0) { - return -EIO; - } - ret = nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, errp, - "option '%s' should have zero length", - nbd_opt_lookup(client->opt)); + ret = nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp, + "option '%s' has unexpected length", + nbd_opt_lookup(client->opt)); if (fatal && !ret) { - error_setg(errp, "option '%s' should have zero length", + error_setg(errp, "option '%s' has unexpected length", nbd_opt_lookup(client->opt)); return -EINVAL; } @@ -408,7 +439,6 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, bool blocksize = false; uint32_t sizes[3]; char buf[sizeof(uint64_t) + sizeof(uint16_t)]; - const char *msg; /* Client sends: 4 bytes: L, name length (can be 0) @@ -416,48 +446,34 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, 2 bytes: N, number of requests (can be 0) N * 2 bytes: N requests */ - if (client->optlen < sizeof(namelen) + sizeof(requests)) { - msg = "overall request too short"; - goto invalid; - } - if (nbd_read(client->ioc, &namelen, sizeof(namelen), errp) < 0) { - return -EIO; + rc = nbd_opt_read(client, &namelen, sizeof(namelen), errp); + if (rc <= 0) { + return rc; } 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 (namelen >= sizeof(name)) { - msg = "name too long for qemu"; - goto invalid; + return nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp, + "name too long for qemu"); } - if (nbd_read(client->ioc, name, namelen, errp) < 0) { - return -EIO; + rc = nbd_opt_read(client, name, namelen, errp); + if (rc <= 0) { + return rc; } name[namelen] = '\0'; - client->optlen -= namelen; trace_nbd_negotiate_handle_export_name_request(name); - if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) { - return -EIO; + rc = nbd_opt_read(client, &requests, sizeof(requests), errp); + if (rc <= 0) { + return rc; } 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) { - return -EIO; + rc = nbd_opt_read(client, &request, sizeof(request), errp); + if (rc <= 0) { + return rc; } 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; @@ -472,7 +488,9 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, break; } } - assert(client->optlen == 0); + if (client->optlen) { + return nbd_reject_length(client, false, errp); + } exp = nbd_export_find(name); if (!exp) { @@ -560,13 +578,6 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, rc = 1; } return rc; - - invalid: - if (nbd_drop(client->ioc, client->optlen, errp) < 0) { - return -EIO; - } - return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, - errp, "%s", msg); } @@ -696,6 +707,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, return -EINVAL; } length = be32_to_cpu(length); + assert(!client->optlen); client->optlen = length; if (length > NBD_MAX_BUFFER_SIZE) { @@ -736,14 +748,9 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, return -EINVAL; default: - if (nbd_drop(client->ioc, length, errp) < 0) { - return -EIO; - } - ret = nbd_negotiate_send_rep_err(client, - NBD_REP_ERR_TLS_REQD, errp, - "Option 0x%" PRIx32 - "not permitted before TLS", - option); + ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp, + "Option 0x%" PRIx32 + "not permitted before TLS", option); /* Let the client keep trying, unless they asked to * quit. In this mode, we've already sent an error, so * we can't ack the abort. */ @@ -812,14 +819,9 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, break; default: - if (nbd_drop(client->ioc, length, errp) < 0) { - return -EIO; - } - ret = nbd_negotiate_send_rep_err(client, - NBD_REP_ERR_UNSUP, errp, - "Unsupported option 0x%" - PRIx32 " (%s)", option, - nbd_opt_lookup(option)); + ret = nbd_opt_drop(client, NBD_REP_ERR_UNSUP, errp, + "Unsupported option 0x%" PRIx32 " (%s)", + option, nbd_opt_lookup(option)); break; } } else { From 1d17922a28f5b9a8adcea3c93f49086e0a751e86 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 10 Jan 2018 17:08:25 -0600 Subject: [PATCH 6/6] nbd/server: structurize option reply sending Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20171122101958.17065-6-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- nbd/server.c | 40 +++++++++++++--------------------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 78b08f5891..6caa8d17be 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -137,43 +137,29 @@ static void nbd_client_receive_next_request(NBDClient *client); */ +static inline void set_be_option_rep(NBDOptionReply *rep, uint32_t option, + uint32_t type, uint32_t length) +{ + stq_be_p(&rep->magic, NBD_REP_MAGIC); + stl_be_p(&rep->option, option); + stl_be_p(&rep->type, type); + stl_be_p(&rep->length, length); +} + /* 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, uint32_t len, Error **errp) { - uint64_t magic; - QIOChannel *ioc = client->ioc; - uint32_t opt = client->opt; + NBDOptionReply rep; - trace_nbd_negotiate_send_rep_len(opt, nbd_opt_lookup(opt), + trace_nbd_negotiate_send_rep_len(client->opt, nbd_opt_lookup(client->opt), type, nbd_rep_lookup(type), len); assert(len < NBD_MAX_BUFFER_SIZE); - magic = cpu_to_be64(NBD_REP_MAGIC); - if (nbd_write(ioc, &magic, sizeof(magic), errp) < 0) { - error_prepend(errp, "write failed (rep magic): "); - return -EINVAL; - } - opt = cpu_to_be32(opt); - if (nbd_write(ioc, &opt, sizeof(opt), errp) < 0) { - error_prepend(errp, "write failed (rep opt): "); - return -EINVAL; - } - - type = cpu_to_be32(type); - if (nbd_write(ioc, &type, sizeof(type), errp) < 0) { - error_prepend(errp, "write failed (rep type): "); - return -EINVAL; - } - - len = cpu_to_be32(len); - if (nbd_write(ioc, &len, sizeof(len), errp) < 0) { - error_prepend(errp, "write failed (rep data length): "); - return -EINVAL; - } - return 0; + set_be_option_rep(&rep, client->opt, type, len); + return nbd_write(client->ioc, &rep, sizeof(rep), errp); } /* Send a reply header with default 0 length.