From 7423f417827146f956df820f172d0bf80a489495 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 21 Jul 2016 13:34:46 -0600 Subject: [PATCH] nbd: Limit nbdflags to 16 bits Rather than asserting that nbdflags is within range, just give it the correct type to begin with :) nbdflags corresponds to the per-export portion of NBD Protocol "transmission flags", which is 16 bits in response to NBD_OPT_EXPORT_NAME and NBD_OPT_GO. Furthermore, upstream NBD has never passed the global flags to the kernel via ioctl(NBD_SET_FLAGS) (the ioctl was first introduced in NBD 2.9.22; then a latent bug in NBD 3.1 actually tried to OR the global flags with the transmission flags, with the disaster that the addition of NBD_FLAG_NO_ZEROES in 3.9 caused all earlier NBD 3.x clients to treat every export as read-only; NBD 3.10 and later intentionally clip things to 16 bits to pass only transmission flags). Qemu should follow suit, since the current two global flags (NBD_FLAG_FIXED_NEWSTYLE and NBD_FLAG_NO_ZEROES) have no impact on the kernel's behavior during transmission. CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake Message-Id: <1469129688-22848-3-git-send-email-eblake@redhat.com> Signed-off-by: Paolo Bonzini --- block/nbd-client.h | 2 +- include/block/nbd.h | 6 +++--- nbd/client.c | 28 +++++++++++++++------------- nbd/server.c | 10 ++++------ qemu-nbd.c | 4 ++-- 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/block/nbd-client.h b/block/nbd-client.h index fa9817b7d7..044aca4530 100644 --- a/block/nbd-client.h +++ b/block/nbd-client.h @@ -20,7 +20,7 @@ typedef struct NbdClientSession { QIOChannelSocket *sioc; /* The master data channel */ QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */ - uint32_t nbdflags; + uint16_t nbdflags; off_t size; CoMutex send_mutex; diff --git a/include/block/nbd.h b/include/block/nbd.h index cb91820f38..1897557a9b 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -90,11 +90,11 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc, size_t niov, size_t length, bool do_read); -int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, +int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags, QCryptoTLSCreds *tlscreds, const char *hostname, QIOChannel **outioc, off_t *size, Error **errp); -int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size); +int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size); ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request); ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply); int nbd_client(int fd); @@ -104,7 +104,7 @@ typedef struct NBDExport NBDExport; typedef struct NBDClient NBDClient; NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, - uint32_t nbdflags, void (*close)(NBDExport *), + uint16_t nbdflags, void (*close)(NBDExport *), Error **errp); void nbd_export_close(NBDExport *exp); void nbd_export_get(NBDExport *exp); diff --git a/nbd/client.c b/nbd/client.c index 78a7195c45..a92f1e2275 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -408,7 +408,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, } -int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, +int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags, QCryptoTLSCreds *tlscreds, const char *hostname, QIOChannel **outioc, off_t *size, Error **errp) @@ -468,7 +468,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, uint32_t opt; uint32_t namesize; uint16_t globalflags; - uint16_t exportflags; bool fixedNewStyle = false; if (read_sync(ioc, &globalflags, sizeof(globalflags)) != @@ -477,7 +476,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, goto fail; } globalflags = be16_to_cpu(globalflags); - *flags = globalflags << 16; TRACE("Global flags are %" PRIx32, globalflags); if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) { fixedNewStyle = true; @@ -545,17 +543,15 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, goto fail; } *size = be64_to_cpu(s); - TRACE("Size is %" PRIu64, *size); - if (read_sync(ioc, &exportflags, sizeof(exportflags)) != - sizeof(exportflags)) { + if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) { error_setg(errp, "Failed to read export flags"); goto fail; } - exportflags = be16_to_cpu(exportflags); - *flags |= exportflags; - TRACE("Export flags are %" PRIx16, exportflags); + be16_to_cpus(flags); } else if (magic == NBD_CLIENT_MAGIC) { + uint32_t oldflags; + if (name) { error_setg(errp, "Server does not support export names"); goto fail; @@ -572,16 +568,22 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, *size = be64_to_cpu(s); TRACE("Size is %" PRIu64, *size); - if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) { + if (read_sync(ioc, &oldflags, sizeof(oldflags)) != sizeof(oldflags)) { error_setg(errp, "Failed to read export flags"); goto fail; } - *flags = be32_to_cpu(*flags); + be32_to_cpus(&oldflags); + if (oldflags & ~0xffff) { + error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags); + goto fail; + } + *flags = oldflags; } else { error_setg(errp, "Bad magic received"); goto fail; } + TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags); if (read_sync(ioc, &buf, 124) != 124) { error_setg(errp, "Failed to read reserved block"); goto fail; @@ -593,7 +595,7 @@ fail: } #ifdef __linux__ -int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size) +int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size) { unsigned long sectors = size / BDRV_SECTOR_SIZE; if (size / BDRV_SECTOR_SIZE != sectors) { @@ -689,7 +691,7 @@ int nbd_disconnect(int fd) } #else -int nbd_init(int fd, QIOChannelSocket *ioc, uint32_t flags, off_t size) +int nbd_init(int fd, QIOChannelSocket *ioc, uint16_t flags, off_t size) { return -ENOTSUP; } diff --git a/nbd/server.c b/nbd/server.c index 3c1e2b336b..80fbb4da1d 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -63,7 +63,7 @@ struct NBDExport { char *name; off_t dev_offset; off_t size; - uint32_t nbdflags; + uint16_t nbdflags; QTAILQ_HEAD(, NBDClient) clients; QTAILQ_ENTRY(NBDExport) next; @@ -544,8 +544,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data) NBDClient *client = data->client; char buf[8 + 8 + 8 + 128]; int rc; - const int myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM | - NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA); + const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM | + NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA); bool oldStyle; /* Old style negotiation header without options @@ -575,7 +575,6 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data) oldStyle = client->exp != NULL && !client->tlscreds; if (oldStyle) { - assert ((client->exp->nbdflags & ~65535) == 0); TRACE("advertising size %" PRIu64 " and flags %x", client->exp->size, client->exp->nbdflags | myflags); stq_be_p(buf + 8, NBD_CLIENT_MAGIC); @@ -606,7 +605,6 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data) goto fail; } - assert ((client->exp->nbdflags & ~65535) == 0); TRACE("advertising size %" PRIu64 " and flags %x", client->exp->size, client->exp->nbdflags | myflags); stq_be_p(buf + 18, client->exp->size); @@ -810,7 +808,7 @@ static void nbd_eject_notifier(Notifier *n, void *data) } NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, - uint32_t nbdflags, void (*close)(NBDExport *), + uint16_t nbdflags, void (*close)(NBDExport *), Error **errp) { NBDExport *exp = g_malloc0(sizeof(NBDExport)); diff --git a/qemu-nbd.c b/qemu-nbd.c index 321f02bd15..e3571c2025 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -251,7 +251,7 @@ static void *nbd_client_thread(void *arg) { char *device = arg; off_t size; - uint32_t nbdflags; + uint16_t nbdflags; QIOChannelSocket *sioc; int fd; int ret; @@ -465,7 +465,7 @@ int main(int argc, char **argv) BlockBackend *blk; BlockDriverState *bs; off_t dev_offset = 0; - uint32_t nbdflags = 0; + uint16_t nbdflags = 0; bool disconnect = false; const char *bindto = "0.0.0.0"; const char *port = NULL;