aboutsummaryrefslogtreecommitdiff
path: root/nbd
AgeCommit message (Collapse)AuthorFilesLines
2017-11-28nbd/server: CVE-2017-15118 Stack smash on large export nameEric Blake1-0/+4
Introduced in commit f37708f6b8 (2.10). The NBD spec says a client can request export names up to 4096 bytes in length, even though they should not expect success on names longer than 256. However, qemu hard-codes the limit of 256, and fails to filter out a client that probes for a longer name; the result is a stack smash that can potentially give an attacker arbitrary control over the qemu process. The smash can be easily demonstrated with this client: $ qemu-io f raw nbd://localhost:10809/$(printf %3000d 1 | tr ' ' a) If the qemu NBD server binary (whether the standalone qemu-nbd, or the builtin server of QMP nbd-server-start) was compiled with -fstack-protector-strong, the ability to exploit the stack smash into arbitrary execution is a lot more difficult (but still theoretically possible to a determined attacker, perhaps in combination with other CVEs). Still, crashing a running qemu (and losing the VM) is bad enough, even if the attacker did not obtain full execution control. CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com>
2017-11-28nbd/server: CVE-2017-15119 Reject options larger than 32MEric Blake1-0/+6
The NBD spec gives us permission to abruptly disconnect on clients that send outrageously large option requests, rather than having to spend the time reading to the end of the option. No real option request requires that much data anyways; and meanwhile, we already have the practice of abruptly dropping the connection on any client that sends NBD_CMD_WRITE with a payload larger than 32M. For comparison, nbdkit drops the connection on any request with more than 4096 bytes; however, that limit is probably too low (as the NBD spec states an export name can theoretically be up to 4096 bytes, which means a valid NBD_OPT_INFO could be even longer) - even if qemu doesn't permit exports longer than 256 bytes. It could be argued that a malicious client trying to get us to read nearly 4G of data on a bad request is a form of denial of service. In particular, if the server requires TLS, but a client that does not know the TLS credentials sends any option (other than NBD_OPT_STARTTLS or NBD_OPT_EXPORT_NAME) with a stated payload of nearly 4G, then the server was keeping the connection alive trying to read all the payload, tying up resources that it would rather be spending on a client that can get past the TLS handshake. Hence, this warranted a CVE. Present since at least 2.5 when handling known options, and made worse in 2.6 when fixing support for NBD_FLAG_C_FIXED_NEWSTYLE to handle unknown options. CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com>
2017-11-17nbd/server: Fix error reporting for bad requestsEric Blake1-24/+12
The NBD spec says an attempt to NBD_CMD_TRIM on a read-only export should fail with EPERM, as a trim has the potential to change disk contents, but we were relying on the block layer to catch that for us, which might not always give the right error (and even if it does, it does not let us pass back a sane message for structured replies). The NBD spec says an attempt to NBD_CMD_WRITE_ZEROES out of bounds should fail with ENOSPC, not EINVAL. Our check for u64 offset + u32 length wraparound up front is pointless; nothing uses offset until after the second round of sanity checks, and we can just as easily ensure there is no wraparound by checking whether offset is in bounds (since a disk size cannot exceed off_t which is 63 bits, adding a 32-bit number for a valid offset can't overflow). Bonus: dropping the up-front check lets us keep the connection alive after NBD_CMD_WRITE, whereas before we would drop the connection (of course, any client sending a packet that would trigger the failure is already buggy, so it's also okay to drop the connection, but better quality-of-implementation never hurts). Solve all of these issues by some code motion and improved request validation. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171115213557.3548-1-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
2017-11-17nbd/client: Don't hard-disconnect on ESHUTDOWN from serverEric Blake1-6/+0
The NBD spec says that a server may fail any transmission request with ESHUTDOWN when it is apparent that no further request from the client can be successfully honored. The client is supposed to then initiate a soft shutdown (wait for all remaining in-flight requests to be answered, then send NBD_CMD_DISC). However, since qemu's server never uses ESHUTDOWN errors, this code was mostly untested since its introduction in commit b6f5d3b5. More recently, I learned that nbdkit as the NBD server is able to send ESHUTDOWN errors, so I finally tested this code, and noticed that our client was special-casing ESHUTDOWN to cause a hard shutdown (immediate disconnect, with no NBD_CMD_DISC), but only if the server sends this error as a simple reply. Further investigation found that commit d2febedb introduced a regression where structured replies behave differently than simple replies - but that the structured reply behavior is more in line with the spec (even if we still lack code in nbd-client.c to properly quit sending further requests). So this patch reverts the portion of b6f5d3b5 that introduced an improper hard-disconnect special-case at the lower level, and leaves the future enhancement of a nicer soft-disconnect at the higher level for another day. CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171113194857.13933-1-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
2017-11-17nbd/client: Use error_prepend() correctlyEric Blake1-24/+26
When using error prepend(), it is necessary to end with a space in the format string; otherwise, messages come out incorrectly, such as when connecting to a socket that hangs up immediately: can't open device nbd://localhost:10809/: Failed to read dataUnexpected end-of-file before all bytes were read Originally botched in commit e44ed99d, then several more instances added in the meantime. Pre-existing and not fixed here: we are inconsistent on capitalization; some of our messages start with lower case, and others start with upper, although the use of error_prepend() is much nicer to read when all fragments consistently start with lower. CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171113152424.25381-1-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
2017-11-09nbd/server: Fix structured read of length 0Eric Blake2-1/+21
The NBD spec was recently clarified to state that a read of length 0 should not be attempted by a compliant client; but that a server must still handle it correctly in an unspecified manner (that is, either a successful no-op or an error reply, but not a crash) [1]. However, it also implies that NBD_REPLY_TYPE_OFFSET_DATA must have a non-zero payload length, but our existing code was replying with a chunk that a picky client could reject as invalid because it was missing a payload (our own client implementation was recently patched to be that picky, after first fixing it to not send 0-length requests). We are already doing successful no-ops for 0-length writes and for non-structured reads; so for consistency, we want structured reply reads to also be a no-op. The easiest way to do this is to return a NBD_REPLY_TYPE_NONE chunk; this is best done via a new helper function (especially since future patches for other structured replies may benefit from using the same helper). [1] https://github.com/NetworkBlockDevice/nbd/commit/ee926037 Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171108215703.9295-8-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
2017-11-09nbd: Fix struct name for structured readsEric Blake1-1/+1
A closer read of the NBD spec shows that a structured reply chunk for a hole is not quite identical to the prefix of a data chunk, because the hole has to also send a 32-bit size field. Although we do not yet send holes, we should fix the misleading information in our header and make it easier for a future patch to support sparse reads. Messed up in commit bae245d1. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171108215703.9295-5-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
2017-11-09nbd/client: Nicer trace of structured replyEric Blake2-2/+4
It's useful to know which structured reply chunk is being processed. Missed in commit d2febedb. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171108215703.9295-4-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
2017-11-08nbd/server: fix nbd_negotiate_handle_infoVladimir Sementsov-Ogievskiy1-1/+2
namelen should be here, length is unrelated, and always 0 at this point. Broken in introduction in commit f37708f6, but mostly harmless (replying with '' as the name does not violate protocol, and does not confuse qemu as the nbd client since our implementation does not ask for the name; but might confuse some other client that does ask for the name especially if the default export is different than the export name being queried). Adding an assert makes it obvious that we are not skipping any bytes in the client's message, as well as making it obvious that we were using the wrong variable. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> CC: qemu-stable@nongnu.org Message-Id: <20171101154204.27146-1-vsementsov@virtuozzo.com> [eblake: improve commit message, squash in assert addition] Signed-off-by: Eric Blake <eblake@redhat.com>
2017-10-30nbd: Minimal structured read for clientVladimir Sementsov-Ogievskiy2-1/+12
Minimal implementation: for structured error only error_report error message. Note that test 83 is now more verbose, because the implementation prints more warnings about unexpected communication errors; perhaps future patches should tone things down by using trace messages instead of traces, but the common case of successful communication is no noisier than before. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-13-eblake@redhat.com>
2017-10-30nbd: Move nbd_read() to common headerEric Blake1-9/+0
An upcoming change to block/nbd-client.c will want to read the tail of a structured reply chunk directly from the wire. Move this function to make it easier. Based on a patch from Vladimir Sementsov-Ogievskiy. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-12-eblake@redhat.com>
2017-10-30nbd/client: prepare nbd_receive_reply for structured replyVladimir Sementsov-Ogievskiy2-23/+84
In following patch nbd_receive_reply will be used both for simple and structured reply header receiving. NBDReply is altered into union of simple reply header and structured reply chunk header, simple error translation moved to block/nbd-client to be consistent with further structured reply error translation. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-11-eblake@redhat.com>
2017-10-30nbd/client: refactor nbd_receive_starttlsVladimir Sementsov-Ogievskiy2-18/+42
Split out nbd_request_simple_option to be reused for structured reply option. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-10-eblake@redhat.com>
2017-10-30nbd/server: Include human-readable message in structured errorsEric Blake2-8/+14
The NBD spec permits including a human-readable error string if structured replies are in force, so we might as well send the client the message that we logged on any error. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-9-eblake@redhat.com>
2017-10-30nbd: Minimal structured read for serverVladimir Sementsov-Ogievskiy2-10/+99
Minimal implementation of structured read: one structured reply chunk, no segmentation. Minimal structured error implementation: no text message. Support DF flag, but just ignore it, as there is no segmentation any way. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-8-eblake@redhat.com>
2017-10-30nbd/server: Refactor zero-length option checkEric Blake1-28/+46
Consolidate the response for a non-zero-length option payload into a new function, nbd_reject_length(). This check will also be used when introducing support for structured replies. Note that STARTTLS response differs based on time: if the connection is still unencrypted, we set fatal to true (a client that can't request TLS correctly may still think that we are ready to start the TLS handshake, so we must disconnect); while if the connection is already encrypted, the client is sending a bogus request but is no longer at risk of being confused by continuing the connection. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-7-eblake@redhat.com> [eblake: correct return value on STARTTLS] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
2017-10-30nbd/server: Simplify nbd_negotiate_options loopEric Blake1-15/+4
Instead of making each caller check whether a transmission error occurred, we can sink a common error check to the end of the loop. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20171027104037.8319-6-eblake@redhat.com> [eblake: squash in compiler warning fix] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
2017-10-30nbd/server: Report error for write to read-only exportEric Blake1-1/+2
When the server is read-only, we were already reporting an error message for NBD_CMD_WRITE_ZEROES, but failed to set errp for a similar NBD_CMD_WRITE. This will matter more once structured replies allow the server to propagate the errp information back to the client. While at it, use an error message that makes a bit more sense if viewed on the client side. Note that when using qemu-io to test qemu-nbd behavior, it is rather difficult to convince qemu-io to send protocol violations (such as a read beyond bounds), because we have a lot of active checking on the client side that a qemu-io request makes sense before it ever goes over the wire to the server. The case of a client attempting a write when the server is started as 'qemu-nbd -r' is one of the few places where we can easily test error path handling, without having to resort to hacking in known temporary bugs to either the server or client. [Maybe we want a future patch to the client to do up-front checking on writes to a read-only export, the way it does up-front bounds checking; but I don't see anything in the NBD spec that points to a protocol violation in our current behavior.] Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-5-eblake@redhat.com>
2017-10-30nbd: Expose constants and structs for structured readEric Blake3-1/+30
Upcoming patches will implement the NBD structured reply extension [1] for both client and server roles. Declare the constants, structs, and lookup routines that will be valuable whether the server or client code is backported in isolation. This includes moving one constant from an internal header to the public header, as part of the structured read processing will be done in block/nbd-client.c rather than nbd/client.c. [1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md Based on patches from Vladimir Sementsov-Ogievskiy. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-4-eblake@redhat.com>
2017-10-30nbd: Move nbd_errno_to_system_errno() to public headerEric Blake4-45/+37
This is needed in preparation for structured reply handling, as we will be performing the translation from NBD error to system errno value higher in the stack at block/nbd-client.c. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-3-eblake@redhat.com>
2017-10-30nbd: Include error names in trace messagesEric Blake5-4/+30
NBD errors were originally sent over the wire based on Linux errno values; but not all the world is Linux, and not all platforms share the same values. Since a number isn't very easy to decipher on all platforms, update the trace messages to include the name of NBD errors being sent/received over the wire. Tweak the trace messages to be at the point where we are using the NBD error, not the translation to the host errno values. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171027104037.8319-2-eblake@redhat.com>
2017-10-13nbd: header constants indentingVladimir Sementsov-Ogievskiy1-17/+17
Prepare indenting for the following commit. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20171012095319.136610-9-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
2017-10-13nbd/server: simplify reply transmissionVladimir Sementsov-Ogievskiy1-25/+24
Send qiov via qio_channel_writev_all instead of calling nbd_write twice with a cork. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171012095319.136610-8-vsementsov@virtuozzo.com> [eblake: rebase to tweaks earlier in series] Signed-off-by: Eric Blake <eblake@redhat.com>
2017-10-13nbd/server: refactor nbd_co_send_simple_reply parametersVladimir Sementsov-Ogievskiy1-6/+7
Pass client and buffer (*data) parameters directly, to make the function consistent with further structured reply sending functions. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171012095319.136610-7-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
2017-10-13nbd/server: do not use NBDReply structureVladimir Sementsov-Ogievskiy1-21/+15
NBDReply structure will be upgraded in future patches to handle both simple and structured replies and will be used only in the client Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171012095319.136610-6-vsementsov@virtuozzo.com> [eblake: rebase to tweaks earlier in series] Signed-off-by: Eric Blake <eblake@redhat.com>
2017-10-12nbd/server: structurize simple reply header sendingVladimir Sementsov-Ogievskiy2-24/+14
Use packed structure instead of pointer arithmetics. Also, merge two redundant traces into one. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20171012095319.136610-5-vsementsov@virtuozzo.com> [eblake: tweak and mention impact on traces, fix errp usage] Signed-off-by: Eric Blake <eblake@redhat.com>
2017-10-12nbd: rename some simple-request related objects to be _simple_Vladimir Sementsov-Ogievskiy4-10/+10
To be consistent when their _structured_ analogs will be introduced. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20171012095319.136610-4-vsementsov@virtuozzo.com> [eblake: also tweak trace message contents] Signed-off-by: Eric Blake <eblake@redhat.com>
2017-10-12NBD: use g_new() family of functionsMarc-André Lureau1-2/+2
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <20171006235023.11952-22-f4bug@amsat.org> Signed-off-by: Eric Blake <eblake@redhat.com>
2017-09-06nbd: Use new qio_channel_*_all() functionsEric Blake2-79/+7
Rather than open-coding our own read/write-all functions, we can make use of the recently-added qio code. It slightly changes the error message in one of the iotests. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170905191114.5959-4-eblake@redhat.com> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
2017-08-30nbd/client: fix nbd_send_request to return intVladimir Sementsov-Ogievskiy1-1/+1
Fix nbd_send_request to return int, as it returns a return value of nbd_write (which is int), and the only user of nbd_send_request's return value (nbd_co_send_request) consider it as int too. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170804151440.320927-5-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
2017-08-30nbd/client: refactor nbd_receive_replyVladimir Sementsov-Ogievskiy1-3/+9
Refactor nbd_receive_reply to return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170804151440.320927-4-vsementsov@virtuozzo.com> [eblake: tweak function comments] Signed-off-by: Eric Blake <eblake@redhat.com>
2017-08-30nbd/client: refactor nbd_read_eofVladimir Sementsov-Ogievskiy2-14/+24
Refactor nbd_read_eof to return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170804151440.320927-3-vsementsov@virtuozzo.com> [eblake: tweak function comments, rebase to test 083 enhancements] Signed-off-by: Eric Blake <eblake@redhat.com>
2017-08-30nbd/client: fix nbd_opt_goVladimir Sementsov-Ogievskiy1-2/+0
Do not send NBD_OPT_ABORT to the broken server. After sending NBD_REP_ACK on NBD_OPT_GO server is most probably in transmission phase, when option sending is finished. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170804151440.320927-2-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com>
2017-08-15nbd: Fix order of bdrv_set_perm and bdrv_invalidate_cacheKevin Wolf1-9/+11
The "inactive" state of BDS affects whether the permissions can be granted, we must call bdrv_invalidate_cache before bdrv_set_perm to support "-incoming defer" case. Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Fam Zheng <famz@redhat.com> Message-Id: <20170815130740.31229-3-famz@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>
2017-08-15nbd: Fix trace message for disconnectEric Blake1-1/+1
NBD_CMD_DISC is a disconnect request, not a data discard request. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170811015749.20365-1-eblake@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
2017-08-01trace-events: fix code style: print 0x before hex numbersVladimir Sementsov-Ogievskiy1-9/+9
The only exception are groups of numers separated by symbols '.', ' ', ':', '/', like 'ab.09.7d'. This patch is made by the following: > find . -name trace-events | xargs python script.py where script.py is the following python script: ========================= #!/usr/bin/env python import sys import re import fileinput rhex = '%[-+ *.0-9]*(?:[hljztL]|ll|hh)?(?:x|X|"\s*PRI[xX][^"]*"?)' rgroup = re.compile('((?:' + rhex + '[.:/ ])+' + rhex + ')') rbad = re.compile('(?<!0x)' + rhex) files = sys.argv[1:] for fname in files: for line in fileinput.input(fname, inplace=True): arr = re.split(rgroup, line) for i in range(0, len(arr), 2): arr[i] = re.sub(rbad, '0x\g<0>', arr[i]) sys.stdout.write(''.join(arr)) ========================= Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Acked-by: Cornelia Huck <cohuck@redhat.com> Message-id: 20170731160135.12101-5-vsementsov@virtuozzo.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2017-07-28nbd: fix memory leak in nbd_opt_go()Philippe Mathieu-Daudé1-3/+5
nbd/client.c:385:12: warning: Potential leak of memory pointed to by 'buf' Reported-by: Clang Static Analyzer Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20170727024224.22900-5-f4bug@amsat.org> [introduced in commit 8ecaeae8] Signed-off-by: Eric Blake <eblake@redhat.com>
2017-07-17nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clientsEric Blake2-10/+16
A typo in commit 23e099c set the size of buf[] used in response to NBD_OPT_EXPORT_NAME according to the length needed for old-style negotiation (4 bytes of flag information) instead of the intended 2 bytes used in new style. If the client doesn't enable NBD_FLAG_C_NO_ZEROES, then the server sends two bytes too many, and is then out of sync in response to the client's next command (the bug is masked when modern qemu is the client, since we enable the no zeroes flag). While touching this code, add some more defines to nbd_internal.h rather than having quite so many magic numbers in the .c; also, use "" initialization rather than memset(), and tweak the oldstyle negotiation to better match the spec description of the layout (since the spec is big-endian, skipping two bytes as 0 followed by writing a 2-byte flag is the same as writing a zero-extended 4-byte flag), to make it a bit easier to follow compared to the spec. [checkpatch.pl has some false positives in the comments] Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170717192635.17880-3-eblake@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com>
2017-07-17nbd: Trace client command being sentEric Blake2-2/+3
Make the client trace slightly more legible by including the name of the command being sent. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-Id: <20170717192635.17880-2-eblake@redhat.com>
2017-07-17nbd: Fix iotests failure due to changed client error messageEric Blake1-3/+2
Commit 8ecaeae8 changed the way the client requests an NBD export, and in the process also changed the resulting error message when the export is not present, breaking a couple of iotests. The error message is now directly given by the server (a failed NBD_OPT_GO) instead of implied by the client (after exhausting NBD_OPT_LIST), but looking at the testsuite changes, it proves worthwhile to reword the error message to be slightly less verbose (as this is one particular error message likely to be hit by a user). Note that the error message is now sensitive to which binary is running the server as well as the client (since the expected output is replaying a message received from the server - for that matter, it depends on a server new enough to understand NBD_OPT_GO); in general iotests are run on client and server from the same source code base so the default setup will pass; but if it proves problematic for people overriding QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, and QEMU_NBD_PROG to point across multiple builds for cross-version integration testing, we may have to later tweak or sanitize the output somehow. Reported-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170717142310.17048-1-eblake@redhat.com> Tested-by: John Snow <jsnow@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com>
2017-07-14nbd: Implement NBD_INFO_BLOCK_SIZE on clientEric Blake2-12/+70
The upstream NBD Protocol has defined a new extension to allow the server to advertise block sizes to the client, as well as a way for the client to inform the server whether it intends to obey block sizes. When using the block layer as the client, we will obey block sizes; but when used as 'qemu-nbd -c' to hand off to the kernel nbd module as the client, we are still waiting for the kernel to implement a way for us to learn if it will honor block sizes (perhaps by an addition to sysfs, rather than an ioctl), as well as any way to tell the kernel what additional block sizes to obey (NBD_SET_BLKSIZE appears to be accurate for the minimum size, but preferred and maximum sizes would probably be new ioctl()s), so until then, we need to make our request for block sizes conditional. When using ioctl(NBD_SET_BLKSIZE) to hand off to the kernel, use the minimum block size as the sector size if it is larger than 512, which also has the nice effect of cooperating with (non-qemu) servers that don't do read-modify-write when exposing a block device with 4k sectors; it might also allow us to visit a file larger than 2T on a 32-bit kernel. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-10-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-14nbd: Implement NBD_INFO_BLOCK_SIZE on serverEric Blake2-4/+45
The upstream NBD Protocol has defined a new extension to allow the server to advertise block sizes to the client, as well as a way for the client to inform the server that it intends to obey block sizes. Thanks to a recent fix (commit df7b97ff), our real minimum transfer size is always 1 (the block layer takes care of read-modify-write on our behalf), but we're still more efficient if we advertise 512 when the client supports it, as follows: - OPT_INFO, but no NBD_INFO_BLOCK_SIZE: advertise 512, then fail with NBD_REP_ERR_BLOCK_SIZE_REQD; client is free to try something else since we don't disconnect - OPT_INFO with NBD_INFO_BLOCK_SIZE: advertise 512 - OPT_GO, but no NBD_INFO_BLOCK_SIZE: advertise 1 - OPT_GO with NBD_INFO_BLOCK_SIZE: advertise 512 We can also advertise the optimum block size (presumably the cluster size, when exporting a qcow2 file), and our absolute maximum transfer size of 32M, to help newer clients avoid EINVAL failures or abrupt disconnects on oversize requests. We do not reject clients for using the older NBD_OPT_EXPORT_NAME; we are no worse off for those clients than we used to be. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-9-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-14nbd: Implement NBD_OPT_GO on clientEric Blake3-2/+130
NBD_OPT_EXPORT_NAME is lousy: per the NBD protocol, any failure requires the server to close the connection rather than report an error to us. Therefore, upstream NBD recently added NBD_OPT_GO as the improved version of the option that does what we want [1]: it reports sane errors on failures, and on success provides at least as much info as NBD_OPT_EXPORT_NAME. [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.md This is a first cut at use of the information types. Note that we do not need to use NBD_OPT_INFO, and that use of NBD_OPT_GO means we no longer have to use NBD_OPT_LIST to learn whether a server requires TLS (this requires servers that gracefully handle unknown NBD_OPT, many servers prior to qemu 2.5 were buggy, but I have patched qemu, upstream nbd, and nbdkit in the meantime, in part because of interoperability testing with this patch). We still fall back to NBD_OPT_LIST when NBD_OPT_GO is not supported on the server, as it is still one last chance for a nicer error message. Later patches will use further info, like NBD_INFO_BLOCK_SIZE. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-8-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-14nbd: Implement NBD_OPT_GO on serverEric Blake2-3/+179
NBD_OPT_EXPORT_NAME is lousy: per the NBD protocol, any failure requires us to close the connection rather than report an error. Therefore, upstream NBD recently added NBD_OPT_GO as the improved version of the option that does what we want [1], along with NBD_OPT_INFO that returns the same information but does not transition to transmission phase. [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.md This is a first cut at the information types, and only passes the same information already available through NBD_OPT_LIST and NBD_OPT_EXPORT_NAME; items like NBD_INFO_BLOCK_SIZE (and thus any use of NBD_REP_ERR_BLOCK_SIZE_REQD) are intentionally left for later patches. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-7-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-14nbd: Refactor reply to NBD_OPT_EXPORT_NAMEEric Blake1-22/+28
Reply directly in nbd_negotiate_handle_export_name(), rather than waiting until nbd_negotiate_options() completes. This will make it easier to implement NBD_OPT_GO. Pass additional parameters around, rather than stashing things inside NBDClient. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-6-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-14nbd: Simplify trace of client flags in negotiationEric Blake2-7/+3
Simplify the tracing of client flags in the server, and return -EINVAL instead of -EIO if we successfully read but don't like those flags. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-5-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-14nbd: Expose and debug more NBD constantsEric Blake5-37/+147
The NBD protocol has several constants defined in various extensions that we are about to implement. Expose them to the code, along with an easy way to map various constants to strings during diagnostic messages. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-4-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-14nbd: Don't bother tracing an NBD_OPT_ABORT response failureEric Blake2-12/+4
We really don't care if our spec-compliant reply to NBD_OPT_ABORT was received, so shave off some lines of code by not even tracing it. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-3-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-14nbd: Create struct for tracking export infoEric Blake1-22/+22
The NBD Protocol is introducing some additional information about exports, such as minimum request size and alignment, as well as an advertised maximum request size. It will be easier to feed this information back to the block layer if we gather all the information into a struct, rather than adding yet more pointer parameters during negotiation. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20170707203049.534-2-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-07-10nbd: use generic trace subsystem instead of TRACE macroVladimir Sementsov-Ogievskiy4-95/+119
Let NBD use the trace mechanisms already present in qemu. Now you can use the -trace optino of qemu, or the -T/--trace option of qemu-img, qemu-io, and qemu-nbd, to select nbd traces. For qemu, the QMP commands trace-event-{get,set}-state can also toggle tracing on the fly. Example: qemu-nbd --trace 'nbd_*' <image file> # enables all nbd traces Recompilation with CFLAGS=-DDEBUG_NBD is no more needed, furthermore, DEBUG_NBD macro is removed from the code. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20170707152918.23086-11-vsementsov@virtuozzo.com> [eblake: minor tweaks to a couple of traces] Signed-off-by: Eric Blake <eblake@redhat.com>