Re: [PATCH] spec: Add NBD_OPT_EXTENDED_HEADERS
[Updating Vladimir's new preferred address in cc list]
On Thu, Mar 24, 2022 at 07:31:48PM +0200, Wouter Verhelst wrote:
> Hi Eric,
>
> Thanks for the ping; it had slipped my mind.
>
> On Fri, Dec 03, 2021 at 05:14:34PM -0600, Eric Blake wrote:
> > #### Request message
> >
> > -The request message, sent by the client, looks as follows:
> > +The compact request message, sent by the client when extended
> > +transactions are not negotiated using `NBD_OPT_EXTENDED_HEADERS`,
> > +looks as follows:
> >
> > C: 32 bits, 0x25609513, magic (`NBD_REQUEST_MAGIC`)
> > C: 16 bits, command flags
> > @@ -353,14 +370,26 @@ C: 64 bits, offset (unsigned)
> > C: 32 bits, length (unsigned)
> > C: (*length* bytes of data if the request is of type `NBD_CMD_WRITE`)
> >
> > +If negotiation agreed on extended transactions with
> > +`NBD_OPT_EXTENDED_HEADERS`, the client instead uses extended requests:
> > +
> > +C: 32 bits, 0x21e41c71, magic (`NBD_REQUEST_EXT_MAGIC`)
> > +C: 16 bits, command flags
> > +C: 16 bits, type
> > +C: 64 bits, handle
> > +C: 64 bits, offset (unsigned)
> > +C: 64 bits, length (unsigned)
> > +C: (*length* bytes of data if the request is of type `NBD_CMD_WRITE`)
> > +
>
> Perhaps we should decouple the ideas of "effect length" and "payload
> length"? As in,
>
> C: 32 bits, 0x21e41c71, magic (`NBD_REQUEST_EXT_MAGIC`)
> C: 16 bits, command flags
> C: 16 bits, type
> C: 64 bits, handle
> C: 64 bits, offset
> C: 64 bits, effect length
> C: 64 bits, payload length
> C: (*payload length* bytes of data)
>
> This makes the protocol more context free. With the current set of
> commands, only NBD_CMD_WRITE would have payload length be nonzero, but
> that doesn't have to remain the case forever; e.g., we could have a
> command that extends NBD_CMD_BLOCK_STATUS to only query a subset of the
> metadata contexts that we declared (if that is wanted, of course).
>
> Of course, that does have the annoying side effect of no longer fitting
> in 32 bytes, requiring a 40-byte header instead. I think it would be
> worth it though.
Could we still keep a 32-byte header, by having a new command (or new
command flag to the existing NBD_CMD_BLOCK_STATUS), such that the
payload itself contains the needed extra bytes?
Hmm - right now, the only command with a payload is NBD_CMD_WRITE, and
all other commands use the length field as an effect length. So maybe
what we do is have a single command flag that says whether the length
field is serving as payload length or as effect length. NBD_CMD_WRITE
would always set the new flag (if extended headers were negotiated),
and most other NBD_CMD_* would leave the flag unset, but in the case
of BLOCK_STATUS wanting only a subset of id status reported, we could
then have:
HEADER:
C: 32 bits, 0x21e41c71, magic (`NBD_REQUEST_EXT_MAGIC`)
C: 16 bits, command flags, NBD_CMD_FLAG_PAYLOAD
C: 16 bits, type, NBD_CMD_BLOCK_STATUS
C: 64 bits, handle
C: 64 bits, offset
C: 64 bits, payload length = 12 + 4*n
PAYLOAD:
C: 64 bits, effect length (hint on desired range)
C: 32 bits, number of ids = n
C: 32 bits, id[0]
...
C: 32 bits, id[n-1]
vs.
HEADER:
C: 32 bits, 0x21e41c71, magic (`NBD_REQUEST_EXT_MAGIC`)
C: 16 bits, command flags, 0
C: 16 bits, type, NBD_CMD_BLOCK_STATUS
C: 64 bits, handle
C: 64 bits, offset
C: 64 bits, effect length (hint on desired range)
HEADER:
C: 32 bits, 0x21e41c71, magic (`NBD_REQUEST_EXT_MAGIC`)
C: 16 bits, command flags, NBD_CMD_FLAG_PAYLOAD
C: 16 bits, type, NBD_CMD_WRITE
C: 64 bits, handle
C: 64 bits, offset
C: 64 bits, payload length = n
PAYLOAD:
C: n*8 bits data
>
> (This is obviously not relevant for reply messages, only for request
> messages)
Staying at a power of 2 may still be worth the expense of a new cmd
flag which must always be set for writes when extended headers are in
use.
>
> > #### Simple reply message
> >
> > The simple reply message MUST be sent by the server in response to all
> > requests if structured replies have not been negotiated using
> > -`NBD_OPT_STRUCTURED_REPLY`. If structured replies have been negotiated, a simple
> > -reply MAY be used as a reply to any request other than `NBD_CMD_READ`,
> > -but only if the reply has no data payload. The message looks as
> > -follows:
> > +`NBD_OPT_STRUCTURED_REPLY`. If structured replies have been
> > +negotiated, a simple reply MAY be used as a reply to any request other
> > +than `NBD_CMD_READ`, but only if the reply has no data payload. If
> > +extended headers were not negotiated using `NBD_OPT_EXTENDED_HEADERS`,
> > +the message looks as follows:
> >
> > S: 32 bits, 0x67446698, magic (`NBD_SIMPLE_REPLY_MAGIC`; used to be
> > `NBD_REPLY_MAGIC`)
> > @@ -369,6 +398,16 @@ S: 64 bits, handle
> > S: (*length* bytes of data if the request is of type `NBD_CMD_READ` and
> > *error* is zero)
> >
> > +If extended headers were negotiated using `NBD_OPT_EXTENDED_HEADERS`,
> > +the message looks like:
> > +
> > +S: 32 bits, 0x60d12fd6, magic (`NBD_SIMPLE_REPLY_EXT_MAGIC`)
> > +S: 32 bits, error (MAY be zero)
> > +S: 64 bits, handle
> > +S: 128 bits, padding (MUST be zero)
>
> Should all these requirements about padding not be a SHOULD rather than
> a MUST?
Elsewhere in the thread, we talked about having
NBD_SIMPLE_REPLY_EXT_MAGIC have 64 bits length (only non-zero when
replying to NBD_CMD_READ) and 64 bits pad, instead of 128 bits pad.
For future extensibility, it's probably safest to require the server
to send 0 bits in the pad now, so that we can use them later. Should
clients then ignore unknown padding bits, or is there a risk that a
future definition of non-zero values in what is now padding bits may
confuse an existing client that merely ignores those bits?
If we don't think extensibility is needed, then using SHOULD instead
of MUST means a non-careful server can leak data through the padding.
But it is certainly less restrictive to use SHOULD instead of MUST
(well-written servers won't leak, sloppy servers might, clients must
ignore the padding, and extension is not possible because of sloppy
servers).
>
> [...]
> > +* `NBD_OPT_EXTENDED_HEADERS` (11)
> > +
> > + The client wishes to use extended headers during the transmission
> > + phase. The client MUST NOT send any additional data with the
> > + option, and the server SHOULD reject a request that includes data
> > + with `NBD_REP_ERR_INVALID`.
> > +
> > + The server replies with the following, or with an error permitted
> > + elsewhere in this document:
> > +
> > + - `NBD_REP_ACK`: Extended headers have been negotiated; the client
> > + MUST use the 32-byte extended request header, and the server
> > + MUST use the 32-byte extended reply header.
> > + - For backwards compatibility, clients SHOULD be prepared to also
> > + handle `NBD_REP_ERR_UNSUP`; in this case, only the compact
> > + transmission headers will be used.
> > +
> > + If the client requests `NBD_OPT_STARTTLS` after this option, it
> > + MUST renegotiate extended headers.
> > +
>
> Two thoughts here:
>
> - We should probably allow NBD_REP_ERR_BLOCK_SIZE_REQD as a reply to
> this message; I could imagine a server might not want to talk 64-bit
> lengths if it doesn't know that block sizes are going to be
> reasonable.
Good addition. I'll include it in v2.
> - In the same vein, should we perhaps also add an error message for when
> extended headers are negotiated without structured replies? Perhaps a
> server implementation might not want to implement the "extended
> headers but no structured replies" message format.
Seems reasonable.
>
> On that note, while I know I had said earlier that I would prefer not
> making this new extension depend on structured replies, in hindsight
> perhaps it *is* a good idea to add that dependency; otherwise we create
> an extra message format that is really a degenerate case of "we want to
> be modern in one way but not in another", and that screams out to me
> "I'm not going to be used much, look at me for security issues!"
>
> Which perhaps is not a very good idea.
Yeah, the more I read back over Vladimir's message, the more I am
agreeing that just because we CAN be orthogonal doesn't mean we WANT
to be orthogonal. Every degree of orthogonality increases the testing
burden. I'm happy to rework v2 along those lines (structured replies
mandatory, and only one extended reply header, so that only compact
style has two header types).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Reply to: