Re: [nfsv4] Proposed updates to draft-ietf-nfsv4-rpcrdma-cm-pvt-data

[Tom Talpey <[e-mail]>]

On 12/16/2019 9:52 AM, Chuck Lever wrote:
> 
> 
>> On Dec 16, 2019, at 9:36 AM, Tom Talpey <[e-mail]> wrote:
>>
>> On 12/15/2019 3:48 PM, Chuck Lever wrote:
>>> As a result of expert review, changes are needed to Section 4.1.2
>>> to clarify the purpose and implementation guidance of the Format
>>> Identifier field.
>>> I propose that the new Section read (in its entirety):
>>> 4.1.2.  Amongst Implementations of Other Upper-Layer Protocols
>>
>> That first word is a very odd one in a section title. Would
>> "Interoperability With" be more meaningful?
> 
> I will review the titles of the sub-sections here.
> 
> 
>>>     The Format Identifier field in the message format defined in this
>>>     document is provided to enable implementations to distinguish RPC-
>>>     over-RDMA version 1 Private Data from private data inserted at layers
>>>     below RPC-over-RDMA version 1.  An example of a layer below RPC-over-
>>
>> "Below" is problematic here. The RFC6581 MPA enhanced connection
>> processing can insert private data at the start of the field, and
>> it is "below" RPC in the stack, but the peer's RFC6581 processing
>> strips it off. Therefore, of RPC is the "lowest upper layer" in
>> such a stack, there is no issue.
>>
>> However, there might well be other lower layers, with different
>> behaviors, injecting their own private data payloads. Or indeed,
>> upper layers. And these payloads may choose to append, or prepend
>> to the buffer.
> 
> OK. Are you requesting a change to this paragraph?

I think it would be better to avoid introducing a formal layering, so
yes. Instead of "layers below", it may be best simply say "other layers".

The next issue makes this clearer:

>>>     RDMA version 1 that makes use of CM Private Data is iWARP, via the
>>>     MPA enhancement described in [RFC6581].
>>>     During connection establishment, an implementation of the extension
>>>     described in this document checks the Format Identifier field before
>>>     decoding subsequent fields.  If the RPC-over-RDMA version 1 CM
>>>     Private Data Format Identifier is not present as the first 4 octets,
>>
>> So, just to be clear - this introduces a new requirement on other
>> layers over the same connection. They MUST NOT inject private data
>> at the beginning of the buffer, or if they do, they MUST strip it
>> off.
>>
>>>     an RPC-over-RDMA version 1 receiver MUST ignore the CM Private Data,
>>>     behaving as if no RPC-over-RDMA version 1 Private Data has been
>>>     provided (see above).
>>
>> And, if the prior requirement is not made, then the RPC layer needs
>> to scan the prvate data rather carefully to see if the identifier,
>> and the payload associated with it, is present somewhere in the
>> private data.
> 
> You might have misread this paragraph. I don't think there's a need
> for any new requirements: the paragraph states if the first word in
> the buffer is not the RPC-over-RDMA Format Identifier, then the
> receiver ignores the CM private data.

Ok, I didn't misread the paragraph, but I drew a different conclusion.
If the protocol requires the RPC sender to place its private data
payload "first", then it is logical to require the receiver to look
at it only there. Lower layers may do the same (e.g. the MPAv2 ird/ord
exchange), but of course those layers must strip it off before passing
the remainder. So at the RPC layer, it is no longer present.

The draft currently doesn't discuss this type of sharing, however, it
merely states:

    The first 8 octets of the CM Private Data field is to be formatted as
    follows:

In reality, the private data field is a property of the underlying
transport, and in the case of iWARP/MPAv2 the first bytes are already
"taken". So, my concern was that the text was somehow stating that
the entire private data paylod was to be inspected, when in reality
it's just the part which was *not* otherwise taken by other layers.

> No other scanning is necessary, it's a fail-safe design since the
> CM private data contains only hints.

Yes, that's understood. Again, it's just a matter of describing how
the field is to be found. The CM API may strip off other non-shared
private data chunks, but what's on the wire will look quite different.
So the word "first" is, I find, problematic.

I'm short on time right now to craft suggested text for this, but I'll
take a shot at it hopefully tomorrow.

>>>     Because the Format Identifier field is newer than some other
>>>     potential users of private data (such as iWARP), there is a risk that
>>>     a lower layer might inject its own private data with a payload
>>>     somehow containing the identifier of RPC-over-RDMA version 1.  It is
>>
>> Well, *and* not strip it off. This could happen if the peer implemented
>> a lower layer that wasn't recognized by the receiver, and the data
>> simply passed up. See previous paragraph.
>>
>>>     recommended that RPC-over-RDMA version 1 implementations perform
>>>     additional checks on the content of received CM private data before
>>>     making use of it.
>>
>> "Additional checks" is pretty vague. Are there any specific requirements?
> 
> Would "perform sanity checks on the content" be preferable?

I was asking what the sanity checks might actually be. Is there a specific
requirement? If not, I don't think the sentence really says anything,
and should be dropped.

What I think is being implied is that after matching the identifier,
somehow the range of values in the following structure needs to be
checked, and if any of the values fall outside, that the entire payload
be ignored. Leaving me to ask, what are these limits?

Tom.

Tom.