Re: [Libguestfs] [libnbd PATCH] info: Try harder for graceful disconnect from server
On Fri, Sep 22, 2023 at 04:03:19PM -0500, Eric Blake wrote: > On Fri, Sep 22, 2023 at 09:47:55PM +0100, Richard W.M. Jones wrote: > > On Fri, Sep 22, 2023 at 12:33:36PM -0500, Eric Blake wrote: > > > There are a number of ways in which gathering information can fail. > > > But when possible, it makes sense to let the server know that we are > > > done talking, as it minimizes the likelihood that nbdinfo's error > > > message will be obscured by an accompanying error message by the > > > server complaining about an unclean disconnect. > > > > > > For example, with a one-off qemu temporarily patched as mentioned in > > > commit 0f8ee8c6 to advertise sizes larger than 2^63, kicking off > > > 'qemu-nbd -f raw -r file &' produces: > > > > > > pre-patch: > > > > > > $ ./run nbdinfo --size nbd://localhost > > > /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size > > > 9223372036854781440 which does not fit in signed result: Value too large > > > for defined data type > > > qemu-nbd: option negotiation failed: Failed to read opts magic: > > > Unexpected end-of-file before all data were read > > > > This doesn't necessarily seem like a bug? > > It's a quality of service thing - qemu is just being noisy that the > client closed the socket abruptly without giving any reason why. It > doesn't change libnbd's behavior (nbdinfo has already reported its > error message), but may confuse people reading qemu-nbd logs. > > It may also be a case where we could patch qemu-nbd to not output a > complaint if the client exited at a clean point in the back-and-forth > transactions. We still want to be noisy if the socket closes after > the first byte has been read, but if there are zero bytes available, > announcing that the client no longer cares does not add much value. > > > > > This feels like quite a significant change in behaviour to me. In > > particular I'm worried if it interacts in some subtle way with the > > forking done by the "[ ... ]" syntax for running servers on the > > command line (or any of the other ways that libnbd might fork/exec). > > Interesting observation. atexit() handlers are not preserved across > exec, and I think all our fork() paths end in either exec or _exit > (also where atexit handlers are ignored), so I don't think we are > risking calling the handler twice. > > > > > Can we hold this patch until after 1.18 is released and then put it > > in? Should only be a week or two. > > Sure, being conservative on this one is fine by me. I'll delay it > until after 1.18. > > > > > Provisionally ACKed for post-1.18 Now in as commit fd4f3fea, so we can start getting some soak time under CI. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] libnbd | Failed pipeline for master | fd4f3fea
Pipeline #1018926358 has failed! Project: libnbd ( https://gitlab.com/nbdkit/libnbd ) Branch: master ( https://gitlab.com/nbdkit/libnbd/-/commits/master ) Commit: fd4f3fea ( https://gitlab.com/nbdkit/libnbd/-/commit/fd4f3feabb165c2932cb75a9cacb8c99dbcc847b ) Commit Message: info: Try harder for graceful disconnect from s... Commit Author: Eric Blake ( https://gitlab.com/ebblake ) Pipeline #1018926358 ( https://gitlab.com/nbdkit/libnbd/-/pipelines/1018926358 ) triggered by Eric Blake ( https://gitlab.com/ebblake ) had 1 failed job. Job #5179635441 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5179635441/raw ) Stage: builds Name: x86_64-freebsd-13 -- You're receiving this email because of your account on gitlab.com. ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On Wed, Sep 27, 2023 at 08:46:58PM +0800, Ming Xie wrote: > Hi Richard, > > Sorry, I missed the email, I found out that the ova OS used by the > customer is win11 uefi, so I tried to reproduce the error today using > win11-uefi guest > > Steps: > 1. Convert win10-efi with UTC-8 timezone, win11-non-efi with UTC-8 timezone, > win11-efi with UTC-8 timezone, win11-efi with UTC+8 timezone, win2022-efi with > UTC-8 timezone from VMware by v2v, then check the installation status after > finishing conversion > > > Summary the test result as below: > > qemu-ga > > Win10-efi-UTC-8 PASS > Win11-non-efi UTC-8 PASS > Win11-efi-UTC-8 FAIL(can't find qemu-ga log in c:\) > Win11-efi-UTC+8 FAIL(can't find qemu-ga log in c:\) > Win2022-efi-UTC-8 PASS Thanks for testing. The symptoms look the same as Lee reported. You're running virt-v2v from RHEL 9? If so I will prepare a scratch RHEL 9 package with the patches which we think might fix this, for testing. Rich. > Based on the above results, I think this problem is only caused by win11-efi > and has nothing to do with the time zone "UTC-8" > > The following are existing bugs about qemu-ga: > > Bug 1820144 - cannot install qemu-ga to some guests even if scheduled qemu-ga > installation task exist > Bug 2114809 - Can't install qemu-ga because of network address error after > converting MD-RAID1 win11 host by virt-p2v > Bug 1820152 - Fail to execute installation-qemu-ga.msi for win2012r2 and > win8.1-i386 guests > > > Thanks & Regards > Ming Xie > > On Fri, Sep 22, 2023 at 6:40 PM Richard W.M. Jones wrote: > > On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote: > > On 21.09.23 19:43, Richard W.M. Jones wrote: > > >So this is probably another instance or variation of the timezone > > >formatting problem (of schtasks). Which version of virt-v2v is this? > > >I want to check that you have a version with all the latest patches in > > >this area. > > > > It's 2.2.0-1 from Debian (12) bookworm. I've verified that it > > doesn't have any distro-specific patches. > > > > (https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/ > debian > > would have a patches/series file in this case) > > The timezone fixes are: > > commit 597d177567234c3a539098c423649781424eeb6f > Author: Laszlo Ersek > Date: Tue Mar 8 15:30:51 2022 +0100 > > convert_windows: rewrite "configure_qemu_ga" script purely in > PowerShell > > commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f > Author: Richard W.M. Jones > Date: Fri Nov 12 08:47:55 2021 + > > convert/convert_windows.ml: Handle date formats with dots instead of / > > They are all included in >= 2.0 > > I wonder if 597d177567 has a subtle flaw, or if we introduced a bug > somewhere when refactoring this code later. > > Lee: Do you have a theory about exactly what is wrong with the > schtasks date? Like what was it supposed to be, assuming it was 120 > seconds in the future from boot time, versus what it was set to: > > > Firstboot-qemu-ga 9/21/2023 4:04:00 PM Ready > > Could a date or time field have not been swapped or been corrupted > in some predictable way? > > The code we run is here: > > https://github.com/libguestfs/libguestfs-common/blob/ > e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml# > L571 > > Ming: this could be a bug affecting PST (UTC-8) guests, perhaps > somehow related to having a single digit month field? > > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/ > ~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > libguestfs lets you edit virtual machines. Supports shell scripting, > bindings from many languages. http://libguestfs.org > > -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v7 01/12] nbd/server: Support a request payload
On Wed, Sep 27, 2023 at 11:55:41AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 25.09.23 22:22, Eric Blake wrote:
> > Upcoming additions to support NBD 64-bit effect lengths allow for the
> > possibility to distinguish between payload length (capped at 32M) and
> > effect length (64 bits, although we generally assume 63 bits because
> > of off_t limitations). Without that extension, only the NBD_CMD_WRITE
> > request has a payload; but with the extension, it makes sense to allow
> > at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
> > in a future patch (where the payload is a limited-size struct that in
> > turn gives the real effect length as well as a subset of known ids for
> > which status is requested). Other future NBD commands may also have a
> > request payload, so the 64-bit extension introduces a new
> > NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
> > length is a payload length or an effect length, rather than
> > hard-coding the decision based on the command.
> >
> > According to the spec, a client should never send a command with a
> > payload without the negotiation phase proving such extension is
> > available. So in the unlikely event the bit is set or cleared
> > incorrectly, the client is already at fault; if the client then
> > provides the payload, we can gracefully consume it off the wire and
> > fail the command with NBD_EINVAL (subsequent checks for magic numbers
> > ensure we are still in sync), while if the client fails to send
> > payload we block waiting for it (basically deadlocking our connection
> > to the bad client, but not negatively impacting our ability to service
> > other clients, so not a security risk). Note that we do not support
> > the payload version of BLOCK_STATUS yet.
> >
> > Signed-off-by: Eric Blake
> > ---
> >
> > v7: another try at better logic [Vladimir]
> >
> > v5: retitled from v4 13/24, rewrite on top of previous patch's switch
> > statement [Vladimir]
> >
> > v4: less indentation on several 'if's [Vladimir]
> > ---
> > nbd/server.c | 37 +
> > nbd/trace-events | 1 +
> > 2 files changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 7a6f95071f8..1eabcfc908d 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -2322,9 +2322,11 @@ static int coroutine_fn
> > nbd_co_receive_request(NBDRequestData *req,
> > Error **errp)
> > {
> > NBDClient *client = req->client;
> > +bool extended_with_payload;
> > bool check_length = false;
> > bool check_rofs = false;
> > bool allocate_buffer = false;
> > +bool payload_okay = false;
> > unsigned payload_len = 0;
> > int valid_flags = NBD_CMD_FLAG_FUA;
> > int ret;
> > @@ -2338,6 +2340,13 @@ static int coroutine_fn
> > nbd_co_receive_request(NBDRequestData *req,
> >
> > trace_nbd_co_receive_request_decode_type(request->cookie,
> > request->type,
> >
> > nbd_cmd_lookup(request->type));
> > +extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
> > +request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
> > +if (extended_with_payload) {
> > +payload_len = request->len;
> > +check_length = true;
> > +}
> > +
> > switch (request->type) {
> > case NBD_CMD_DISC:
> > /* Special case: we're going to disconnect without a reply,
> > @@ -2354,6 +2363,15 @@ static int coroutine_fn
> > nbd_co_receive_request(NBDRequestData *req,
> > break;
> >
> > case NBD_CMD_WRITE:
> > +if (client->mode >= NBD_MODE_EXTENDED) {
> > +if (!extended_with_payload) {
> > +/* The client is noncompliant. Trace it, but proceed. */
> > +trace_nbd_co_receive_ext_payload_compliance(request->from,
> > +request->len);
> > +}
> > +valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> > +}
> > +payload_okay = true;
> > payload_len = request->len;
> > check_length = true;
> > allocate_buffer = true;
> > @@ -2395,6 +2413,14 @@ static int coroutine_fn
> > nbd_co_receive_request(NBDRequestData *req,
> > request->len, NBD_MAX_BUFFER_SIZE);
> > return -EINVAL;
> > }
> > +if (payload_len && !payload_okay) {
> > +/*
> > + * For now, we don't support payloads on other commands; but
> > + * we can keep the connection alive by ignoring the payload.
> > + */
> > +assert(request->type != NBD_CMD_WRITE);
> > +request->len = 0;
>
> So, actually we handle a syntactic request with len=0 and return success...
> I'm afraid, that in the most scenarios that would not be what client want,
> but client will be confused by success return.
If
Re: [Libguestfs] LIBNBD SECURITY: Negative results from nbd_get_size() - CVE-2023-5215
On Tue, Sep 26, 2023 at 02:12:27PM -0500, Eric Blake wrote: > We have discovered a security flaw with potential minor impact in > libnbd. > > Lifecycle > - > > Reported: 2023-09-17 Fixed: 2023-09-22 Published: 2023-09-26 > > At the time of this email, the Red Hat security team is analyzing > potential security impacts to determine if a CVE is warranted against > libnbd; if one is assigned, a followup email will announce that > identifier. However, even if a CVE is not assigned to libnbd, the > issues documented here warrant an audit of clients that utilize the > nbd_get_size() API from libnbd, to see if they might be subject to a > weakness when interpreting a large size as a negative value. The > libnbd developers felt it more important to issue this security notice > prior to the release of v1.18 than to hold up the release schedule > waiting for final analysis on whether libnbd needs a CVE. The Red Hat security team assigned this CVE-2023-5215 as a low-impact security vulnerability, with a rating of low impact severity. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] ANNOUNCE: nbdkit 1.36 and libnbd 1.18 released
I'm pleased to announce new stable releases of nbdkit 1.36 and libnbd 1.18. nbdkit is a Network Block Device (NBD) server with a stable plugin ABI and a permissive license. libnbd is an NBD client library in userspace. Among the major new features are: Rust bindings for libnbd, 64 bit extended headers (NBD protocol extension) support in libnbd, a completely rewritten curl plugin for nbdkit, qcow2 support for nbdkit. Complete release notes are attached below. nbdkit 1.36.0 can be downloaded here: https://download.libguestfs.org/nbdkit/1.36-stable/ libnbd 1.18.0 can be downloaded here: https://download.libguestfs.org/libnbd/1.18-stable/ Release notes for nbdkit 1.36 online: https://libguestfs.org/nbdkit-release-notes-1.36.1.html Release notes for libnbd 1.18 online: https://libguestfs.org/libnbd-release-notes-1.18.1.html Rich. -- nbdkit-release-notes-1.36 - release notes for nbdkit 1.36 These are the release notes for nbdkit stable release 1.36. This describes the major changes since 1.34. nbdkit 1.36.0 was released on 27 September 2023. Security No security issues were identified in this release. All past security issues and information about how to report new ones can be found in nbdkit-security(1). Plugins New nbdkit-ones-plugin(1) which creates a fully allocated disk containing all 0xff (all ones), or another byte of your choice. nbdkit-curl-plugin(1) now uses a curl "multi" interface. This enables much better performance, and also allows the curl plugin to handle requests in parallel. The curl plugin now falls back to making a "GET" request to get the size of the remote file for certain servers which do not support "HEAD" requests. This plugin adds new options: "ipresolve" (force IPv4 or IPv6), "resolve" (force a particular IP address), -D curl.times=1 (print detailed timing stats), and -D curl.verbose.ids=1 (display connection and transfer IDs). nbdkit-memory-plugin(1) now uses a read-write lock to protect internal structures, resulting in improved performance for mostly read workloads. nbdkit-data-plugin(1) now has more optimizations. nbdkit-file-plugin(1) now supports 4k sector sizes on Windows (Brian Carnes). Filters New nbdkit-evil-filter(1) adds random but consistent data corruption to the underlying plugin. New nbdkit-qcow2dec-filter(1) which can decode qcow2 files (but not write to them). nbdkit-ip-filter(1) can now filter by client SELinux label. nbdkit-partition-filter(1) now supports 4k sector sizes (Brian Carnes). nbdkit-retry-request-filter(1) allows the "get_size" operation to be retried. nbdkit-tar-filter(1) adds new "tar-limit" parameter which can be used to ensure the filter does not read indefinite amounts of input when opening the tar file. Filters can now append their own output to nbdkit --dump-plugin output. Language bindings Rust bindings add support for "after_fork", "block_size", "nbdkit_debug", "nbdkit_is_tls", "nbdkit_parse_size", "nbdkit_parse_bool" and "nbdkit_parse_probability". The "open" method can now return an error; note this is not backwards compatible and requires a small source code change to Rust plugins. (Thanks Alan Somers) nbdkit-ocaml-plugin(3) now supports OCaml 5. OCaml bindings add support for "nbdkit_stdio_safe", "nbdkit_is_tls", "nbdkit_peer_name" and "nbdkit_peer_security_context". nbdkit-perl-plugin(3) now supports Perl 5.38. Shell script plugins (nbdkit-sh-plugin(3)) may now ignore stdin in their "pwrite" method, whereas previously it was required to read and discard stdin along error paths (thanks Eric Blake). Server When using the --run option, the default is now to use a private Unix domain socket (as if -U - was specified), whereas in nbdkit ≤ 1.34 the default was to open a TCP port. The new default reflects the most common and safest way to use the --run option. You can find out if nbdkit has the new behaviour by checking "nbdkit --dump-config" and looking for "run_default_socket=Unix" in the output. Debug strings containing control codes and other non-printable characters are now escaped properly. New flag -D nbdkit.environ=1 can be used to dump the server environment in debug output. API New "nbdkit_parse_probability" function which can be used to parse probabilities in various formats, like "10%" or "1:10". New "nbdkit_peer_security_context" function which returns the security context (usually SELinux label) of the client. Bug fixes Fix long-standing double-free in
Re: [Libguestfs] [PATCH v7 06/12] nbd/client: Plumb errp through nbd_receive_replies
On 25.09.23 22:22, Eric Blake wrote: Instead of ignoring the low-level error just to refabricate our own message to pass to the caller, we can just plumb the caller's errp down to the low level. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v7 01/12] nbd/server: Support a request payload
On 25.09.23 22:22, Eric Blake wrote:
Upcoming additions to support NBD 64-bit effect lengths allow for the
possibility to distinguish between payload length (capped at 32M) and
effect length (64 bits, although we generally assume 63 bits because
of off_t limitations). Without that extension, only the NBD_CMD_WRITE
request has a payload; but with the extension, it makes sense to allow
at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
in a future patch (where the payload is a limited-size struct that in
turn gives the real effect length as well as a subset of known ids for
which status is requested). Other future NBD commands may also have a
request payload, so the 64-bit extension introduces a new
NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
length is a payload length or an effect length, rather than
hard-coding the decision based on the command.
According to the spec, a client should never send a command with a
payload without the negotiation phase proving such extension is
available. So in the unlikely event the bit is set or cleared
incorrectly, the client is already at fault; if the client then
provides the payload, we can gracefully consume it off the wire and
fail the command with NBD_EINVAL (subsequent checks for magic numbers
ensure we are still in sync), while if the client fails to send
payload we block waiting for it (basically deadlocking our connection
to the bad client, but not negatively impacting our ability to service
other clients, so not a security risk). Note that we do not support
the payload version of BLOCK_STATUS yet.
Signed-off-by: Eric Blake
---
v7: another try at better logic [Vladimir]
v5: retitled from v4 13/24, rewrite on top of previous patch's switch
statement [Vladimir]
v4: less indentation on several 'if's [Vladimir]
---
nbd/server.c | 37 +
nbd/trace-events | 1 +
2 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 7a6f95071f8..1eabcfc908d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2322,9 +2322,11 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
Error **errp)
{
NBDClient *client = req->client;
+bool extended_with_payload;
bool check_length = false;
bool check_rofs = false;
bool allocate_buffer = false;
+bool payload_okay = false;
unsigned payload_len = 0;
int valid_flags = NBD_CMD_FLAG_FUA;
int ret;
@@ -2338,6 +2340,13 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
nbd_cmd_lookup(request->type));
+extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
+request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
+if (extended_with_payload) {
+payload_len = request->len;
+check_length = true;
+}
+
switch (request->type) {
case NBD_CMD_DISC:
/* Special case: we're going to disconnect without a reply,
@@ -2354,6 +2363,15 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
break;
case NBD_CMD_WRITE:
+if (client->mode >= NBD_MODE_EXTENDED) {
+if (!extended_with_payload) {
+/* The client is noncompliant. Trace it, but proceed. */
+trace_nbd_co_receive_ext_payload_compliance(request->from,
+request->len);
+}
+valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+}
+payload_okay = true;
payload_len = request->len;
check_length = true;
allocate_buffer = true;
@@ -2395,6 +2413,14 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
request->len, NBD_MAX_BUFFER_SIZE);
return -EINVAL;
}
+if (payload_len && !payload_okay) {
+/*
+ * For now, we don't support payloads on other commands; but
+ * we can keep the connection alive by ignoring the payload.
+ */
+assert(request->type != NBD_CMD_WRITE);
+request->len = 0;
So, actually we handle a syntactic request with len=0 and return success... I'm
afraid, that in the most scenarios that would not be what client want, but
client will be confused by success return.
So, for example, if client pass READ with positive length and accidentlly set
NBD_CMD_FLAG_PAYLOAD_LEN bit, it will get successful result with wrong length=0.
Or, for WRITE_ZEROES (with accidental NBD_CMD_FLAG_PAYLOAD_LEN) it will just
get successful result, when actually nothing is done.
+}
if (allocate_buffer) {
/* READ, WRITE */
req->data = blk_try_blockalign(client->exp->common.blk,
@@ -2405,10 +2431,13 @@ static int coroutine_fn
