chucklever / i-d-rpcrdma-cm-pvt-data Goto Github PK

Expert review called out the weaknesses of the format identifier word.

Reviewer:

I agree that the format specifier is useless. There are many existing users which could be setting the private data to anything. You really need to rely on the port numbers being correct, with checks on the other fields detecting mismatched protocols.

Lever:

Other reviewers have been ambivalent about the format identifier, but we don't have a better solution at the moment. The problem is the port numbers are all arbitrary. AFAIK they are not used in a way that identify the connection as RPC.
The 2049 destination port number means "NFS" but that's a layer /above/ RPC/RDMA.

Talpey:

This is not the purpose of the identifier. It is there to signal to the receiver that the payload is the one defined in the document. If the identifier is not present, the receiver will ignore the private data. This means the identifier must be defined, and present. It's not meant to be universal. Yes, there's a risk that another layer injects its own private data, with a payload somehow containing the identifier. That might warrant a sentence or two.

I propose that text explaining the risks (Talpey) be introduced. In addition, it's not likely that an RPC consumer will attempt to use the CM private data. Remove mention of "protocols above RPC/RDMA" using it. Also, it's likely that only transport layer protocols (ie., iWARP) will use CM private data at the same time as RPC/RDMA.

Filing this as a separate issue, since it is relevant to several other documents.

The current URL listed in -04 is http://www.infinibandta.org/content/pages.php?pg=technology_download . This URL appears in other documents, including RFC 5040, but is no longer valid. The IBTA appears to have reworked their web site.

Proposed and rejected:

• http://www.infinibandta.org/specs - also no longer valid
• https://www.infinibandta.org/ibta-specification/ - not specific to this document
• https://www.infinibandta.org/ibta-specifications-download/ - not specific to this document
• https://cw.infinibandta.org/document/dl/7859 - marked Proprietary and Confidential, is behind the registration wall

Roman Danyliw has entered the following ballot position for draft-ietf-nfsv4-rpcrdma-cm-pvt-data-07: No Objection

COMMENT:

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

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.

So this private data field is only existing in Infinite Band Connection manger? So what this document is defining a structure to a field which currently don’t have any structure but are part of a protocol defined by another body?

Have this work been formally agreed with IBTA?

Or is the relationship to the connection manager used to establish the RPC over RDMA different, if that is the case it needs to be better explained.

Is it bit 8 or bit 15 of the flags field that are used. Section 5.1 and 4.1 do not agree. The later thinks it is bit 8.

Section 5: Is there only going to be one object of private data forever in the relevant communication? If not, why isn’t this a well-specified TLV so that unknow type entries can be skipped to the nest object?

Even if there ever is going to be one entry, can you be more formal in description of what is a valid message using another format identifier, is that only the 32-bit field of the format Identifier, or also the version.

Isn’t the version field just unnecessary? If one needs version using another format identifier is more easy than the identifier + version construct.

Section 6: The IANA consideration is confusing. If the specification for a CM private data format requires IESG approval, then why have the expert review policy. In that case one can simply use the IESG approval policy instead. However, requiring any type of IESG approval here appear to be a way to high bar. I think expert review is likely appropriate, but the guidance to the Experts needs to be clearer. For example what happens if IBTA want to use this to add some format?

Section 7: The security consideration. I am missing any discussion about the security requirements that the actual extension. It appears that integrity and source authenticity are required for safe operation of these two extensions.

IBARCH URL Is not valid, it results in a 404.

Alissa Cooper has entered the following ballot position for draft-ietf-nfsv4-rpcrdma-cm-pvt-data-07: No Objection

COMMENT:

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.

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" ?

DISCUSS:

Thanks for this document. This is a simple DISCUSS point that should be very
easy to resolve:

— Section 5.2 —

A sender computes the encoded
value by dividing the buffer size, in octets, by 1024 and subtracting
one from the result.

Is the buffer size necessarily a multiple of 1024? If so, where is that
specified? If not, what is the encoded value when the buffer size is, say,
2000? Is it zero? Or one?

COMMENT:

Some purely editorial comments:

— Abstract —
The abstract needs to stand alone, so you should expand the term RDMA-CM in the
abstract. (RPC doesn’t need expanding, so once you’ve expanded RDMA-CM,
RPC-over-RDMA should be OK.)

— Introduction —
Please expand “XDR” on first use.

Section 7 of the current document

“of this document” is better, I think.

— Section 3.2 —
Please expand “RNIC” and “STag”.

invalidation without the need for additional protocol to be defined.

Either “an additional protocol” or “additional protocols”.

— Section 4.1 —

Realizing these goals
require that implementations of this extension follow the practices

The subject is “realizing”, which is singular. So, “requires’.

— Section 5.1 —

Bits 14 - 8: These bits are reserved and are always zero when the
Version field contains 1.

In other protocols, leaving it unspecified as to what happens if not all
reserved bits are zero has caused interoperability problems. If you know
that’s not a concern here, that’s fine. Otherwise, it might be good to say
explicitly that either they are ignored on receipt or non-zero bits result in
an error.

During AD write-up, Magnus pointed out that this document should be a Proposed Standard rather than Informational. I checked with Tom Talpey on this (emphasis mine):

TL;DR Proposed Standard is appropriate.

I may be the guilty party here. When Chuck originally was exploring using private data to extend rpcrdmav1, I encouraged him to publish the format as a draft. By design, the private data is optional, for both the sender and the receiver. Its presence, and its recognition at the peer, drives the extended features.

At the time, the concern for standardization was whether it would be of interest to the WG, and/or to other implementations. Its adoption as a WG item settles that.

When published as Proposed Standard, the document should additionally be tagged as an optional extension to RFC8166.