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

[Magnus Westerlund <[e-mail]>]

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.

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