Re: [nfsv4] WG last call for draft-ietf-nfsv4-rpcrdma-cm-pvt-data-02.txt (Ends May 31st)

[Tom Talpey <[e-mail]>]

These changes look good, but I do have a few comments, inline.

On 6/11/2019 3:20 PM, Chuck Lever wrote:
> At this point WGLC closed over a week ago, so I will assume
> that there are no further pending comments on this document.
> As soon as Tom is satisfied that his comments have been
> addressed, I will submit the updated draft as
> 
>    draft-ietf-nfsv4-rpcrdma-cm-pvt-data-03
> 
> and it can proceed to the next step.
> 
> Tom- Thanks for your review!
> 
> I propose the following changes below to address your
> comments. Feel free to follow up if you have additional
> concerns.
> 
> 
>> On May 30, 2019, at 2:30 PM, Tom Talpey <[e-mail]> wrote:
>>
>> On 5/6/2019 12:46 PM, spencer shepler wrote:
>>> All,
>>> Thanks to Chuck, we have the following I-D entering working group last call.
>>> “RDMA Connection Manager Private Data For RPC-Over-RDMA Version 1”
>>> We will start the review today and end on May 31^st .
>>> The source of the I-D can be found here and comments should be sent to the WG alias.
>>> https://datatracker.ietf.org/doc/draft-ietf-nfsv4-rpcrdma-cm-pvt-data/
>>
>> I have some comments on final review.
>>
>> In the Abstract, it should be stated that the addition of the
>> private data payload specified in the draft is an optional
>> extension, and that the rpcrdmav1 protocol does not require it.
> 
> I plan to add the following two sentences to the end
> of the Abstract:
> 
> NEW TEXT:
> 
> The addition of the private data payload specified in this
> document is an OPTIONAL extension. The RPC-over-RDMA version
> 1 protocol does not require the payload to be present.

Looks good.

>> In the Introduction final paragraph the statement "The message
>> format can be extended as needed" is perhaps overly broad. I think
>> the better way to state what is meant here is that the specification
>> is intended to be further extensible, within the normal scope of
>> such IETF work, and it defines an IANA registry for this. A forward
>> reference to section 5 would help.
> 
> In the last paragraph of the Introduction:
> 
> OLD TEXT:
> 
> The message format can be extended as needed.
> 
> REPLACEMENT TEXT:
> 
> The message format is intended to be further extensible
> within the normal scope of such IETF work (see Section 5 for
> further details). Section 6 of the current document defines
> an IANA registry for this purpose.

Also good.

>> In section 3.2 third paragraph two comments. First, it describes an
>> advantage of avoiding a context switch. This is a local implementation
>> matter, and not really something the protocol can or cannot proscribe.
>> It would be state the benefit in protocol terms, such as "avoids
>> processing of the invalidate completion", then go on to say which may
>> enable avoiding interrupt(s) and context switch(es). Second, a nit - the
>> word "upshot" is slang. Suggest "benefit".
> 
> I plan to replace the third paragraph of Section 3.2:
> 
> OLD TEXT:
> 
> An RPC-over-RDMA responder can use remote invalidation
> when replying to an RPC request that provided Read or
> Write chunks. The requester thus avoids dispatching an
> extra Work Request, the resulting context switch, and
> the invalidation completion interrupt as part of
> completing an RPC transaction that uses chunks. The
> upshot is faster completion of RPC transactions that
> involve RDMA data transfer.
> 
> REPLACEMENT TEXT:
> 
> An RPC-over-RDMA responder can use remote invalidation
> when replying to an RPC request that provided chunks.
> The requester thus avoids processing an invalidation
> completion for that chunk. The benefit is faster
> completion of RPC transactions that involve RDMA data
> transfer.

The words "The requester thus avoids processing..." don't quite
capture the benefit, I feel. In particular, "processing" is a
rather passive term and doesn't capture the full benefit.

Here's a swag:

