I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair. Please treat these comments just like any other last call comments. For more information, please see the FAQ at < http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>. Document: draft-ietf-nfsv4-minorversion2-39.txt Reviewer: Elwyn Davies Review Date: 2015-12-13 IETF LC End Date: 2015-12-09 IESG Telechat date: (if known) - Summary: Almost ready. There are a fair number of minor nits and editorial suggestions. The couple of 'minor issues' are predominantly questions that came into my mind that might be interesting to a reader/implementer but are probably unlikely to affect the performance of the protocol. However, the comments on s11.2, s12.1, s13 and s15.2.3 appear to be actual omissions or errors. Major issues: None. Minor issues: s4: There is no discussion in s4 of the Named Attributes associated with a file and the restriction of server-to-server copy to 'regular files' seems to indicate that Named Attributes cannot be copied by this mechanism. Is there anything to be said about getting the Named Attributes across? s4/s9: Does server-to-server copy interact (badly, or otherwise) with Labeled NFS? Nothing is said, but I wonder whether there are issues around atomicity and MAC between the different servers for inter-server copy. s4.10.1.1.1: Size and reuse for random number used as a shared secret for inter-server copy. No advice is given on the desirable size/length of the random number or the risks or otherwise of reusing the same context for multiple file copies. This maybe a non-issue - I defer to those with more security clue than me. Nits/editorial comments: General: bytes or octets? Abstract: Probably ought to expand i/O. s1/s1.1: I think you could safely delete the s1.1 header leaving the text as the body of s1 - this is generally considered better practice than leaving s1 itself empty. s1.2, 3rd bullet: s/protocols. I.e.,/protocols, that is/; s/apply/apply only/ s1.4: The overview doesn't cover the two LAYOUT related operations and there is is no section describing what their usage might be in with sections 4-10. s1.4: Similarly, there is noting about the CLONE operation. s1.4.1, para 1: s/remotely accessed/remotely accessed file/; s/location/locations/ s1.4.1: (very nitty!) Suggest making the descriptions of the two cases (intra- and inter-server) into bulleted paragraphs to make it clearer that they are separate features. s1.4.2: (as with the Abstract) expand I/O on first occurrence. s1.4.2: It would be worth putting in the reference for posix_fadvise here. s1.4.3: If you write the data in the hole, it isn't really a hole ;-) .... OLD: Such holes are typically transferred as 0s during I/O. NEW: Such holes are typically transferred as 0s when read from the file. END s1.5: Suggest s/I.e., READ becomes either/For instance, READ would have to be replaced or supplemented by, say, either/ s2, last bullet: Need to expand pNFS on first use (and maybe remind readers that it is a feature of NFS4.1 - Section 12 of RFC 5661) s2, last bullet: s/metadata sever/metadata server/ - again a pointer to (say) Sections 1.7.2.2 and 12.2.2 of RFC 5661 would clarify what a metadata server is. s4.2.1: The first para reads: OLD: COPY_NOTIFY: Used by the client to notify the source server of a future file copy from a given destination server for the given user. (Section 15.3) I completely misread this on first pass, but I understand that it is correct. Having checked with s15.3 and thought a bit more about it, it is the 'copy from a given destination' that threw me - I guess it means 'the copy will be made from' rather than 'the file being copied from the destination'... Doh! A client sends this operation in a COMPOUND request to the source server to authorize a destination server identified by cna_destination_server to read the file specified by CURRENT_FH on behalf of the given user. I suggest the following might be avoid this brain fade: NEW: COPY_NOTIFY: Used by the client to request the source server to authorize a future file copy that will be made by a given destination server on behalf of the given user. (Section 15.3) END s4.2.2, para 5: s/support all these operations/support these operations/ ('all' is confusing given that only two are then explicitly mentioned). s4.3, para 1: s/Inter-server copy is driven/The specification of inter-server copy is driven/ s4.4.1, last para: s/The source MUST equate/The source MUST interpret/ s4.6/Figures 4 and 5: Need a 'key' to explain os1 and os2. s4.7.2: Adding a reference to Section 15.3(.3) would help. s4.7.3, para 2: idnits identifies RFC 2616 as obsoleted by RFC 7230, etc. A more recent ref is desirable. s4.8, para after code fragment: LDAP and NIS need to be expanded (DNS and URL are well-known). s4.9, para 3: Is it possible to add a little explanation as to *why* seqid = 0 is ambiguous? My limited knowledge doesn't grok this. s4.10: Is it possible to provide an abstract definition of 'structured privilege'? The rpcsec-gssv3 draft appears to punt on this, pointing to the 'example' in the NFSv4.2 draft. I think I get the idea but a succinct definition would help I believe. I guess the definition ought to be in the rpcsec-gss document and referenced from this doc. s4.10.1.1.1, 2nd bullet: Need to expand QOP. s4.10.1.2, para 2: s/uiniquely/uniquely/ s5, title: s?IO?I/O? s6.1: This heading could be deleted turning the text in 6.1 into Section 6 which is then not empty. s6.1: The term 'inode' is used four times in this section. Whilst this is well understood, it is strictly associated with *nix file systems. It would be desirable to find an alternative term or, if you can't think of suitable short generic term (I spent a little time thinking about it and couldn't think of one immediately), some weasel words added to the first occurrence saying that it is intended to cover equivalent structures in other sorts of filing system. s6.1, para 2: s/can thought as holes/can thought of as holes/ s6.1, para 4: s/couple/few/ ('couple hundred' is a USA specific colloquialism - UK would use 'couple of') s6.1, last para: s/punching. I.e., an application/punching, where an application/ s7, para 1: s/One example is the posix_fallocate/One example is the posix_fallocate operation/ s7 OLD: space_freed This attribute specifies the space freed when a file is deleted, taking block sharing into consideration. NEW: space_freed This attribute reports the space that would be freed when a file is deleted, taking block sharing into consideration. s8, bullet #2: I found the 2nd sentence hard to parse. Suggested alternative below. OLD: 2. Fields to describe the state of the ADB and a means to detect block corruption. For both pieces of data, a useful property is that allowed values be unique in that if passed across the network, corruption due to translation between big and little endian architectures are detectable. For example, 0xF0DEDEF0 has the same bit pattern in both architectures. NEW: 2. Fields to describe the state of the ADB and a means to detect block corruption. For both pieces of data, a useful property would be that the allowed values are specially selected so that if passed across the network, corruption due to translation between big and little endian architectures is detectable. For example, 0xF0DEDEF0 has the same (32 wide) bit pattern in both architectures, making it inappropriate. END PS: The example is relevant to 32 bit memory architectures... It has never occurred to me to ask if there are 64 bit big and little endian architectures! Well the IA-64 is bi-endian... s8.3: The pictures of the memory patterns don't match the specification in that the guard pattern appears to be at octet offset 4 in each ADB - You can't tell where the checksum is from the pictures, but as specified there would be a 4 octet gap between the guard pattern and the checksum - I presume this is intentional. Would it be guaranteed to be zero? s9.1: Again it would be best to delete the title of s9.1 and leave the text as s9. s9.1, para 2: Expand ACL please. s9.1/s9.6.1.3: I am dubious about referring back to the requirements doc [RFC7204] for definitions (rather than indicating what requirements were met/not met) . For the ref in s9.1, I think that the MAC definition in RFC 4949 would suffice. [I noted when reviewing the rpcsec-gssv3 draft a couple of days ago that it makes *normative* reference to RFC 7204 - this is even more undesirable - my feeling is that it should be possible to find out what implementers need to know from the NFSv4.2 standard, other standards or the security glossary as there is no certainty that what is said in the requirements was actually put into the standard, which could cause confusion for future implementers.] Is it possible to get everything an rpcsec-gssv3 implementer needs to know into this document? My impression is that it is almost all there already. s9.2, MLS definition: RFC 2401 has been obsoleted by RFC 4301... can this be referenced instead? s9.4: Expand DS and MDS on first occurrence (these should probably go back in s3 where metadata servers and data servers are referred to but without the abbreviations). s11.2, Table 2: The operation IO_ADVISE is missing from the table. [CAVEAT: I don't claim to have checked the possible error codes!] Aesthetically, this table would be better with horizontal separators between the operation items. s12.1, Table 4: The data types of clone_blksize (length4 vs uint32_t) and space_freed (length4 vs uint64_t) do not match between this draft and the XDR draft. s12.2.3, NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR: Monotonic is not really a good name for this I think. This is because the technical definition is either non-increasing or non-decreasing so that it would theoretically cover situations where the change attribute doesn't actually change! IMO a better name would be NFS4_CHANGE_TYPE_STRICTLY_INCR. This would imply that the value actually increases whenever the info changes. s12.2.3: With reference to the previous comment... OLD: If either NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR, NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, or NFS4_CHANGE_TYPE_IS_TIME_METADATA are set, then the client knows at the very least that the change attribute is monotonically increasing, which is sufficient to resolve the question of which value is the most recent. NEW: If either NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR, NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, or NFS4_CHANGE_TYPE_IS_TIME_METADATA are set, then the client knows at the very least that the change attribute is strictly increasing, which is sufficient to resolve the question of which value is the most recent. END s13, Operations table: The abbreviation EOL in the title line does not seem to be expanded anywhere (and isn't used in the table either). It would be good to have table numbers for the tables. s13, Operations table: Does not include IO_ADVISE ( and technically doesn't include ILLEGAL, and CB_ILLEGAL isn't in Callbacks). s15.2.3: If it cannot make that determination, it must set to false the one it can and set to true the other. The setting of true and false appears to be the inverse of the meaning implied in the previous part of the section. s15.5.6/s15.5.6.1: Pointers into the appropriate parts of the NFS v4.1 RFC would be helpful in giving the background needed to understand what is going on here. The discussion of dense and sparse packing in s15.5.6.1 is particularly obscure if you are not very well versed in pNFS lore. ss15.6 and 15.7: An overview of the LAYOUT* operations in the earlier part of the document would be helpful, as mentioned above, plus some pointers to parts of the NFSv4.1 document that ties in with the pNFS layout story would be helpful. s19.2, [Quigley14]: This is now RFC 7569. Also I think this should be normative. s19.2, [BL73]: Can you point me to a copy of this? I have found some closely related work which was originally published as MITRE Technical Report 2547 Vols I and II at http://www.albany.edu/acc/courses/ia/classics/belllapadula1.pdf (belllapadula2.pdf) and in the Journal of Computer Security but the original doesn't seem to be visible.