Re: [nfsv4] AD review of draft-ietf-nfsv4-rpcrdma-cm-pvt-data

[Chuck Lever <[e-mail]>]

> On Nov 7, 2019, at 11:22 AM, Magnus Westerlund <[e-mail]> wrote:
> 
> Hi Chuck,
> 
> This updates contains quite a lot of improvmenets. Howver I do have some
> feedback where I think it can be further improved.
> 
> 
> Section 1: I am missing in the introduction of the assumption that to be able to
> use this option one requires an ULP with a private data field. I think there
> should be a paragraph here providing that context and include infromational
> references to protocols that may use this extension. And I think including that
> IETF defined ULP here as one option in addition to IBA would be good. 
> 
> Section 4 calls the 32-bit ID for format identifier, while the IANA Section
> calls it protocol number. Please align the language here. Also the name of the
> format is inconsistent after these latest changes. I think the more extensive
> name of the format was clearer then what is propsoed now. Becasue it is really
> this specific option that is identified, not RPC over RDMA protocol in general. 
> 
> Section 4: First paragraph. Depending on what is added to Section 1, this may or
> may not need this first paragraph in the current form. But, I get the
> impression, that this text either isn't needed here or needs to contain way more
> details for the specific protocol. I think the later is hard to do, especially
> for an protocol that we don't have change control over. 
> 
> Section 6.1: The appointment of the designated expert is done by the IESG and
> not the Area Director themselves. See section 5.2.1 of RFC 8126. 
> 
> Section 6.1:
> "The DE will post the request to the nfsv4 WG mailing list (or a successor to
> that list) for comment and review.  The DE will approve	or deny the request and
> publish notice of the decision within 30 days."
> 
> I would propose that this is reworded so that it will not be necessary for the
> IETF to designate a successor list, even if there are no real community. I would
> propose a simple addition like:
> 
> The DE will post the request to the nfsv4 WG mailing list (or a successor to
> that list), if such a list exist, for comment and review.

Thanks for the further comments. I've updated

https://chucklever.github.io/i-d-rpcrdma-cm-pvt-data/draft-ietf-nfsv4-rpcrdma-cm-pvt-data.html

to address them.


