[Libguestfs] [PATCH libnbd 2/3] states: Prevent multi-conn connection unless the server advertizes it.

[Eric Blake]

On 5/23/19 4:32 AM, Richard W.M. Jones wrote:
> It is unsafe to connect using multi-conn to a server unless the server
> advertizes it, so fail if the caller tries to do this.

Actually, I think we can be a little bit looser:

It is unsafe to connect with write access using multi-conn.

But for read-only (where we can't corrupt data), having multiple
connections even when the server does not advertise multi-conn should
(in general) still be safe.

> ---
>  generator/generator | 16 ++++++++++++++--
>  lib/flags.c         | 10 ++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
> index a372074..a8e6fb6 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -897,7 +897,8 @@ The C<multi_conn> parameter controls whether this feature is
>  enabled (if E<gt> 1) or disabled (if C<1>).  The parameter
>  passed must not be C<0>.  Usually small powers of 2 (eg. 2, 4, 8)
>  will provide increments in performance.  Some servers do not
> -support this feature and will return an error on connection.
> +support this feature and libnbd will fail on connection to
> +these servers if this setting is E<gt> 1.

So maybe change this to: ...libnbd will fail on read-write connections
to these servers if...

>  
>  At present, libnbd assumes that all connections will report the
>  same capabilities; a server that behaves otherwise may trigger
> @@ -1171,7 +1172,18 @@ connected to and completed the handshake with the server.";
>  Returns true if the server supports multi-conn
>  (see C<nbd_set_multi_conn>).  Returns false if
>  the server does not.  Can return an error if we have not
> -connected to and completed the handshake with the server.";
> +connected to and completed the handshake with the server.
> +
> +Note that you I<cannot> set multi-conn on the handle
> +to E<gt> 1, connect, and then check this setting, because
> +if the server does not support multi-conn the connection
> +will fail.  Instead you should connect with multi-conn
> +set to C<1> (the default), check this setting, then if
> +multi-conn is found to be supported you can call
> +C<nbd_set_multi_conn> with a larger value to increase
> +the number of connection objects, then call one of the
> +C<nbd_connect*> functions again on the handle to connect
> +the remaining connections.";

This one is good.

>    };
>  
>    "can_cache", {
> diff --git a/lib/flags.c b/lib/flags.c
> index 825b326..1956db6 100644
> --- a/lib/flags.c
> +++ b/lib/flags.c
> @@ -42,6 +42,16 @@ nbd_internal_set_size_and_flags (struct nbd_handle *h,
>      return -1;
>    }
>  
> +  /* It is unsafe to connect to a server with multi-conn set unless
> +   * the server says it is safe to do so.
> +   */
> +  if (h->multi_conn > 1 && (eflags & NBD_FLAG_CAN_MULTI_CONN) == 0) {

Here, I'd add && !(eflags & NBD_FLAG_READ_ONLY)

> +    set_error (EINVAL, "handshake: multi-conn is set on this handle, "
> +               "but the server does not advertize multi-conn support "

I know that in general -ise vs. -ize is a common US/UK thing, but
https://english.stackexchange.com/questions/341707/should-advertised-be-spelled-with-a-z-in-american-english
says that advertise is preferred in both.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190523/7f7d1ab3/attachment.sig>