[Qemu-devel] [PATCH 0/3] Fix some ambiguities in the NBD protocol

[Qemu-devel] [PATCH 0/3] Fix some ambiguities in the NBD protocol

[Denis V. Lunev]

This patch set attempts to fix some ambiguities in the NBD protocol.

Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Wouter Verhelst <w@uter.be>
CC: Eric Blake <eblake@redhat.com>
CC: Alex Bligh <alex@alex.org.uk>

Pavel Borzenkov (3):
  NBD proto: forbid TRIM command without negotiation
  NBD proto: document additional error conditions
  NBD proto: add "Command flags" section

 doc/proto.md | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

[Qemu-devel] [PATCH 1/3] NBD proto: forbid TRIM command without negotiation

[Denis V. Lunev]

From: Pavel Borzenkov <pborzenkov@virtuozzo.com>

There is a loophole in the protocol that allows a client to send TRIM
request even if support for it wasn't negotiated with the server. State
explicitly that the client MUST NOT send such command without prior
successful negotiation.

Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Wouter Verhelst <w@uter.be>
CC: Eric Blake <eblake@redhat.com>
CC: Alex Bligh <alex@alex.org.uk>
---
 doc/proto.md | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/proto.md b/doc/proto.md
index 6d1cb34..d54ed19 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -471,6 +471,9 @@ The following request types exist:
     about the contents of the export affected by this command, until
     overwriting it again with `NBD_CMD_WRITE`.
 
+    A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM`
+    was set in the export flags field.
+
 * Other requests
 
     Some third-party implementations may require additional protocol
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/3] NBD proto: forbid TRIM command without negotiation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> 
> There is a loophole in the protocol that allows a client to send TRIM
> request even if support for it wasn't negotiated with the server. State
> explicitly that the client MUST NOT send such command without prior
> successful negotiation.
> 
> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Wouter Verhelst <w@uter.be>
> CC: Eric Blake <eblake@redhat.com>
> CC: Alex Bligh <alex@alex.org.uk>
> ---
>  doc/proto.md | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 6d1cb34..d54ed19 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -471,6 +471,9 @@ The following request types exist:
>      about the contents of the export affected by this command, until
>      overwriting it again with `NBD_CMD_WRITE`.
>  
> +    A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM`
> +    was set in the export flags field.
> +

Do we also want to mention that the server SHOULD fail with EINVAL if
the client sends it anyway, and similarly if NBD_CMD_FLUSH was sent
without the appropriate export flag (but that the client should not rely
on that particular failure)?

But as this is a strict improvement,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Nbd] [PATCH 1/3] NBD proto: forbid TRIM command without negotiation

[Wouter Verhelst]

On Mon, Mar 28, 2016 at 07:00:17AM -0600, Eric Blake wrote:
> On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> > From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> > 
> > There is a loophole in the protocol that allows a client to send TRIM
> > request even if support for it wasn't negotiated with the server. State
> > explicitly that the client MUST NOT send such command without prior
> > successful negotiation.
> > 
> > Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Wouter Verhelst <w@uter.be>
> > CC: Eric Blake <eblake@redhat.com>
> > CC: Alex Bligh <alex@alex.org.uk>
> > ---
> >  doc/proto.md | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/doc/proto.md b/doc/proto.md
> > index 6d1cb34..d54ed19 100644
> > --- a/doc/proto.md
> > +++ b/doc/proto.md
> > @@ -471,6 +471,9 @@ The following request types exist:
> >      about the contents of the export affected by this command, until
> >      overwriting it again with `NBD_CMD_WRITE`.
> >  
> > +    A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM`
> > +    was set in the export flags field.
> > +
> 
> Do we also want to mention that the server SHOULD fail with EINVAL if
> the client sends it anyway, and similarly if NBD_CMD_FLUSH was sent
> without the appropriate export flag (but that the client should not rely
> on that particular failure)?

I think the protocol should mention that the server MAY fail with
EINVAL, rather than SHOULD. Rationale: the robusness principle -- if you
didn't negotiate it, you may end up with a server who doesn't know about
the feature; but if it just so happens that the server does know about it even
though you didn't negotiate it, there is little harm in it following up on the
request.

> But as this is a strict improvement,
> Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [Nbd] [PATCH 1/3] NBD proto: forbid TRIM command without negotiation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On 03/29/2016 01:22 AM, Wouter Verhelst wrote:

>>> +++ b/doc/proto.md
>>> @@ -471,6 +471,9 @@ The following request types exist:
>>>      about the contents of the export affected by this command, until
>>>      overwriting it again with `NBD_CMD_WRITE`.
>>>  
>>> +    A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM`
>>> +    was set in the export flags field.
>>> +
>>
>> Do we also want to mention that the server SHOULD fail with EINVAL if
>> the client sends it anyway, and similarly if NBD_CMD_FLUSH was sent
>> without the appropriate export flag (but that the client should not rely
>> on that particular failure)?
> 
> I think the protocol should mention that the server MAY fail with
> EINVAL, rather than SHOULD. Rationale: the robusness principle -- if you
> didn't negotiate it, you may end up with a server who doesn't know about
> the feature; but if it just so happens that the server does know about it even
> though you didn't negotiate it, there is little harm in it following up on the
> request.