> Cheers
> 
> Magnus
> 
> 
> On Wed, 2019-11-06 at 17:00 -0500, Chuck Lever wrote:
>> Hi Magnus-
>> 
>>> On Nov 5, 2019, at 4:46 AM, Magnus Westerlund <
>>> [e-mail]> wrote:
>>> 
>>> On Mon, 2019-11-04 at 11:36 -0500, Chuck Lever wrote:
>>>> Hi Magnus, thanks for your review and moving forward with the AD write-up.
>>>> 
>>>>> On Nov 4, 2019, at 9:56 AM, Magnus Westerlund <
>>>>> magnus.westerlund=[e-mail]> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> I have started my AD review. It was such a short document that I thought
>>>>> I
>>>>> read through it to figure out what more I need to read. So I have not
>>>>> yet
>>>>> read like RFC 8166. So from a context of having little context
>>>>> understanding
>>>>> the below are my comments and questions. I think it may be faster if you
>>>>> help answering them then I read a lot of documents and still have
>>>>> question
>>>>> marks. 
>>>>> 
>>>>> 
>>>>> 	1. 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? 
>>>>> 		a. Have this work been formally agreed with IBTA? 
>>>>> 		b. 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. 
>>>> 
>>>> The private data field is for use by Upper Layer Protocols. It's existence
>>>> is defined by the IBTA, but not its content. It's opaque to the CM, hence
>>>> the name "private".
>>>> 
>>>> Both iSER and iWARP itself (both IETF-defined) can place data in this
>>>> field.
>>>> That's why there
>>>> is a 32-bit format ID field: if that doesn't match the constant defined in
>>>> this document, that means some other ULP is using the private data field,
>>>> and its contents are then ignored by RPC-over-RDMA.
>>> 
>>> So with new text as proposed in other email to clarify that this extensions
>>> apply to several different protocols will help making it clear that this is
>>> useful for IETF to define and not stepping on anyone's particular toes. 
>>> 
>>>> 
>>>> 
>>>>> 	3. 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?
>>>> 
>>>> As I understand it, the private data field is used by one ULP at a time.
>>>> I'm not aware of a TLV. There are already existing ULP implementations
>>>> that do not assume a TLV, so that might be a tough sell.
>>>> 
>>> 
>>> That helps a bit, but basically what we end up with is that the ULP need to
>>> cordinate between the end-point about which private data format(s) it will
>>> use.
>>> And if is to include multiple formats it need to know that the peer can
>>> delimit
>>> all of them. That is what I see as the issue. Maybe this assumption and
>>> responsibility of the ULP can be made clearer in the document. 
>>> 
>>>> 
>>>>> 	4. 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.
>>>> 
>>>> I can cite the use of this field by other protocols, if that would help.
>>> 
>>> As I said yes. 
>>>> 
>>>> 
>>>>> 	5. Isn’t the version field just unnecessary? If one needs
>>>>> version using
>>>>> another format identifier is more easy than the identifier + version
>>>>> construct.
>>>> 
>>>> It's possibly superfluous. But:
>>>> 
>>>> - the two fields serve different purposes, even though they could be
>>>> combined.
>>>> - there's already an implementation that uses both fields.
>>> 
>>> So, I understand that your down the road on this one and you don't need to
>>> implement any change. But, it was a reflection I had after commenting on
>>> issue
>>> 4). 
>>> 
>>>> 
>>>> 
>>>>> 	6. 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?
>>>> 
>>>> I would be OK with Expert Review only, but:
>>>> 
>>>> 1. I will need suggestions about expert guidance. Will look at other RFCs.
>>> 
>>> Basically the WG can specify what requirements you consider needed for a
>>> registration request. Publicly availble specification, contact information,
>>> what
>>> criterias the expert should make judgment on. Like if there are sensitive
>>> information or not. Etc. 
>>> 
>>>> 2. IMO WG consensus is needed on the replacement text of this section.
>>> 
>>> Sure, and if the WG want to choice IESG approval or Specification Required
>>> as
>>> policy for the registry that is also fine with me. However, I am currently
>>> not
>>> fine with the strange mix that is currently documented. 
>>> 
>>>> 
>>>> 
>>>>> 	7. 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.
>>>> 
>>>> Integrity is a property of RDMA Reliable Connection transports.
>>>> 
>>>> Not clear about the source authenticity requirement. Can you elaborate?
>>> 
>>> What I am trying to say, is that security consideration sections should make
>>> a
>>> security requirement analysis. And what I can see this private data when
>>> delivered to receiver, the receiver need to know that the data was not
>>> modified
>>> and it needs to know from whom it came. Thus integrity and source
>>> authentication. Then you can simply add a statement that the protocol you
>>> know
>>> to carry this field will provide that security service. 
>>> 
>>> The other aspect to consider is really if one can get any impact on the peer
>>> by
>>> being malicous in setting these fields. For example can I get the peer
>>> entity to
>>> consume more resources? To my understanding the answer is partially yes, but
>>> not
>>> without having to similarily promise to allocate resources. And if I don't
>>> do
>>> things will fail. And the point is that there is other methods that will be
>>> more
>>> efficient to do Denile of Service attacks on the peer in the protocol. 
>>> 
>>> It is usually good to document what aspects of attacks that has been
>>> considered
>>> and what level of an issue they are on.
>> 
>> The latest:
>> 
>> 
> https://protect2.fireeye.com/v1/url?k=86105e9f-da9a7c49-86101e04-0cc47ad93dcc-4cde713b365d5029&q=1&e=1de128de-db49-4c9d-8b49-89fbe4823d29&u=https%3A%2F%2Fchucklever.github.io%2Fi-d-rpcrdma-cm-pvt-data%2Fdraft-ietf-nfsv4-rpcrdma-cm-pvt-data.html
>> 
>> And a convenience URL for diffing with -04:
>> 
>> 
> https://tools.ietf.org/rfcdiff?url1=https://tools.ietf.org/id/draft-ietf-nfsv4-rpcrdma-cm-pvt-data.txt&url2=https://chucklever.github.io/i-d-rpcrdma-cm-pvt-data/draft-ietf-nfsv4-rpcrdma-cm-pvt-data.txt
>> 
>> 
>> I've tried to address the following issues that you called out after your
>> attempt at an AD write-up of -04:
>> 
>> - Now a Proposed Standard updating RFC 8166
>> - Replaced the stale link to the IB specification
>> - Additional text explaining precedents and the purpose of the Format
>> Identifier
>> - Revised the IANA Considerations section to remove "specification required"
>> language
>> - Revised the Security Considerations section
>> 
>> I can take this further if you would like more changes before the next steps.
>> Otherwise I can provide a draft-ietf-nfsv4-rpcrdma-cm-pvt-data-05.xml for you
>> to submit manually.
>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
> -- 
> Cheers
> 
> Magnus Westerlund 
> 
> 
> ----------------------------------------------------------------------
> Networks, Ericsson Research
> ----------------------------------------------------------------------
> Ericsson AB                 | Phone  [phone]
> Torshamnsgatan 23           | Mobile [phone]
> SE-164 80 Stockholm, Sweden | mailto: [e-mail]
> ----------------------------------------------------------------------

--
Chuck Lever