Commit f141228d introduced a mapping from errno values to NBD protocol error numbers, to match the fact that non-Linux systems do not necessarily share the same errno values. But we failed to honor that mapping when we added structured replies. Fixes: eaa4c6e9 Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/protocol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/protocol.c b/server/protocol.c index 3d5668f..212bed0 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -591,7 +591,7 @@ send_structured_reply_error (struct connection *conn, } /* Send the error. */ - error_data.error = htobe32 (error); + error_data.error = htobe32 (nbd_errno (error)); error_data.len = htobe16 (0); r = conn->send (conn, &error_data, sizeof error_data); if (r == -1) { -- 2.20.1

Previously, we were squashing EOVERFLOW into EINVAL; continue to do so at points in the protocol where the client may not be expecting EOVERFLOW. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/protocol.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/server/protocol.c b/server/protocol.c index a52bb56..0a9f73c 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -326,7 +326,7 @@ skip_over_write_buffer (int sock, size_t count) /* Convert a system errno to an NBD_E* error code. */ static int -nbd_errno (int error) +nbd_errno (int error, bool flag_df) { switch (error) { case 0: @@ -349,7 +349,9 @@ nbd_errno (int error) return NBD_ESHUTDOWN; #endif case EOVERFLOW: - return NBD_EOVERFLOW; + if (flag_df) + return NBD_EOVERFLOW; + /* fallthrough */ case EINVAL: default: return NBD_EINVAL; @@ -368,7 +370,7 @@ send_simple_reply (struct connection *conn, reply.magic = htobe32 (NBD_SIMPLE_REPLY_MAGIC); reply.handle = handle; - reply.error = htobe32 (nbd_errno (error)); + reply.error = htobe32 (nbd_errno (error, false)); r = conn->send (conn, &reply, sizeof reply); if (r == -1) { @@ -573,7 +575,8 @@ send_structured_reply_block_status (struct connection *conn, static int send_structured_reply_error (struct connection *conn, - uint64_t handle, uint16_t cmd, uint32_t error) + uint64_t handle, uint16_t cmd, uint16_t flags, + uint32_t error) { ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); struct structured_reply reply; @@ -593,7 +596,7 @@ send_structured_reply_error (struct connection *conn, } /* Send the error. */ - error_data.error = htobe32 (nbd_errno (error)); + error_data.error = htobe32 (nbd_errno (error, flags & NBD_CMD_FLAG_DF)); error_data.len = htobe16 (0); r = conn->send (conn, &error_data, sizeof error_data); if (r == -1) { @@ -737,7 +740,8 @@ protocol_recv_request_send_reply (struct connection *conn) extents); } else - return send_structured_reply_error (conn, request.handle, cmd, error); + return send_structured_reply_error (conn, request.handle, cmd, flags, + error); } else return send_simple_reply (conn, request.handle, cmd, buf, count, error); -- 2.20.1

In the nbd plugin, I forgot to swap the server's global flags into host endian order before checking which flags the server had set. As a result, on little-endian machines, we were always replying with cflags of 0 instead of the intended mirroring of the supported server flags, although fortunately it made no real difference up to now (we were never sending any option other than NBD_OPT_EXPORT_NAME, so not mirroring NBD_FLAG_FIXED_NEWSTYLE had no effect; not mirroring NBD_FLAG_NO_ZEROES merely means a longer handshake; and the NBD protocol does not yet define flag 0x100 or 0x200 to confuse us). However, once we add code for NBD_OPT_GO, it becomes essential to get it right. Fixes: 0e8e8eb1 Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- plugins/nbd/nbd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 72e833c..14e6806 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -512,6 +512,7 @@ nbd_open (int readonly) nbdkit_error ("unable to read global flags: %m"); goto err; } + gflags = be16toh (gflags); cflags = htobe32(gflags & (NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES)); if (write_full (h->fd, &cflags, sizeof cflags)) { nbdkit_error ("unable to return global flags: %m"); -- 2.20.1

The NBD spec was recently patched (nbd.git commit 7827f3ae and friends) to require NBD_OPT_GO for baseline interoperability, with the aim of fewer servers and clients falling back to NBD_OPT_EXPORT_NAME. And since nbdkit as server recently started supporting NBD_OPT_GO (commit f7dd9799), our nbd client as plugin should take advantage of it. This patch is a prerequisite to teaching the nbd plugin about structured replies, which in turn will allow an implementation of .extents. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- plugins/nbd/nbd.c | 201 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 183 insertions(+), 18 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 14e6806..313bf59 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2017-2018 Red Hat Inc. + * Copyright (C) 2017-2019 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -453,6 +453,158 @@ nbd_reply (struct handle *h, int fd) return err ? -1 : 0; } +static int +nbd_newstyle_recv_option_reply (struct handle *h, uint32_t option, + struct fixed_new_option_reply *reply, + char **payload) +{ + char *buffer; + + if (payload) + *payload = NULL; + if (read_full (h->fd, reply, sizeof *reply)) { + nbdkit_error ("unable to read option reply: %m"); + return -1; + } + reply->magic = be64toh (reply->magic); + reply->option = be32toh (reply->option); + reply->reply = be32toh (reply->reply); + reply->replylen = be32toh (reply->replylen); + if (reply->magic != NBD_REP_MAGIC || reply->option != option) { + nbdkit_error ("unexpected option reply"); + return -1; + } + if (reply->replylen) { + if (reply->reply == NBD_REP_ACK) { + nbdkit_error ("NBD_REP_ACK should not have replylen %" PRId32, + reply->replylen); + return -1; + } + if (reply->replylen > 16 * 1024 * 1024) { + nbdkit_error ("option reply length is suspiciously large: %" PRId32, + reply->replylen); + return -1; + } + /* buffer is a string for NBD_REP_ERR_*; adding a NUL terminator + makes that string easier to use, without hurting other reply + types where buffer is not a string */ + buffer = malloc (reply->replylen + 1); + if (!buffer) { + nbdkit_error ("malloc: %m"); + return -1; + } + if (read_full (h->fd, buffer, reply->replylen)) { + nbdkit_error ("unable to read option reply payload: %m"); + free (buffer); + return -1; + } + buffer[reply->replylen] = '\0'; + if (!payload) { + nbdkit_debug ("ignoring option reply payload"); + free (buffer); + } + else + *payload = buffer; + } + return 0; +} + +/* Attempt to negotiate structured reads, block status, and NBD_OPT_GO. + Return 1 if haggling completed, 0 if haggling failed but + NBD_OPT_EXPORT_NAME is still viable, or -1 on inability to connect. */ +static int +nbd_newstyle_haggle (struct handle *h) +{ + struct new_option opt; + uint32_t exportnamelen = htobe32 (strlen (export)); + /* For now, we make no NBD_INFO_* requests, relying on the server to + send its defaults. TODO: nbdkit should let plugins report block + sizes, at which point we should request NBD_INFO_BLOCK_SIZE and + obey any sizes set by server. */ + uint16_t nrinfos = htobe16 (0); + struct fixed_new_option_reply reply; + struct fixed_new_option_reply_info_export reply_export; + char *buffer; + uint16_t info; + + /* TODO: structured reads, block status */ + + /* Try NBD_OPT_GO */ + nbdkit_debug ("trying NBD_OPT_GO"); + opt.version = htobe64 (NEW_VERSION); + opt.option = htobe32 (NBD_OPT_GO); + opt.optlen = htobe32 (sizeof exportnamelen + strlen (export) + + sizeof nrinfos); + if (write_full (h->fd, &opt, sizeof opt) || + write_full (h->fd, &exportnamelen, sizeof exportnamelen) || + write_full (h->fd, export, strlen (export)) || + write_full (h->fd, &nrinfos, sizeof nrinfos)) { + nbdkit_error ("unable to request NBD_OPT_GO: %m"); + return -1; + } + while (1) { + if (nbd_newstyle_recv_option_reply (h, NBD_OPT_GO, &reply, &buffer) < 0) + return -1; + switch (reply.reply) { + case NBD_REP_INFO: + /* Parse payload, but ignore all except NBD_INFO_EXPORT */ + if (reply.replylen < 2) { + nbdkit_error ("NBD_REP_INFO reply too short"); + free (buffer); + return -1; + } + memcpy (&info, buffer, sizeof info); + info = be16toh (info); + switch (info) { + case NBD_INFO_EXPORT: + if (reply.replylen != sizeof reply_export) { + nbdkit_error ("NBD_INFO_EXPORT reply wrong size"); + free (buffer); + return -1; + } + memcpy (&reply_export, buffer, sizeof reply_export); + h->size = be64toh (reply_export.exportsize); + h->flags = be16toh (reply_export.eflags); + break; + default: + nbdkit_debug ("ignoring server info %d", info); + } + free (buffer); + break; + case NBD_REP_ACK: + /* End of replies, valid if server already sent NBD_INFO_EXPORT, + which is observable since h->flags will be set */ + assert (!buffer); + if (!h->flags) { + nbdkit_error ("server omitted NBD_INFO_EXPORT reply to NBD_OPT_GO"); + return -1; + } + nbdkit_debug ("NBD_OPT_GO complete"); + return 1; + case NBD_REP_ERR_UNSUP: + /* Special case this failure to fall back to NBD_OPT_EXPORT_NAME */ + nbdkit_debug ("server lacks NBD_OPT_GO support"); + free (buffer); + return 0; + default: + /* Unexpected. Either the server sent a legitimate error or an + unexpected reply, but either way, we can't connect. */ + if (NBD_REP_IS_ERR (reply.reply)) + if (reply.replylen) + nbdkit_error ("server rejected NBD_OPT_GO with %s: %s", + name_of_nbd_rep (reply.reply), buffer); + else + nbdkit_error ("server rejected NBD_OPT_GO with %s", + name_of_nbd_rep (reply.reply)); + else + nbdkit_error ("server used unexpected reply %s to NBD_OPT_GO", + name_of_nbd_rep (reply.reply)); + free (buffer); + return -1; + } + } +} + /* Create the per-connection handle. */ static void * nbd_open (int readonly) @@ -492,6 +644,7 @@ nbd_open (int readonly) } version = be64toh (old.version); if (version == OLD_VERSION) { + nbdkit_debug ("trying oldstyle connection"); if (read_full (h->fd, (char *) &old + offsetof (struct old_handshake, exportsize), sizeof old - offsetof (struct old_handshake, exportsize))) { @@ -508,6 +661,7 @@ nbd_open (int readonly) struct new_handshake_finish finish; size_t expect; + nbdkit_debug ("trying newstyle connection"); if (read_full (h->fd, &gflags, sizeof gflags)) { nbdkit_error ("unable to read global flags: %m"); goto err; @@ -519,25 +673,36 @@ nbd_open (int readonly) goto err; } - /* For now, we don't do any option haggling, but go straight into - transmission phase */ - opt.version = htobe64 (NEW_VERSION); - opt.option = htobe32 (NBD_OPT_EXPORT_NAME); - opt.optlen = htobe32 (strlen (export)); - if (write_full (h->fd, &opt, sizeof opt) || - write_full (h->fd, export, strlen (export))) { - nbdkit_error ("unable to request export '%s': %m", export); - goto err; + /* Prefer NBD_OPT_GO if possible */ + if (gflags & NBD_FLAG_FIXED_NEWSTYLE) { + int rc = nbd_newstyle_haggle (h); + if (rc < 0) + goto err; + if (!rc) + goto export_name; } - expect = sizeof finish; - if (gflags & NBD_FLAG_NO_ZEROES) - expect -= sizeof finish.zeroes; - if (read_full (h->fd, &finish, expect)) { - nbdkit_error ("unable to read new handshake: %m"); - goto err; + else { + export_name: + /* Option haggling untried or failed, use older NBD_OPT_EXPORT_NAME */ + nbdkit_debug ("trying NBD_OPT_EXPORT_NAME"); + opt.version = htobe64 (NEW_VERSION); + opt.option = htobe32 (NBD_OPT_EXPORT_NAME); + opt.optlen = htobe32 (strlen (export)); + if (write_full (h->fd, &opt, sizeof opt) || + write_full (h->fd, export, strlen (export))) { + nbdkit_error ("unable to request export '%s': %m", export); + goto err; + } + expect = sizeof finish; + if (gflags & NBD_FLAG_NO_ZEROES) + expect -= sizeof finish.zeroes; + if (read_full (h->fd, &finish, expect)) { + nbdkit_error ("unable to read new handshake: %m"); + goto err; + } + h->size = be64toh (finish.exportsize); + h->flags = be16toh (finish.eflags); } - h->size = be64toh (finish.exportsize); - h->flags = be16toh (finish.eflags); } else { nbdkit_error ("unexpected version %#" PRIx64, version); -- 2.20.1

Time to enhance the nbd plugin to request structured replies from the server. For now, deal only with structured reads. The server can now return sparse reads, even though we need nbdkit version 3 before we can in turn return sparse reads back to the client. In general, we have to assume the server is malicious, and so we must sanity check that it sends replies we expect. Thus, we have a choice between either implementing bookkeeping to ensure that the server sends exactly as many non-overlapping chunks as needed to cover the entire read request, or else ensuring that even when the server cheats, we do not leak uninitialized memory back to our client. I chose for simplicity, with the result that I end up calling memset() more frequently than necessary for a compliant server, rather than worrying about bookkeeping to detect a non-compliant server that is attempting to cause an information leak. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- plugins/nbd/nbd.c | 232 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 212 insertions(+), 20 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 313bf59..f729051 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -127,7 +127,9 @@ struct transaction { int fds[2]; } u; void *buf; + uint64_t offset; uint32_t count; + uint32_t err; struct transaction *next; }; @@ -137,6 +139,7 @@ struct handle { int fd; int flags; int64_t size; + bool structured; pthread_t reader; /* Prevents concurrent threads from interleaving writes to server */ @@ -230,9 +233,10 @@ nbd_mark_dead (struct handle *h) return -1; } -/* Find and remove the transaction corresponding to cookie from the list. */ +/* Find and possibly remove the transaction corresponding to cookie + from the list. */ static struct transaction * -find_trans_by_cookie (struct handle *h, uint64_t cookie) +find_trans_by_cookie (struct handle *h, uint64_t cookie, bool remove) { struct transaction **ptr; struct transaction *trans; @@ -244,7 +248,7 @@ find_trans_by_cookie (struct handle *h, uint64_t cookie) break; ptr = &trans->next; } - if (trans) + if (trans && remove) *ptr = trans->next; nbd_unlock (h); return trans; @@ -303,6 +307,7 @@ nbd_request_full (struct handle *h, uint16_t flags, uint16_t type, } trans->buf = rep_buf; trans->count = rep_buf ? count : 0; + trans->offset = offset; nbd_lock (h); if (h->dead) { nbd_unlock (h); @@ -315,7 +320,7 @@ nbd_request_full (struct handle *h, uint16_t flags, uint16_t type, nbd_unlock (h); if (nbd_request_raw (h, flags, type, offset, count, cookie, req_buf) == 0) return fd; - trans = find_trans_by_cookie (h, cookie); + trans = find_trans_by_cookie (h, cookie, true); err: err = errno; @@ -340,35 +345,196 @@ nbd_request (struct handle *h, uint16_t flags, uint16_t type, uint64_t offset, /* Read a reply, and look up the fd corresponding to the transaction. Return the server's non-negative answer (converted to local errno - value) on success, or -1 on read failure. */ + value) on success, or -1 on read failure. If structured replies + were negotiated, fd is set to -1 if there are still more replies + expected. */ static int nbd_reply_raw (struct handle *h, int *fd) { - struct simple_reply rep; + union { + struct simple_reply simple; + struct structured_reply structured; + } rep; struct transaction *trans; - void *buf; + void *buf = NULL; uint32_t count; + int error = NBD_SUCCESS; + bool more = false; + uint64_t offset = 0; /* absolute offset of structured read chunk from buf */ + uint32_t len = 0; /* 0 except for structured reads */ + bool zero = false; /* Whether to read or memset on structured read */ *fd = -1; - if (read_full (h->fd, &rep, sizeof rep) < 0) + /* magic and handle overlap between simple and structured replies */ + if (read_full (h->fd, &rep, sizeof rep.simple)) return nbd_mark_dead (h); - if (be32toh (rep.magic) != NBD_SIMPLE_REPLY_MAGIC) + switch (be32toh (rep.simple.magic)) { + case NBD_SIMPLE_REPLY_MAGIC: + nbdkit_debug ("received simple reply for cookie %#" PRIx64 ", status %s", + rep.simple.handle, + name_of_nbd_error(be32toh (rep.simple.error))); + error = be32toh (rep.simple.error); + break; + case NBD_STRUCTURED_REPLY_MAGIC: + if (!h->structured) { + nbdkit_error ("structured response without negotiation"); + return nbd_mark_dead (h); + } + if (read_full (h->fd, sizeof rep.simple + (char *) &rep, + sizeof rep - sizeof rep.simple)) + return nbd_mark_dead (h); + rep.structured.flags = be16toh (rep.structured.flags); + rep.structured.type = be16toh (rep.structured.type); + rep.structured.length = be32toh (rep.structured.length); + nbdkit_debug ("received structured reply %s for cookie %#" PRIx64 + ", payload length %" PRId32, + name_of_nbd_reply_type(rep.structured.type), + rep.structured.handle, rep.structured.length); + if (rep.structured.length > 64 * 1024 * 1024) { + nbdkit_error ("structured reply length is suspiciously large: %" PRId32, + rep.structured.length); + return nbd_mark_dead (h); + } + if (rep.structured.length) { + /* Special case for OFFSET_DATA in order to read tail of chunk + directly into final buffer later on */ + len = (rep.structured.type == NBD_REPLY_TYPE_OFFSET_DATA && + rep.structured.length > sizeof offset) ? sizeof offset : + rep.structured.length; + buf = malloc (len); + if (!buf) { + nbdkit_error ("reading structured reply payload: %m"); + return nbd_mark_dead (h); + } + if (read_full (h->fd, buf, len)) { + free (buf); + return nbd_mark_dead (h); + } + len = 0; + } + more = !(rep.structured.flags & NBD_REPLY_FLAG_DONE); + switch (rep.structured.type) { + case NBD_REPLY_TYPE_NONE: + if (rep.structured.length) { + nbdkit_error ("NBD_REPLY_TYPE_NONE with invalid payload"); + free (buf); + return nbd_mark_dead (h); + } + if (more) { + nbdkit_error ("NBD_REPLY_TYPE_NONE without done flag"); + return nbd_mark_dead (h); + } + break; + case NBD_REPLY_TYPE_OFFSET_DATA: + if (rep.structured.length <= sizeof offset) { + nbdkit_error ("structured reply OFFSET_DATA too small"); + free (buf); + return nbd_mark_dead (h); + } + memcpy (&offset, buf, sizeof offset); + offset = be64toh (offset); + len = rep.structured.length - sizeof offset; + break; + case NBD_REPLY_TYPE_OFFSET_HOLE: + if (rep.structured.length != sizeof offset + sizeof len) { + nbdkit_error ("structured reply OFFSET_HOLE size incorrect"); + free (buf); + return nbd_mark_dead (h); + } + memcpy (&offset, buf, sizeof offset); + offset = be64toh (offset); + memcpy (&len, buf, sizeof len); + len = be32toh (len); + if (!len) { + nbdkit_error ("structured reply OFFSET_HOLE length incorrect"); + free (buf); + return nbd_mark_dead (h); + } + zero = true; + break; + default: + if (NBD_REPLY_TYPE_IS_ERR (rep.structured.type)) { + uint16_t errlen; + + if (rep.structured.length < sizeof error + sizeof errlen) { + nbdkit_error ("structured reply error size incorrect"); + free (buf); + return nbd_mark_dead (h); + } + memcpy (&errlen, (char *) buf + sizeof error, sizeof errlen); + errlen = be16toh (errlen); + if (errlen > rep.structured.length - sizeof error - sizeof errlen) { + nbdkit_error ("structured reply error message size incorrect"); + free (buf); + return nbd_mark_dead (h); + } + memcpy (&error, buf, sizeof error); + error = be32toh (error); + if (errlen) + nbdkit_debug ("received structured error %s with message: %.*s", + name_of_nbd_error (error), errlen, + (char *) buf + sizeof error + sizeof errlen); + else + nbdkit_debug ("received structured error %s without message", + name_of_nbd_error (error)); + free (buf); + } + else { + nbdkit_error ("received unexpected structured reply %s", + name_of_nbd_reply_type(rep.structured.type)); + free (buf); + return nbd_mark_dead (h); + } + } + break; + default: + nbdkit_error ("received unexpected magic in reply: %#" PRIx32, + rep.simple.magic); return nbd_mark_dead (h); - nbdkit_debug ("received reply for cookie %#" PRIx64 ", status %s", - rep.handle, name_of_nbd_error(be32toh (rep.error))); - trans = find_trans_by_cookie (h, rep.handle); + } + + trans = find_trans_by_cookie (h, rep.simple.handle, !more); if (!trans) { - nbdkit_error ("reply with unexpected cookie %#" PRIx64, rep.handle); + nbdkit_error ("reply with unexpected cookie %#" PRIx64, rep.simple.handle); return nbd_mark_dead (h); } - *fd = trans->u.fds[1]; + if (!more) + *fd = trans->u.fds[1]; + else if (error && !trans->err) + trans->err = error; buf = trans->buf; count = trans->count; + if (buf && h->structured && + be32toh (rep.simple.magic) == NBD_SIMPLE_REPLY_MAGIC) { + nbdkit_error ("simple read reply when structured was expected"); + return nbd_mark_dead (h); + } + if (len) { + if (!buf) { + nbdkit_error ("structured read response to a non-read command"); + return nbd_mark_dead (h); + } + if (offset < trans->offset || offset > INT64_MAX || + offset + len > trans->offset + count) { + nbdkit_error ("structured read reply with unexpected offset/length"); + return nbd_mark_dead (h); + } + buf = (char *) buf + offset - trans->offset; + if (zero) { + /* We do not check whether the server mistakenly sends + non-compliant overlapping chunks, thus we must re-zero holes + rather than relying on our pre-initialization to zero */ + memset (buf, 0, len); + buf = NULL; + } + else + count = len; + } free (trans); - switch (be32toh (rep.error)) { + switch (error) { case NBD_SUCCESS: - if (buf && read_full (h->fd, buf, count) < 0) + if (buf && read_full (h->fd, buf, count)) return nbd_mark_dead (h); return 0; case NBD_EPERM: @@ -378,8 +544,7 @@ nbd_reply_raw (struct handle *h, int *fd) case NBD_ENOMEM: return ENOMEM; default: - nbdkit_debug ("unexpected error %d, squashing to EINVAL", - be32toh (rep.error)); + nbdkit_debug ("unexpected error %d, squashing to EINVAL", error); /* fallthrough */ case NBD_EINVAL: return EINVAL; @@ -405,7 +570,9 @@ nbd_reader (void *handle) r = nbd_reply_raw (h, &fd); if (r >= 0) { - if (write (fd, &r, sizeof r) != sizeof r) { + if (fd < 0) + nbdkit_debug ("partial reply handled, waiting for final reply"); + else if (write (fd, &r, sizeof r) != sizeof r) { nbdkit_error ("failed to write pipe: %m"); abort (); } @@ -527,7 +694,25 @@ nbd_newstyle_haggle (struct handle *h) char *buffer; uint16_t info; - /* TODO: structured reads, block status */ + nbdkit_debug ("trying NBD_OPT_STRUCTURED_REPLY"); + opt.version = htobe64 (NEW_VERSION); + opt.option = htobe32 (NBD_OPT_STRUCTURED_REPLY); + opt.optlen = htobe32 (0); + if (write_full (h->fd, &opt, sizeof opt)) { + nbdkit_error ("unable to request NBD_OPT_STRUCTURED_REPLY: %m"); + return -1; + } + if (nbd_newstyle_recv_option_reply (h, NBD_OPT_STRUCTURED_REPLY, &reply, + NULL) < 0) + return -1; + if (reply.reply == NBD_REP_ACK) { + nbdkit_debug ("structured replies enabled"); + h->structured = true; + /* TODO: block status */ + } + else { + nbdkit_debug ("structured replies disabled"); + } /* Try NBD_OPT_GO */ nbdkit_debug ("trying NBD_OPT_GO"); @@ -828,6 +1013,13 @@ nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset, int c; assert (!flags); + /* We do not check whether structured read chunks cover the entire + buffer; as such, to avoid leaking uninit memory back to the + client, we must pre-zero the buffer. TODO: should nbdkit + guarantee a clean buffer to .pread, perhaps based on a flag set + when we register our plugin? */ + if (h->structured) + memset (buf, 0, count); c = nbd_request_full (h, 0, NBD_CMD_READ, offset, count, NULL, buf); return c < 0 ? c : nbd_reply (h, c); } -- 2.20.1

Add support for calling NBD_CMD_BLOCK_STATUS on the remote server when it is supported. I could have parsed that the server's id response to NBD_OPT_SET_META_CONTEXT is the same as what later appears in NBD_REPLY_TYPE_BLOCK_STATUS, but didn't think it worth the effort as long as we expect exactly one meta context. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- plugins/nbd/nbd.c | 135 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 129 insertions(+), 6 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 8eb8a31..9986d6c 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -130,6 +130,7 @@ struct transaction { uint64_t offset; uint32_t count; uint32_t err; + struct nbdkit_extents *extents; struct transaction *next; }; @@ -140,6 +141,7 @@ struct handle { int flags; int64_t size; bool structured; + bool extents; pthread_t reader; /* Prevents concurrent threads from interleaving writes to server */ @@ -286,7 +288,7 @@ nbd_request_raw (struct handle *h, uint16_t flags, uint16_t type, static int nbd_request_full (struct handle *h, uint16_t flags, uint16_t type, uint64_t offset, uint32_t count, const void *req_buf, - void *rep_buf) + void *rep_buf, struct nbdkit_extents *extents) { int err; struct transaction *trans; @@ -308,6 +310,7 @@ nbd_request_full (struct handle *h, uint16_t flags, uint16_t type, trans->buf = rep_buf; trans->count = rep_buf ? count : 0; trans->offset = offset; + trans->extents = extents; nbd_lock (h); if (h->dead) { nbd_unlock (h); @@ -340,7 +343,7 @@ static int nbd_request (struct handle *h, uint16_t flags, uint16_t type, uint64_t offset, uint32_t count) { - return nbd_request_full (h, flags, type, offset, count, NULL, NULL); + return nbd_request_full (h, flags, type, offset, count, NULL, NULL, NULL); } /* Read a reply, and look up the fd corresponding to the transaction. @@ -358,6 +361,12 @@ nbd_reply_raw (struct handle *h, int *fd) struct transaction *trans; void *buf = NULL; uint32_t count; + uint32_t id; + struct { + uint32_t length; + uint32_t flags; + } *extents = NULL; + size_t nextents = 0; int error = NBD_SUCCESS; bool more = false; uint64_t offset = 0; /* absolute offset of structured read chunk from buf */ @@ -452,6 +461,34 @@ nbd_reply_raw (struct handle *h, int *fd) } zero = true; break; + case NBD_REPLY_TYPE_BLOCK_STATUS: + if (!h->extents) { + nbdkit_error ("block status response without negotiation"); + free (buf); + return nbd_mark_dead (h); + } + if (rep.structured.length < sizeof *extents || + rep.structured.length % sizeof *extents != sizeof id) { + nbdkit_error ("structured reply OFFSET_HOLE size incorrect"); + free (buf); + return nbd_mark_dead (h); + } + nextents = rep.structured.length / sizeof *extents; + extents = malloc (rep.structured.length - sizeof id); + if (!extents) { + nbdkit_error ("malloc: %m"); + return nbd_mark_dead (h); + } + memcpy (&id, buf, sizeof id); + id = be32toh (id); + nbdkit_debug ("parsing %zu extents for context id %" PRId32, + nextents, id); + memcpy (extents, (char *) buf + sizeof id, sizeof *extents * nextents); + for (size_t i = 0; i < nextents; i++) { + extents[i].length = be32toh (extents[i].length); + extents[i].flags = be32toh (extents[i].flags); + } + break; default: if (NBD_REPLY_TYPE_IS_ERR (rep.structured.type)) { uint16_t errlen; @@ -497,9 +534,29 @@ nbd_reply_raw (struct handle *h, int *fd) trans = find_trans_by_cookie (h, rep.simple.handle, !more); if (!trans) { nbdkit_error ("reply with unexpected cookie %#" PRIx64, rep.simple.handle); + free (extents); return nbd_mark_dead (h); } + if (nextents) { + if (!trans->extents) { + nbdkit_error ("block status response to a non-status command"); + free (extents); + return nbd_mark_dead (h); + } + offset = trans->offset; + for (size_t i = 0; i < nextents; i++) { + /* We rely on the fact that NBDKIT_EXTENT_* match NBD_STATE_* */ + if (nbdkit_add_extent (trans->extents, offset, extents[i].length, + extents[i].flags) == -1) { + error = EINVAL; + break; + } + offset += extents[i].length; + } + free (extents); + } + if (!more) { *fd = trans->u.fds[1]; if (!error) @@ -686,8 +743,11 @@ nbd_newstyle_recv_option_reply (struct handle *h, uint32_t option, static int nbd_newstyle_haggle (struct handle *h) { + const char *const query = "base:allocation"; struct new_option opt; uint32_t exportnamelen = htobe32 (strlen (export)); + uint32_t nrqueries = htobe32 (1); + uint32_t querylen = htobe32 (strlen (query)); /* For now, we make no NBD_INFO_* requests, relying on the server to send its defaults. TODO: nbdkit should let plugins report block sizes, at which point we should request NBD_INFO_BLOCK_SIZE and @@ -710,9 +770,47 @@ nbd_newstyle_haggle (struct handle *h) NULL) < 0) return -1; if (reply.reply == NBD_REP_ACK) { - nbdkit_debug ("structured replies enabled"); + nbdkit_debug ("structured replies enabled, trying NBD_OPT_SET_META_CONTEXT"); h->structured = true; - /* TODO: block status */ + + opt.version = htobe64 (NEW_VERSION); + opt.option = htobe32 (NBD_OPT_SET_META_CONTEXT); + opt.optlen = htobe32 (sizeof exportnamelen + strlen (export) + + sizeof nrqueries + sizeof querylen + strlen (query)); + if (write_full (h->fd, &opt, sizeof opt) || + write_full (h->fd, &exportnamelen, sizeof exportnamelen) || + write_full (h->fd, export, strlen (export)) || + write_full (h->fd, &nrqueries, sizeof nrqueries) || + write_full (h->fd, &querylen, sizeof querylen) || + write_full (h->fd, query, strlen (query))) { + nbdkit_error ("unable to request NBD_OPT_SET_META_CONTEXT: %m"); + return -1; + } + if (nbd_newstyle_recv_option_reply (h, NBD_OPT_SET_META_CONTEXT, &reply, + NULL) < 0) + return -1; + if (reply.reply == NBD_REP_META_CONTEXT) { + /* Cheat: we asked for exactly one context. We could double + check that the server is replying with exactly the + "base:allocation" context, and then remember the id it tells + us to later confirm that responses to NBD_CMD_BLOCK_STATUS + match up; but in the absence of multiple contexts, it's + easier to just assume the server is compliant, and will reuse + the same id, without bothering to check further. */ + nbdkit_debug ("extents enabled"); + h->extents = true; + if (nbd_newstyle_recv_option_reply (h, NBD_OPT_SET_META_CONTEXT, &reply, + NULL) < 0) + return -1; + } + if (reply.reply != NBD_REP_ACK) { + if (h->extents) { + nbdkit_error ("unexpected response to set meta context"); + return -1; + } + nbdkit_debug ("ignoring meta context response %s", + name_of_nbd_rep (reply.reply)); + } } else { nbdkit_debug ("structured replies disabled"); @@ -1008,6 +1106,14 @@ nbd_can_multi_conn (void *handle) return h->flags & NBD_FLAG_CAN_MULTI_CONN; } +static int +nbd_can_extents (void *handle) +{ + struct handle *h = handle; + + return h->extents; +} + /* Read data from the file. */ static int nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset, @@ -1024,7 +1130,7 @@ nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset, when we register our plugin? */ if (h->structured) memset (buf, 0, count); - c = nbd_request_full (h, 0, NBD_CMD_READ, offset, count, NULL, buf); + c = nbd_request_full (h, 0, NBD_CMD_READ, offset, count, NULL, buf, NULL); return c < 0 ? c : nbd_reply (h, c); } @@ -1038,7 +1144,7 @@ nbd_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, assert (!(flags & ~NBDKIT_FLAG_FUA)); c = nbd_request_full (h, flags & NBDKIT_FLAG_FUA ? NBD_CMD_FLAG_FUA : 0, - NBD_CMD_WRITE, offset, count, buf, NULL); + NBD_CMD_WRITE, offset, count, buf, NULL, NULL); return c < 0 ? c : nbd_reply (h, c); } @@ -1086,6 +1192,21 @@ nbd_flush (void *handle, uint32_t flags) return c < 0 ? c : nbd_reply (h, c); } +/* Read extents of the file. */ +static int +nbd_extents (void *handle, uint32_t count, uint64_t offset, + uint32_t flags, struct nbdkit_extents *extents) +{ + struct handle *h = handle; + int c; + + assert (!(flags & ~NBDKIT_FLAG_REQ_ONE) && h->extents); + c = nbd_request_full (h, flags & NBDKIT_FLAG_REQ_ONE ? NBD_CMD_FLAG_REQ_ONE : 0, + NBD_CMD_BLOCK_STATUS, offset, count, NULL, NULL, + extents); + return c < 0 ? c : nbd_reply (h, c); +} + static struct nbdkit_plugin plugin = { .name = "nbd", .longname = "nbdkit nbd plugin", @@ -1104,11 +1225,13 @@ static struct nbdkit_plugin plugin = { .can_zero = nbd_can_zero, .can_fua = nbd_can_fua, .can_multi_conn = nbd_can_multi_conn, + .can_extents = nbd_can_extents, .pread = nbd_pread, .pwrite = nbd_pwrite, .zero = nbd_zero, .flush = nbd_flush, .trim = nbd_trim, + .extents = nbd_extents, .errno_is_preserved = 1, }; -- 2.20.1