[1/3] NBD proto: forbid TRIM command without negotiation
diff mbox

Message ID 1459161798-32120-2-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev March 28, 2016, 10:43 a.m. UTC
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(+)

Comments

Eric Blake March 28, 2016, 1 p.m. UTC | #1
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>
Wouter Verhelst March 29, 2016, 7:22 a.m. UTC | #2
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>
Eric Blake March 29, 2016, 1:54 p.m. UTC | #3
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).

Patch
diff mbox

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