Good point; furthermore, a server is compliant if it accepts and ignores
NBD_CMD_TRIM (as that command is only a hint); which is different from
NBD_CMD_FLUSH (which must ensure a barrier).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/3] NBD proto: document additional error conditions

[Denis V. Lunev]

From: Pavel Borzenkov <pborzenkov@virtuozzo.com>

It is unclear what the behaviour of a server should be if it receives
an unknown command. Similar uncertainty exists for command flags.

Make it explicit that the server should return EINVAL in all such cases.

Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Wouter Verhelst <w@uter.be>
CC: Eric Blake <eblake@redhat.com>
CC: Alex Bligh <alex@alex.org.uk>
---
 doc/proto.md | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/proto.md b/doc/proto.md
index d54ed19..036d6d9 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -512,6 +512,13 @@ return `EINVAL` if it receives a read or trim request including one or
 more sectors beyond the size of the device.  It also SHOULD map the
 `EDQUOT` and `EFBIG` errors to `ENOSPC`.  Finally, it SHOULD return
 `EPERM` if it receives a write or trim request on a read-only export.
+
+The server SHOULD return `EINVAL` if it receives an unknown command.
+
+The server SHOULD return `EINVAL` if it receives an unknown command flag. It
+also SHOULD return `EINVAL` if it receives a request with a flag not explicitly
+documented as applicable to the given request.
+
 Which error to return in any other case is not specified by the NBD
 protocol.
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/3] NBD proto: document additional error conditions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> 
> It is unclear what the behaviour of a server should be if it receives
> an unknown command. Similar uncertainty exists for command flags.
> 
> Make it explicit that the server should return EINVAL in all such cases.

I'd go one step further and document that for backwards compatibility,
clients should not rely on this particular error.

> 
> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Wouter Verhelst <w@uter.be>
> CC: Eric Blake <eblake@redhat.com>
> CC: Alex Bligh <alex@alex.org.uk>
> ---
>  doc/proto.md | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index d54ed19..036d6d9 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -512,6 +512,13 @@ return `EINVAL` if it receives a read or trim request including one or
>  more sectors beyond the size of the device.  It also SHOULD map the
>  `EDQUOT` and `EFBIG` errors to `ENOSPC`.  Finally, it SHOULD return
>  `EPERM` if it receives a write or trim request on a read-only export.
> +
> +The server SHOULD return `EINVAL` if it receives an unknown command.
> +
> +The server SHOULD return `EINVAL` if it receives an unknown command flag. It
> +also SHOULD return `EINVAL` if it receives a request with a flag not explicitly
> +documented as applicable to the given request.

In other words, while this is good for the server side, we have proven
that existing server implementations did not follow this, and therefore
it would probably be good to add a sentence along the lines of:

However, clients should not rely on this particular error, as some
historical servers silently ignored invalid commands or flags.

But as what you have proposed is a strict improvement,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 3/3] NBD proto: add "Command flags" section

[Denis V. Lunev]

From: Pavel Borzenkov <pborzenkov@virtuozzo.com>

Add separate "Command flags" section to make it clear which flags are
currently defined by the protocol.

Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Wouter Verhelst <w@uter.be>
CC: Eric Blake <eblake@redhat.com>
CC: Alex Bligh <alex@alex.org.uk>
---
 doc/proto.md | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/doc/proto.md b/doc/proto.md
index 036d6d9..662f741 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -485,6 +485,16 @@ The following request types exist:
     Currently one such message is known: `NBD_CMD_CACHE`, with type set to
     5, implemented by xnbd.
 
+#### Command flags
+
+This field of 16 bits is sent by the client with every request and provides
+additional information to the server to execute the command. Refer to
+aforementioned "Request types" section for information about the flags
+supported by particular commands.
+
+- bit 0, `NBD_CMD_FLAG_FUA`; should be set to 1 if the client requires
+  "Force Unit Access" mode of operation
+
 #### Error values
 
 The error values are used for the error field in the reply message.
-- 
2.1.4

Re: [Qemu-devel] [PATCH 3/3] NBD proto: add "Command flags" section

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> 
> Add separate "Command flags" section to make it clear which flags are
> currently defined by the protocol.
> 
> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Wouter Verhelst <w@uter.be>
> CC: Eric Blake <eblake@redhat.com>
> CC: Alex Bligh <alex@alex.org.uk>
> ---
>  doc/proto.md | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 036d6d9..662f741 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -485,6 +485,16 @@ The following request types exist:
>      Currently one such message is known: `NBD_CMD_CACHE`, with type set to
>      5, implemented by xnbd.
>  
> +#### Command flags
> +
> +This field of 16 bits is sent by the client with every request and provides
> +additional information to the server to execute the command. Refer to
> +aforementioned "Request types" section for information about the flags