"This extensions provides a means for the RPC-over-RDMA requester
to signal the responder to optionally use this remote invalidation
when replying to an RPC request that provided chunks. In this way,
the RDMA provider performs the invalidation in the process of
delivering the response, which allows the requester to avoid
required additional, and possibly expensive, invalidation prior
to finalizing the results of the RPC."

I admit, it's tricky language to get right. Maybe it can be stated
more simply.

>> At the end of section 3.2, the final paragraph is quite mysterious. It
>> refers to "a simple signaling mechanism" and "some NFS/RDMA client".
>> Really, something more needs to be said here. If this signaling
>> mechanism involves a protocol payload, then definitely so. Can you
>> elaborate? I admit I don't fully understand what is meant here.
> 
> I plan to replace the final paragraph of Section 3.2:
> 
> OLD TEXT:
> 
> However, it is possible to provide a simple signaling mechanism
> for a requester to indicate it can deal with remote invalidation
> of any STag it has presented to a responder.
> There are some NFS/RDMA client implementations that
> can successfully make use of such a signaling mechanism.
> 
> REPLACEMENT TEXT:
> 
> There are some NFS/RDMA client implementations whose STags
> are always safe to invalidate remotely.
> For such clients, indicating to the responder that remote
> invalidation is always safe can allow such invalidation
> without the need for more complex communication between the
> requester and responder.

"...without the need for additional protocol to be defined for
management of STags between requester and responder."

Maybe?

>> In section 4, the scope of the properties are correctly confined to
>> the connection on which they are exchanged. But, this does not mention
>> the visibility of the properties to the upper layer consumer, which
>> must be aware of them, especially when reconnecting since RDMA behaviors
>> may change. I suggest a short statement in sectin 4.1 Interoperability
>> Considerations to this point.
> 
> In fact these properties are not visible to the upper layer
> consumer (if by "ULP consumer" you mean RPC). However, if you
> mean the RPC/RDMA transport implementation, yes, it does
> have to be prepared for those settings to change after a
> reconnect. I've inserted a sentence at the end of the second
> paragraph of Section 4 to clarify this:
> 
> OLD TEXT:
> 
> The transport properties exchanged via this mechanism are
> fixed for the life of the connection. Each new connection
> presents an opportunity for a fresh exchange.
> 
> REPLACEMENT TEXT:
> 
> The transport properties exchanged via this mechanism are
> fixed fo the life of the connection. Each new connection
> presents an opportunity for a fresh exchange. An
> implementation of the extension described in this document
> MUST be prepared for the settings to change upon a
> reconnection.

Good.

>> In section 7, the vulnerability is in the RDMA-CM protocol, not this
>> protocol. And, private data is carried differently by different lower
>> layers - for example in the IETF iWARP family, it's part of MPA.
>> Honestly, I'm not sure this is relevant to this document. It basically
>> inherits all the considerations of the protocols it extends.
> 
> I've replaced the entirety of the Security Considerations
> section as follows:
> 
> OLD TEXT:
> 
> RDMA-CM Private Data typically traverses the link layer in
> the clear. A man-in-the-middle attack could alter the
> settings exchanged at connect time such that one or both
> peers might perform operations that result in premature
> termination of the connection.
> 
> REPLACEMENT TEXT:
> 
> The private data extension specified in this document
> inherits the security considerations of the link layer
> protocols it extends (e.g., the RDMA-CM protocol).

Hmm, this will trigger the next logical question "where is this protocol
defined?", and the answer ("outside the IETF") will cause trouble.

Also, it's not strictly the link layer. All the IETF RDMA protocols
operate above transport.

I'd suggest keeping the example to the IETF-owned MPA protocol, which
is the one governing RDMA connection establishment and which carries
private data. I.e, RFC 5044 (MPA) and RFC6581 (MPA extensions). MPA, in 
turn, makes its own security considerations and refers to RFC5042 (the
original RDMA Security analysis), which may be a useful reference but
is not specifically about connection establishment.

Tom.