Hi, Tom.
Some comments inline - I have deleted the various things that are agreed.
Regards,
E;lwyn
On 12/11/14 22:11, Tom Haynes wrote:
Hi Elwyn,
Before I forget, thanks for the review.
.. and your prompt response. :-)
I have replied inline for the changes I have made or have a question on.
Tom
On Nov 11, 2014, at 12:48 PM, Elwyn Davies <[email protected]
<mailto:[email protected]>> wrote:
I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
Please resolve these comments along with any other Last Call comments
you may receive.
Document: draft-ietf-nfsv4-rfc3530bis-33.txt
Reviewer: Elwyn Davies
Review Date: 2014-11-06
IETF LC End Date: 2014-10-06
IESG Telechat date: 2014-12-04
Summary:
Much more ready than last time I reviewed this draft! There are a few
issues at the borderline between editorial and minor plus quite a lot
of editorial nits.
Major Issues:
Minor Issues:
s8.8.1: I suspect that this section should have been deleted when
s7.7 was deleted between v26 and v27. The references to various
server class attributes (handle class, change class, etc) are orphaned
with no definitions of these items
I’m fine with deleting this section.
OK
-----------------------------------------
Handling of wrap around of seqid4 variables:
David will reply with to the seqid4 issues.
OK
============================================
The specification of how variables/fields of type seqid4 are
incremented and compared, dealing with wrap around is very messy.
Currently there are two separate descriptions of handling wrap around
when a seqid4 variable is incremented through NFS4_UINT32_MAX (which
turns out to be the all ones binary pattern. The descriptions are
slightly different but not incompatible.
The following text appears in s9.1.6
Since the sequence number is represented with an unsigned 32-bit
integer, the arithmetic involved with the sequence number is mod
2^32. Note that when the seqid wraps, it SHOULD bypass zero and use
one as the next seqid value. For an example of modulo arithmetic
involving sequence numbers see [RFC0793].
and s9.1.3.2 has:
This pattern continues until the seqid is incremented past
NFS4_UINT32_MAX, and one (not zero) SHOULD be the next seqid value.
The purpose of the incrementing of the seqid is to allow the server
to communicate to the client the order in which operations that
modified locking state associated with a stateid have been processed.
In making comparisons between seqids, both by the client in
determining the order of operations and by the server in determining
whether the NFS4ERR_OLD_STATEID is to be returned, the possibility of
the seqid being swapped around past the NFS4_UINT32_MAX value needs
to be taken into account.
The definition of the seqid4 type in s2.1 doesn't mention this sort of
constraint.
Incrementing of seqid4 type fields is mentioned in a number of
sections along with special values (including 0):
s9.1.3.1, bullet 1 (also bullet 2):
Such stateids are subject
to change (with consequent incrementing of the stateid's seqid) in
response to OPENs that result in upgrade and OPEN_DOWNGRADE
operations.
s9.1.3.3: Refers to special values of seqid (all zeroes, all ones)
s9.1.3.4, two bullets before last bullet have:
o If the "seqid" field is not zero, and it is greater than the
current sequence value corresponding the current "other" field,
return NFS4ERR_BAD_STATEID.draft-ietf-nfsv4-rfc3530bis
o If the "seqid" field is less than the current sequence value
corresponding the current "other" field, return
NFS4ERR_OLD_STATEID.
s9.11, last para:
The "seqid" value in the returned stateid
MUST be incremented, even in situations in which there is no change
to the access and deny bits for the file.
s15.8.5, para 3 before last:
In this case, the
stateid returned as an "other" field that matches that of the
previous open while the "seqid" field is incremented to reflect the
change status due to the new open.
It would be much cleaner if the texts from s9.1.3.2 and s9.1.6 were
merged, probably into a section just after 2.1 (say s2.1.1) together
draft-ietf-nfsv4-rfc3530biswith mention of the special values. If
this is done, s2.1.1 can then be referenced wherever a seqid is
mentioned as being incremented.
Also, I cannot see why the current texts use SHOULD. I am pretty
certain that skipping 0 and going to 1 from NFS4_UINT32_MAX is a MUST.
If it isn't a MUST, what alternative scheme will make the wrap around
work without both client and server knowing what scheme is being used?
This is at least partly implied by the definition of NFS4ERR_BAD_SEQID
in s13.1.8.2 which indicates that both ends have a common idea of what
the next expected number ought to be..
-----------------------------------
change_info4: Wrap around in before and after fields.
======================================================
I assume that it is deemed that using a 64 bit number as changeid4
should obviate the need for dealing with wrap around. However I don't
think there is any advice on starting values for the change values -
some strategies (e.g., random values) could result in change
indicators starting uncomfortably close to the maximum value and
running up against wrap around. A brief note on this would be desirable.
(see s10.4.3 which talks about adding 1 to a change_info4 value.)
—————————————————
<— to here —>
s10.7: Informing the server that a file is being memory mapped.
Is there a mechanism? If not should there be (e.g., a flag on OPEN)?
I stumbled on this as well, until I noticed this:
o If there is an application on the server that has memory mapped a
file that a client is also accessing, the client may not be able
to get a consistent value of the change attribute to determine
whether its cache is stale or not.
I.e., the server only knows about locally memory mapped files.
Perhaps better expressed as the server *doesn't* know about locally
memory mapped files if they are being accessed behind the server's back
through the underlying file system. There would be some very limited
point to having a mechanism (flag on open, as suggested) that allowed a
client to inform the server that a file was being memory mapped, but
this would of course require OS support but could give a way of
overcoming the issue recorded at the end of bullet 3 in s10.7:
Depending on the
nature of the client's memory management system, this weak
obligation may not be possible. A client MAY return stale
information in CB_GETATTR whenever the file is memory mapped.
Probably not worth worrying about.
-----------------------------------
s15.2.3, paras 4 and 7: The last sentence in para 7 that implies
NFS4ERR_MINOR_VERSION_MISMATCH takes priority over all others appears
to be incompatible with the second XDR decoding strategy in para 4.
If decoding of the operation array is interleaved with performing the
requested operations then it seems possible that the COMPOUND may be
terminated by an earlier error before the offending operation 2 is
reached, so that it would never be decoded. Further, I don't
understand why operation 2 is singled out for special discussion here,
other than maybe to say that it is available for future use.
I think it is just because it is reserved for future use with minor
versioning.
I.e., other operations having values might also be bad, but if they do,
we can assume it is because they are not involved with minor versioning.
How does it differ from any other value that is not supported by the
minorversion implemented by the server? Why should the base version
be singled out for special treatment?
Having checked out what is signed off in RFC 5661 for v4.1 and proposed
for v4.2 in the current draft, and given the demise of most of s11,
wouldn't it be easier just to make operation 2 reserved for ever? I
can't see that a distinguished op 2 is ever that important.
Nits/editorial comments:
========================
s1.3.3.2: Worth reinforcing that attribute names are (presumably)
encoded in utf8 as they are just fileobject names.
I’m not sure that strictly follows - see S 12.8 for example.
Indeed, it is conceivable that in different parts of the name space of
the server, different character sets are possible (because of different
underlying file systems). That has the corollary that moving a
filesystem object may result in an error because the target rejects the
name.
s1,4, Definition of Stateid: Definitions of open-owner and lock-owner
would be helpful. The terms are used a few times before they are
defined in s9.
Agreed:
<t hangText="Lock-Owner:">
Each byte-range lock is associated with a specific lock-owner
and an open-owner. The lock-owner consists of a
Client ID and an opaque owner string.
The client presents this to the server to establish
the ownership of the byte-range lock as needed.
</t>
<t hangText="Open-Owner:">
Each open file is associated with a specific open-owner,
which consists of a Client ID and an opaque owner string.
The client presents this to the server to establish
the ownership of the open as needed.
</t>
That looks good.
s3.3: The terms 'policy boundary' and 'crossing policy boundaries' are
used in subsequent sections but not explicitly defined. I assume that
they refer to the client accessing fileobjects in areas accessed via
the different entry points referred to in s3.3. Assuming I am right
introducing the 'policy boundary' term in s3.3 would be helpful.
The document strives very hard not to discuss export mechanisms:
kitty:linux loghyr$ more /etc/exports
/ *(rw)
/home/loghyr *(rw)
/pynfs *(rw,no_root_squash,no_all_squash,insecure)
/public *(rw,no_root_squash,no_all_squash,insecure)
Each one of these define a “policy boundary” in the namespace.
OLD:
In general, the client will not have to use
the SECINFO operation except during initial communication with the
server or when the client crosses policy boundaries at the server.
It is possible that the server's policies change during the client's
interaction therefore forcing the client to negotiate a new security
triple.
NEW:
In general, the client will not have to use
the SECINFO operation except during initial communication with the
server or when the client encounters a new security policy as the
client navigates the name space. Either condition will force the
client to negotiate a new security triple.
I think that will address the issue.
s5.3: I think it would be worth being explicit as to whether hard
links can be created between named attribute fileobjects either (1)
for named attributes of the same fileobject and/or (2) for two
different fileobjects on the same file system. Is it also the case
that if the basic file system supports hard links (link_support ==
TRUE) then it will necessarily support hard links between named
attributes? As I understand it, the named attribute system in NFSv4
was modelled on Solaris v9+ and the extended attribute system in Linux
would probably not support hard links (I haven't tried them (yet))
even if the Solaris version does.... Later... I see from s13.1.4.14
(NFS4ERR_XDEV) that hard links between different named attribute
directories are NOT allowed. It would be good to make this explicit
here in s5.3. This still leaves the question of whether hard links
are allowed in a single named attribute directory. I can't see a lot
of use for this capability but it should be explicitly allowed or
disallowed as appropriate.
This is not addressed in 3530, but it is in 5661 (section 18.9.3), which
is equivalent to 15.29.5 in 3530bis:
Shepler, et al. Standards Track [Page 425]
^L
RFC 5661 NFSv4.1 January 2010
On some servers, "." and ".." are illegal values for newname and the
error NFS4ERR_BADNAME will be returned if they are specified.
When the current filehandle designates a named attribute directory
and the object to be linked (the saved filehandle) is not a named
attribute for the same object, the error NFS4ERR_XDEV MUST be
returned. When the saved filehandle designates a named attribute and
the current filehandle is not the appropriate named attribute
directory, the error NFS4ERR_XDEV MUST also be returned.
I.e., it implies that if the current filehandle is the appropriate named
attribute
directory, the LINK is okay.
We could either explicitly state in Section 5.3 that hard links are
legal here or
change the LINK IMPLEMENTATION Section to follow that of 5661.
I am partial to syncing the two sections. Any concern?
No. That seems entirely sensible.
s5.6: There is no definition of nfs_lease4 in either this draft or the
xdr draft.
So nfs_lease4 is in RFC3530, but again with no corresponding XDR.
5661 has it fixed.
Okay, the xdr draft now has a fix for it. Great catch!
Great!
s5.10, last para: Is it conceivable that a client would need to know
what version of Unicode a server was using ?
No
If so should there be a way for the client to find out what version
of Unicode is in use by the server?
<Shudder />
<Cowards /> ;-) Alright, I'll believe you!
s6.1:
* Setting only the mode attribute should provide reasonable
security. For example, setting a mode of 000 should be enough
to ensure that future opens for read or write by any principal
fail, regardless of a previously existing or inherited ACL.
I think this may only apply to non-privileged principals - root can
still read/write *ix files with mode 0. Tie in with last para of
s6.3.1.1.
Which is why this is a “should” and not a “SHOULD” or a “MUST”?
s6.4.3: It is (still) not clear (at least, to me) how inheritance is
supposed to apply to named attribute directories. The named attribute
directory has the file object to which it applies as its 'parent'
rather than an ordinary directory. Presumably inheritance would apply
from the named attribute directory to the named attributes created in
it - but are ACLs always supported on named attributes and the named
attribute directory if they are suported on ordinary files and
directories?
David??
s7.8, para 1: I am unable to parse the 3rd sentence in this para:
However, with the support of multiple security mechanisms
and the ability to negotiate the appropriate use of these mechanisms,
the server is unable to properly determine if a client will be able
to authenticate itself.
And the authors of 5661 seem to agree. It is also S7.8 in that document.
What the sentence means is that since the server has no way to determine
how the client will authenticate in the presence of multiple security
mechanisms,
it should as 5661 states:
Because NFSv4 clients possess the ability to change the security
mechanisms used, after determining what is allowed, by using SECINFO
and SECINFO_NONAME, the server SHOULD NOT present a different view of
the namespace based on the security mechanism being used by a client.
Instead, it should present a consistent view and return
NFS4ERR_WRONGSEC if an attempt is made to access data with an
inappropriate security mechanism.
I’d be willing to pull Section 7.8 from 5661 back to 3530bis - besides using
security policy as a term, it appears easier to read. :-)
I agree. Notes for the 5661 version
- Remove ref to SECINFO_NONAME from para 1
- s/the entire the pseudo file system/the entire pseudo file system/ in
next to last para.
s8.5:
If a single location entry designates multiple server IP addresses,
the client cannot assume that these addresses are multiple paths to
the same server. In most cases, they will be, but the client MUST
verify that before acting on that assumption.
Is there some means in NFSv4 to do this? I couldn't see that there
was. I guess one could use Reverse DNS. If an out-of-band
verification is needed, this should be made explicit.
The client could access each IP address and examine the resulting
ClientID from each.
But even then, I think that is not sufficient. David?
s9.1.5, para 2 and elsewhere in s9; also s10.4.1:
If no state is established by the client, either
byte-range lock or share reservation, a stateid of all bits 0 is
used.
Does this differ from the 'anonymous stateid' defined in s9.1.3.3? If
not it would be better to refer to it by this title. There are a
number of other places in s9 where the expressions like 'stateid of
all 0s/1s' are used. It would be more consistent to use the
'anonymous' and 'READ bypass' titles set up in s9.1.3.3 in all cases.
David...
s9.6.3.1, 3rd bullet: s/haled as courtesy/haled as a courtesy/
also s/haled/held/
... and there I was thinking you had used a nice archaic form... :-)
s9.8, para 5:
The client may accomplish this task by
issuing an I/O request, either a pending I/O or a zero-length read,
specifying the stateid associated with the lock in question.
How do you issue a "pending I/O request”??
I think the intent is one that was pending or if none were pending, fake
it with a zero-length read.
Perhaps, NEW:
The client may accomplish this task by
issuing an I/O request; if there is no relevant I/O pending,
a zero-length read specifying the stateid associated with the lock
in question can be synthesised to trigger the renewal.
s10.2.1, para 9:
Note that the requirement stated above is not meant to imply that
when the client is no longer obliged, as required above, to retain
delegation information, that it should necessarily dispose of it.
Some specific cases are:
I think this para is referring to the server rather than the client:
s/client/server/.
Dave, can you look at this one?
http://www.ietf.org/mail-archive/web/nfsv4/current/msg10668.html
s12.4: Has the dreaded BOM ever been encountered in the UTF-8 strings
created by NFSv5 servers or clients?
Therefore, the mask returned enumerating which access rights can be
determined will have the ACCESS4_DELETE value set to 0.
David?
s15.2.5:
The client SHOULD NOT construct a COMPOUND which mixes operations for
different client IDs.
Is this advisory on the grounds that it makes recovery difficult - in
which case s/SHOULD NOT/should not/ - or something that might result
in the server throwing an error - in which case please point to some
discussion of why this is not legal. Later... this is also considered
in s15.18.6, last para. So there is one set of circumstances when it
will screw up - but there are others where it is not problematical.
Perhaps this can be explained more clearly.
Are these not the same?
Yes, the requirement on the client is indeed the same. The ordering of
the text in s15.2.5 seemed to indicate that changing the Client ID
might make recovery from resource availability problems more difficult
but s15.18.6 makes it clear this isn't the reason. What triggered my
comment was that the (earlier) s15.2.5 version has no explanation of why
it is problematical. A pointer to s15.18.6 in s15.2.5 would help
alleviate the problem but is not a total solution.
Further, the client SHOULD
NOT construct a COMPOUND which mixes operations for different client
IDs.
David, are there migration scenarios where the client might be
presenting different client IDs?
Effectively OPEN with a successful OPEN_DELEGATE_WRITE creates transient
state in the server for the duration of the enclosing COMPOUND
procedure, essentially contravening the statement in the last para of s4.3:
Note that the COMPOUND procedure does not provide atomicity.
The first question is are there any other circumstances that generate
transient state in a similar way or is this the only example?
Later... after some considerable thinking about what is going on here.
Presumably the basic problem here is that if the the client is single
threaded or otherwise would get its knickers twisted if it issued a
GETATTR for a file on which it held a write delegation thus having its
internal state for the file locked when the CB_GETATTR arrived. This is
clearly potentially deadlock time!
Given that the client has the write delegation, it shouldn't need to
call GETATTR except in the one initial circumstance documented.
It seems to me that the server could ALWAYS suppress the CB_GETATTR if
the clientid is set for the client that owns the write delegation, and
the client could be instructed that it MUST NOT call GETATTR on a file
for which it has a write delegation other than in the one special
circumstance (or maybe unless it knows the clientid is set correctly).
Is my analysis right? If so I think this area can be made clearer (and
possibly more robust).
s15.11: Should this section talk about maxlink and the return of
NFS4ERR_MLINK?
It could, is it necessary? No.
s15.16.4: LOOKUPP is said to return the parent directory of a named
attribute directory. Arguably a named attribute directory doesn't have
a parent... so does LOOKUPP return the directory in which the object
to 'owning' the named attribute directory resides? Or does it return
the owning object? Please clarify in the text.
Good question.
From reading the text, it should return the parent of the directory in
which the object to 'owning' the named attribute directory resides.
I’ve asked on list.
Ahh, 5661 to the rescue:
If the current filehandle is a named attribute directory that is
associated with a file system object via OPENATTR (i.e., not a sub-
directory of a named attribute directory), LOOKUPP SHOULD return the
filehandle of the associated file system object.
I have taken that paragraph back into 3530bis,
Fine.
s15.18.2/s15.18.6/s15.21.2/s15.21.4, share_access and share_deny
arguments: Presumably the shorthand DENY/READ/WRITE/BOTH values
referred to are the relevant constants defined in s9.9? If so please
add a reference (and maybe use the full names in s15). Otherwise
please define the values.
Full names it is - btw - I agree, easier to read, but harder to map back
to the real values.
And I did provide a link in the “one” which used the terms heavily.
OK.
s15.18.6, para 9:
If the component provided to OPEN resolves to something other than a
regular file
Worth pointing out this includes named attribute files.
Do you mean:
If the component provided to OPEN resolves to something other than a
regular file (or a named attribute)
Yes.
s15.26.4/s15.26.5: What happens if the directory has been modified
(entries added or deleted) between calls of READDIR intended to
retrieve segments of the directory information? Related to this,
presumably no ordering of entries is implied in the returned
information - it would be desirable to state this.
This is where the cookieverf is supposed to come in.
In most implementations, the cookie needs to be persistent, which indeed
means an ordering of the information.
AFAICS the text doesn't tie the cookieverf to the change attribute of
the directory. Presumably this is what is intended - it would certainly
mean that changes would be picked up and READDIR would have to start
from the beginning again.
s17, para 8: s/see 5.9 and 12/see Sections 5.9 and 12/
I’ll have to track down what is happening here, might be with the newer
xml2rfc:
string comparison - see Sections
<xref target="sec:fattr:owner_group" format="counter" /> and
<xref target="sec:i18n" format="counter" /> for further
Hmm, this is generating correctly in draft 34.
Right. .. and that is what I would expect to see in xml2rfc!
I’ll make sure to track this with every new draft version.
Fine.
s18.1, para 4: I don't believe you can have both policies of FCFS and
Specification Required at the same time, and then with Expert Review
for updates. The proposal sounds like FCFS with a requirement that a
description of format and intended usage is provided.
Already fixed in version 34.
Amazing how it got through 3 different prior reviews (3530, 5661, and
first 3530bis). :-)
:-))))
s19.2/s5.6: I guess it was intended... s/ISEG_errata/IESG_ERRATA/
Ok, first part is fixed, will also change case.
OOPS! I didn't mean to put ERRATA in caps. v34 is just fine. Sorry.
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