Maybe:

s/aforementioned "Request types" section/the "Request types" section above/

> +supported by particular commands.
> +
> +- bit 0, `NBD_CMD_FLAG_FUA`; should be set to 1 if the client requires
> +  "Force Unit Access" mode of operation

Trailing dot?  Should you also mention which command(s) it is valid
with? (NBD_CMD_WRITE for now, until other extension commands are added)
 It might also be worth mentioning that the flag should not be sent
unless export flags included NBD_FLAG_SEND_FUA.

> +
>  #### Error values
>  
>  The error values are used for the error field in the reply message.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Nbd] [PATCH 3/3] NBD proto: add "Command flags" section

[Wouter Verhelst]

On Mon, Mar 28, 2016 at 07:45:27AM -0600, Eric Blake wrote:
> On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> > From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> > 
> > Add separate "Command flags" section to make it clear which flags are
> > currently defined by the protocol.
> > 
> > Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Wouter Verhelst <w@uter.be>
> > CC: Eric Blake <eblake@redhat.com>
> > CC: Alex Bligh <alex@alex.org.uk>
> > ---
> >  doc/proto.md | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/doc/proto.md b/doc/proto.md
> > index 036d6d9..662f741 100644
> > --- a/doc/proto.md
> > +++ b/doc/proto.md
> > @@ -485,6 +485,16 @@ The following request types exist:
> >      Currently one such message is known: `NBD_CMD_CACHE`, with type set to
> >      5, implemented by xnbd.
> >  
> > +#### Command flags
> > +
> > +This field of 16 bits is sent by the client with every request and provides
> > +additional information to the server to execute the command. Refer to
> > +aforementioned "Request types" section for information about the flags
> 
> Maybe:
> 
> s/aforementioned "Request types" section/the "Request types" section above/
> 
> > +supported by particular commands.
> > +
> > +- bit 0, `NBD_CMD_FLAG_FUA`; should be set to 1 if the client requires
> > +  "Force Unit Access" mode of operation
> 
> Trailing dot?  Should you also mention which command(s) it is valid
> with? (NBD_CMD_WRITE for now, until other extension commands are added)
>  It might also be worth mentioning that the flag should not be sent
> unless export flags included NBD_FLAG_SEND_FUA.
> 
> > +
> >  #### Error values
> >  
> >  The error values are used for the error field in the reply message.

Yes, I agree that these are all (typographical, but still) improvements.
If you can update with that, I'll happily apply that.

Regards,

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [PATCH 3/3] NBD proto: add "Command flags" section

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> 
> Add separate "Command flags" section to make it clear which flags are
> currently defined by the protocol.
> 
> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Wouter Verhelst <w@uter.be>
> CC: Eric Blake <eblake@redhat.com>
> CC: Alex Bligh <alex@alex.org.uk>
> ---
>  doc/proto.md | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 036d6d9..662f741 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -485,6 +485,16 @@ The following request types exist:
>      Currently one such message is known: `NBD_CMD_CACHE`, with type set to
>      5, implemented by xnbd.
>  
> +#### Command flags
> +

I think that this new content would belong better as a subsection under
'#### Flag Fields', alongside the mention of all other flags.  I'm going
to propose a v2 of this patch with that alternate position, for comparison.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/3] NBD proto: add "Command flags" section

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

On 03/29/2016 10:01 AM, Eric Blake wrote:
> On 03/28/2016 04:43 AM, Denis V. Lunev wrote:
>> From: Pavel Borzenkov <pborzenkov@virtuozzo.com>
>>
>> Add separate "Command flags" section to make it clear which flags are
>> currently defined by the protocol.
>>
>> Signed-off-by: Pavel Borzenkov <pborzenkov@virtuozzo.com>
>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Wouter Verhelst <w@uter.be>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Alex Bligh <alex@alex.org.uk>
>> ---
>>  doc/proto.md | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/doc/proto.md b/doc/proto.md
>> index 036d6d9..662f741 100644
>> --- a/doc/proto.md
>> +++ b/doc/proto.md
>> @@ -485,6 +485,16 @@ The following request types exist:
>>      Currently one such message is known: `NBD_CMD_CACHE`, with type set to
>>      5, implemented by xnbd.
>>  
>> +#### Command flags
>> +
> 
> I think that this new content would belong better as a subsection under
> '#### Flag Fields', alongside the mention of all other flags.  I'm going
> to propose a v2 of this patch with that alternate position, for comparison.

Hmm, maybe not.  I just looked again, and '#### Flag fields' is a
subsection of '### Handshake phase', while you are correct that command
flags belong to '### Transmission phase'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]