Remote Direct Memory Access - Connection Manager (RDMA-CM) Private Data for RPC-over-RDMA Version 1
draft-ietf-nfsv4-rpcrdma-cm-pvt-data-08

Thanks for all of the changes made in response to the LC SECDIR review.  Also, thank you for the LC SECDIR review, Yaron (Sheffer)!

Section 6.  As there is dependence on the behavior defined in [IBA], this reference should be normative.

Thank you for the work put into this document. I found this document not so easy to read as many acronyms are used without expansion (Stag, CM, ...) notably in the abstract. While the introduction simply refers to RFC 8166, a little more textual introduction would have been welcome.

Nevertheless, please find below some non-blocking COMMENTs (and I would appreciate a response from the authors but this is not required).

I hope that this helps to improve the document,

Regards,

-éric

== COMMENTS ==

-- Section 4 --
Just by sheer curiosity, I wonder where the value "0xf6ab0e18" comes from ? 

-- Section 4.1.1 --
"bit 15 of the Flags field" but the Flags field is only 8-bit long (to be honest, I am sure that I understand the meaning of this but being clearer would be better). Wording in section 5.1 should be used in section 4 when describing the Flags field.

I would also suggest to name the different bits of the Flags field as usually done in other IETF documents.

-- Section 5.1 --
About the reserved bits, why not using the usual wording of "set to 0 when sending and ignored when received" ?

Balloting "No Objection" in the sense of "I trust the sponsoring AD, and have no time this ballot cycle to read the document." I have skimmed the document for typical ART-area gotchas, and found none.

I agree with Barry’s DISCUSS.

I'm not sure how strict we usually are about this, but the guidance in Section 7.1 makes it sound like the proper registration policy is actually Specification Required, not Expert Review.

Thanks for addressing my DISCUSS point and editorial comments.

I agree with Alissa.

Section 4

   For RPC-over-RDMA version 1, the CM Private Data field is formatted
   as described in the following subsection.  RPC clients and servers
   use the same format.  If the capacity of the Private Data field is
   too small to contain this message format, the underlying RDMA
   transport is not managed by a Connection Manager, or the underlying
   RDMA transport uses Private Data for its own purposes, the CM Private
   Data field cannot be used on behalf of RPC-over-RDMA version 1.

How will an implementation know if "the underlying RDMA transport uses
Private Data for its own purposes"?

Section 5

   Although it is possible to reorganize the last three of the eight
   bytes in the existing format, extended formats are unlikely to do so.
   New formats would take the form of extensions of the format described
   in this document with added fields starting at byte eight of the
   format and changes to the definition of previously reserved flags.

I would suggest making it a (mandatory) invariant of this format to
retain these last three bytes' interpretation, requiring the use of a
different "magic word" for future versions that need to diverge from it.
The current text does not really give an implementation anything that it
can rely on.

Section 6

   The RPC-over-RDMA version 1 protocol framework depends on the
   semantics of the Reliable Connected (RC) queue pair (QP) type, as
   defined in Section 9.7.7 of [IBA].  The integrity of CM Private Data

It's interesting to see such a reference to [IBA], when IIUC the RFC
8166 protocol is not limited to Infiniband as the underlying transport.

   Additional analysis of RDMA transport security appears in the
   Security Considerations section of [RFC5042].  That document

nit: the actual analysis isn't *in* the security considerations section
(but is referenced from it).

   Improperly setting one of the fields in a version 1 Private Message
   can result in an increased risk of disconnection (i.e., self-imposed
   Denial of Service).  There is no additional risk of exposing upper-
   layer payloads after exchanging the Private Message format defined in
   the current document.

I'm not entirely sure where or how one might have expected such
additional exposures to occur.


We should probably mention the risk that some (other) CM-private data
item might inadvertenly produce in its payload the "magic number" that
we use to identify this protocol's data structure.  I *think* (but
please confirm) that erroneously doing so would lead only to (likely)
RDMA-channel disconnection and could not introduce (e.g.) data
corruption.

   In addition to describing the structure of a new format version, any
   document that extends the Private Data format described in the
   current document must discuss security considerations of new data
   items exchanged between connection peers.

In a similar vein, future extensions should consider what the risks of
erroneously identifying "random" data as the new format would be.

Section 7

Should the registry also include the length of the private data?

Similarly to the previous section's comments, should prospective
registrations also be analyzing the risks to their protocol of
interpreting "random" data as the data structure (as would happen upon
an inadvertent match of the "magic number")?

Section 7.1

   The new Reference field should contain a reference to that
   documentation.  The DE can assign new Format Identifiers at random as
   long as they do not conflict with existing entries in this registry.

Random may not be the best choice for this, if there are ways to produce
values that are less-likely-than-random to occur inadvertently in the
payload of any of the registered formats.

One quick thought/comment: Another option for extensibility would be to use one of the reserved flags to e.g. extend the fields of the private data field. However, the draft states at all reserved flags need to be zero with version 1. This seems to be a bit of a waste of space but moreover it's a lost opportunity for an easy way to extend the private data field. Why was that decided?

I would like to see a resolution of Barry's DISCUSS as well. In addition if you are using division by 1024, I think the appropriate range needs to be 1KiB-256KiB instead of 1KB-256KB as defined in the draft.