[Libguestfs] [libnbd PATCH] copy: Fail nbdcopy if NBD read or write fails
Nir Soffer
nsoffer at redhat.com
Thu Feb 3 12:46:27 UTC 2022
On Thu, Feb 3, 2022 at 3:51 AM Eric Blake <eblake at redhat.com> wrote:
>
> FIXME: This is CVE-2022-XXXXX (still awaiting assignment of the CVE number).
>
> nbdcopy has a nasty bug when performing multi-threaded copies using
> asynchronous nbd calls - it was blindly treating the completion of an
> asynchronous command as successful, rather than checking the *error
> parameter. This can result in the silent creation of a corrupted
> image in two different ways: when a read fails, we blindly wrote
> garbage to the destination; when a write fails, we did not flag that
> the destination was not written.
Writing zeroes is third failure case
>
> Since nbdcopy already calls exit() on a synchronous read or write
> failure to a file, doing the same for an asynchronous op to an NBD
> server is the simplest solution. A nicer solution, but more invasive
> to write and thus not done here, might be to allow up to N retries of
> the transaction (in case the read or write failure was transient), or
> even having a mode where as much data is copied as possible (portions
> of the copy that failed would be logged on stderr, and nbdcopy would
> still fail with a non-zero exit status, but this would copy more than
> just stopping at the first error).
I think failing on errors is good enough, and we need minimal change
that will be easy to backport.
>
> Note that since we rely on auto-retiring and do NOT call
> nbd_aio_command_completed, our completion callbacks must always return
> 1 (if they do not exit() first), even when acting on *error. As such,
> there is no sane way to return an error to a manual caller of the
> callback, and therefore we can drop dead code that exit()s if the
> callback "fails". It is also worth documenting the contract on when
> we must manually call the callback during the asynch_zero callback, so
> that we do not leak or double-free the command.
That's a nice side effect of the fix.
> And since we are now exit()ing in the callback if *error is set, we
> must be careful that we do not leak an unknown value of errno on paths
> where we did not encounter a failure.
>
> Reported-by: Nir Soffer <nsoffer at redhat.com>
> Fixes: bc896eec4d ("copy: Implement multi-conn, multiple threads, multiple requests in flight.", v1.5.6)
> Fixes: https://bugzilla.redhat.com/2046194
> ---
>
> Once the CVE is assigned, I will also update docs/libnbd-security.pod,
> and backport this to affected stable branches.
>
> I'm not sure if I like passing &errno in the manual calls to
> cb.callback(); better might be 'int dummy = 0; cb.callback(...,
> &dummy)', but changing that could be a separate patch.
I agree.
> I verified that the copy-nbd-error.sh test fails (in both the read and
> write directions, by modifying the filter arguments) without this
> patch.
>
> TODO | 1 +
> copy/Makefile.am | 4 ++-
> copy/copy-nbd-error.sh | 54 +++++++++++++++++++++++++++++++++++++
> copy/file-ops.c | 21 +++++----------
> copy/multi-thread-copying.c | 18 ++++++++++++-
> copy/nbdcopy.h | 7 ++---
> copy/null-ops.c | 12 +++------
> 7 files changed, 88 insertions(+), 29 deletions(-)
> create mode 100755 copy/copy-nbd-error.sh
>
> diff --git a/TODO b/TODO
> index da157942..7c9c15e2 100644
> --- a/TODO
> +++ b/TODO
> @@ -33,6 +33,7 @@ nbdcopy:
> - Better page cache usage, see nbdkit-file-plugin options
> fadvise=sequential cache=none.
> - Consider io_uring if there are performance bottlenecks.
> + - Configurable retries in response to read or write failures.
>
> nbdfuse:
> - If you write beyond the end of the virtual file, it returns EIO.
> diff --git a/copy/Makefile.am b/copy/Makefile.am
> index f2100853..85989798 100644
> --- a/copy/Makefile.am
> +++ b/copy/Makefile.am
> @@ -1,5 +1,5 @@
> # nbd client library in userspace
> -# Copyright (C) 2020 Red Hat Inc.
> +# Copyright (C) 2020-2022 Red Hat Inc.
> #
> # This library is free software; you can redistribute it and/or
> # modify it under the terms of the GNU Lesser General Public
> @@ -33,6 +33,7 @@ EXTRA_DIST = \
> copy-nbd-to-small-nbd-error.sh \
> copy-nbd-to-sparse-file.sh \
> copy-nbd-to-stdout.sh \
> + copy-nbd-error.sh \
> copy-progress-bar.sh \
> copy-sparse.sh \
> copy-sparse-allocated.sh \
> @@ -124,6 +125,7 @@ TESTS += \
> copy-stdin-to-nbd.sh \
> copy-stdin-to-null.sh \
> copy-nbd-to-stdout.sh \
> + copy-nbd-error.sh \
> copy-progress-bar.sh \
> copy-sparse.sh \
> copy-sparse-allocated.sh \
> diff --git a/copy/copy-nbd-error.sh b/copy/copy-nbd-error.sh
> new file mode 100755
> index 00000000..50723195
> --- /dev/null
> +++ b/copy/copy-nbd-error.sh
> @@ -0,0 +1,54 @@
> +#!/usr/bin/env bash
> +# nbd client library in userspace
> +# Copyright (C) 2022 Red Hat Inc.
> +#
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2 of the License, or (at your option) any later version.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> +
> +. ../tests/functions.sh
> +
> +set -e
> +set -x
> +
> +requires nbdkit --exit-with-parent --version
> +
> +pidfile=copy-nbd-error.pid
> +sock=$(mktemp -u /tmp/libnbd-test-copy.XXXXXX)
> +cleanup_fn rm -f $pidfile $sock
> +
> +# Run an nbdkit server that randomly fails.
> +nbdkit --exit-with-parent -f -v -P $pidfile -U $sock \
> + --filter=error --filter=noextents \
> + memory size=5M error-pread-rate=0.5 error-pwrite-rate=0.5 &
What about write_zeroes errors?
> +# Wait for the pidfile to appear.
> +for i in {1..60}; do
> + if test -f $pidfile; then
> + break
> + fi
> + sleep 1
> +done
> +if ! test -f $pidfile; then
> + echo "$0: nbdkit did not start up"
> + exit 1
> +fi
> +
> +fail=0
> +
> +# Failure to read should be fatal
> +$VG nbdcopy -- "nbd+unix:///?socket=$sock" null: && fail=1
> +
> +# Failure to write should be fatal
> +$VG nbdcopy -- [ nbdkit --exit-with-parent -v pattern 5M ] "nbd+unix:///?socket=$sock" && fail=1
> +
> +exit $fail
> diff --git a/copy/file-ops.c b/copy/file-ops.c
> index 84704341..eb30f924 100644
> --- a/copy/file-ops.c
> +++ b/copy/file-ops.c
> @@ -1,5 +1,5 @@
> /* NBD client library in userspace.
> - * Copyright (C) 2020 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -589,11 +589,8 @@ file_asynch_read (struct rw *rw,
> {
> file_synch_read (rw, slice_ptr (command->slice),
> command->slice.len, command->offset);
> - errno = 0;
> - if (cb.callback (cb.user_data, &errno) == -1) {
> - perror (rw->name);
> - exit (EXIT_FAILURE);
> - }
> + errno = 0; /* safe, since file_synch_read exits on error */
> + cb.callback (cb.user_data, &errno);
Much nicer - should we assert that the callback returns 1?
file-ops does not care about the return value but it will help
to detect errors in code using nbd. We cannot assert about this
in nbd-ops since the call returns to libnbd.
> }
>
> static void
> @@ -603,11 +600,8 @@ file_asynch_write (struct rw *rw,
> {
> file_synch_write (rw, slice_ptr (command->slice),
> command->slice.len, command->offset);
> - errno = 0;
> - if (cb.callback (cb.user_data, &errno) == -1) {
> - perror (rw->name);
> - exit (EXIT_FAILURE);
> - }
> + errno = 0; /* safe, since file_synch_write exits on error */
> + cb.callback (cb.user_data, &errno);
> }
>
> static bool
> @@ -617,10 +611,7 @@ file_asynch_zero (struct rw *rw, struct command *command,
> if (!file_synch_zero (rw, command->offset, command->slice.len, allocate))
> return false;
> errno = 0;
> - if (cb.callback (cb.user_data, &errno) == -1) {
> - perror (rw->name);
> - exit (EXIT_FAILURE);
> - }
> + cb.callback (cb.user_data, &errno);
> return true;
> }
>
> diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
> index b17ca598..c578c4bc 100644
> --- a/copy/multi-thread-copying.c
> +++ b/copy/multi-thread-copying.c
> @@ -1,5 +1,5 @@
> /* NBD client library in userspace.
> - * Copyright (C) 2020 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -376,6 +376,13 @@ finished_read (void *vp, int *error)
> {
> struct command *command = vp;
>
> + /* XXX - is it worth retrying a failed command? */
> + if (*error) {
> + errno = *error;
> + perror("read");
> + exit (EXIT_FAILURE);
> + }
Setting errno to use perror is clever, but I think in future we would
like to exit with a more detailed message, like:
nbdcopy: read failed at offset 44040192: Input output error
So we would use fprintf and strerror, and setting errno would be needed
although it can be used with %m (is it portable?).
For now I think this is fine, we want a minimal change for easy backport.
> +
> if (allocated || sparse_size == 0) {
> /* If sparseness detection (see below) is turned off then we write
> * the whole command.
> @@ -475,6 +482,7 @@ finished_read (void *vp, int *error)
> /* Free the original command since it has been split into
> * subcommands and the original is no longer needed.
> */
> + errno = 0;
> free_command (command, &errno);
Using free_command as a callback is ugly, but good enough for now.
> }
>
> @@ -537,6 +545,7 @@ fill_dst_range_with_zeroes (struct command *command)
> free (data);
>
> free_and_return:
> + errno = 0;
> free_command (command, &errno);
> }
>
> @@ -546,6 +555,13 @@ free_command (void *vp, int *error)
> struct command *command = vp;
> struct buffer *buffer = command->slice.buffer;
>
> + /* XXX - is it worth retrying a failed command? */
> + if (*error) {
> + errno = *error;
> + perror("write");
> + exit (EXIT_FAILURE);
> + }
> +
> if (buffer != NULL) {
> if (--buffer->refs == 0) {
> free (buffer->data);
> diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
> index e7fe1eab..c070f8d7 100644
> --- a/copy/nbdcopy.h
> +++ b/copy/nbdcopy.h
> @@ -1,5 +1,5 @@
> /* NBD client library in userspace.
> - * Copyright (C) 2020-2021 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -157,7 +157,8 @@ struct rw_ops {
> bool allocate);
>
> /* Asynchronous I/O operations. These start the operation and call
> - * 'cb' on completion.
> + * 'cb' on completion. 'cb' will return 1, for auto-retiring with
> + * asynchronous libnbd calls.
> *
> * The file_ops versions are actually implemented synchronously, but
> * still call 'cb'.
> @@ -173,7 +174,7 @@ struct rw_ops {
> nbd_completion_callback cb);
>
> /* Asynchronously zero. command->slice.buffer is not used. If not possible,
> - * returns false.
> + * returns false. 'cb' must be called only if returning true.
> */
> bool (*asynch_zero) (struct rw *rw, struct command *command,
> nbd_completion_callback cb, bool allocate);
> diff --git a/copy/null-ops.c b/copy/null-ops.c
> index a38666d6..924eaf1e 100644
> --- a/copy/null-ops.c
> +++ b/copy/null-ops.c
> @@ -1,5 +1,5 @@
> /* NBD client library in userspace.
> - * Copyright (C) 2020-2021 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -118,10 +118,7 @@ null_asynch_write (struct rw *rw,
> nbd_completion_callback cb)
> {
> errno = 0;
> - if (cb.callback (cb.user_data, &errno) == -1) {
> - perror (rw->name);
> - exit (EXIT_FAILURE);
> - }
> + cb.callback (cb.user_data, &errno);
> }
>
> static bool
> @@ -129,10 +126,7 @@ null_asynch_zero (struct rw *rw, struct command *command,
> nbd_completion_callback cb, bool allocate)
> {
> errno = 0;
> - if (cb.callback (cb.user_data, &errno) == -1) {
> - perror (rw->name);
> - exit (EXIT_FAILURE);
> - }
> + cb.callback (cb.user_data, &errno);
> return true;
> }
>
> --
> 2.34.1
Would be good to test also writing zeroes, but otherwise the fix is complete.
Reviewed-by: Nir Soffer <nsoffer at redhat.com>
More information about the Libguestfs
mailing list