On Sun, Jan 15, 2017 at 03:55:16PM +0800, Kinglong Mee wrote: > On 1/13/2017 04:47, J. Bruce Fields wrote: > > On Sat, Jan 07, 2017 at 10:45:47PM +0800, Kinglong Mee wrote: > >> Acorrding to Matthieu Herrb's test cases, a new created file will > >> get a bad mode as 0666 (expected 0644) after commit dff25ddb4808 > >> "nfs: add support for the umask attribute". > >> > >> It is caused by missing check of FATTR4_WORD2_MODE_UMASK > >> in nfs4_exclusive_attrset. > > > > I don't understand: > > > >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >> index 6dcbc5d..a3e9ef1 100644 > >> --- a/fs/nfs/nfs4proc.c > >> +++ b/fs/nfs/nfs4proc.c > >> @@ -2697,7 +2697,8 @@ static inline void nfs4_exclusive_attrset(struct nfs4_opendata *opendata, > >> sattr->ia_valid |= ATTR_MTIME; > >> > >> /* Except MODE, it seems harmless of setting twice. */ > >> - if ((attrset[1] & FATTR4_WORD1_MODE)) > >> + if ((attrset[1] & FATTR4_WORD1_MODE) || > >> + (attrset[2] & FATTR4_WORD2_MODE_UMASK)) > >> sattr->ia_valid &= ~ATTR_MODE; > > > > If I'm understanding this function correctly, attrset is the set of > > attributes which the server tells us were used to store the verifier. > > > > But mode_umask would never be a sensible place to store the > > verifier, so if the server's response really says that then something's > > wrong. > > There are some differences between EXCLUSIVE4 and EXCLUSIVE4_1, > according to rfc5661 18.16.4, > > After the client has performed a successful exclusive create, the > attrset response indicates which attributes were used to store the > verifier. If EXCLUSIVE4 was used, the attributes set in attrset were > used for the verifier. If EXCLUSIVE4_1 was used, the client > determines the attributes used for the verifier by comparing attrset > with cva_attrs.attrmask; any bits set in the former but not the > latter identify the attributes used to store the verifier. The > client MUST immediately send a SETATTR to set attributes used to > store the verifier. Until it does so, the attributes used to store > the verifier cannot be relied upon. The subsequent SETATTR MUST NOT > occur in the same COMPOUND request as the OPEN. > > I think, this patch is a hacker implement for EXCLUSIVE4_1 that just > treat the FATTR4_WORD1_TIME_ACCESS and FATTR4_WORD1_TIME_MODIFY for > exclusive verifier as EXCLUSIVE4. > > Maybe we need update the implement of EXCLUSIVE4_1's verifier > checking. Hi, this patch doesn't fix the issue against our NetApp server (which is running an old version of the system as it has been noticed, but we cannot upgrade until a few months) . My test program is still getting a number of wrong issuess : host$ ./a.out foo foo: ok foo: ok foo: ok foo: ok foo: 700 foo: ok foo: ok foo: ok foo: ok foo: ok foo: ok foo: ok foo: ok foo: ok foo: 700 foo: ok foo: 700 foo: ok foo: ok foo: ok foo: 700 foo: 700 foo: 700 foo: ok foo: 700 foo: 700 foo: 700 foo: ok foo: 700 foo: ok foo: ok foo: ok foo: 700 foo: ok foo: ok foo: ok foo: ok foo: ok foo: ok foo: ok > > thanks, > Kinglong Mee > > > > > We should probably look at a network trace. > > > > --b. > > > >> > >> if (attrset[2] & FATTR4_WORD2_SECURITY_LABEL) > >> -- > >> 2.9.3 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > >> the body of a message to majordomo@xxxxxxxxxxxxxxx > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- Matthieu Herrb
Attachment:
signature.asc
Description: Digital signature
