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

[David Noveck <[e-mail]>]

On Mon, Dec 16, 2019, 3:39 PM Tom Talpey <[e-mail]> wrote:

> 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,


For it to do so is complicated and unnecessary.  The point is that it is
job of the transport protocol to pass along the cm data without modifying
it.

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,


The private data field on the wire is but rpc-over-rdma never sees that.
It does see what it received from the transport implementation via, for
example, the ofed interface. For that data the transport is the custodian
rather than the owner.

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.
>

If it states that it should be changed, but I read it as describing what
the rpc-over-rdma protocol would see.

>
> > 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,


If it doesn't, it is seriously broken.

but what's on the wire will look quite different.
>

What's on the wire might be encrypted and look* really* different. However,
we don't and shouldn't see any of that.

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.
>
> _______________________________________________
> nfsv4 mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/nfsv4
>