| Message ID | a76cec59-f007-5c26-dc26-4cc8a6e4daef@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
On 1/17/2017 01:03, Matthieu Herrb wrote: > 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 : That patch is for another bug of nfsv4.2, not for your problem, so that, you can see the issue again. Olga Kornievskaia said, "That is a rather old NetApp release. Perhaps they've fix something. I've just tried their latest 9.0 release and 4.9 upstream kernel and file is created with 0644." Can you test it in 9.0 release? Also exist? thanks, Kinglong Mee > > 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@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ping... What's the state? The problem is also exist in the latest kernel. Also, the patch should be updated based on the latest kernel. thanks, Kinglong Mee On 1/7/2017 22:45, 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. > > #include <sys/types.h> > #include <sys/stat.h> > #include <err.h> > #include <fcntl.h> > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > #include <unistd.h> > > /* > * Demonstrate file creation bug on NFS v4 and linux kernel 4.4+ > * > * mktemp() is used on purpose. > */ > int > main(int argc, char *argv[]) > { > const char *name = argv[1]; > char tmp[] = "./tmpXXXXXXXXXX"; > struct stat buf; > mode_t expected; > int fd, i, n = 40; > > umask(S_IWGRP | S_IWOTH); > expected = 0666 & ~(S_IWGRP | S_IWOTH); > if (argv[1] == NULL) > name = mktemp(tmp); > for (i = 0; i < n; i++) { > fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0666); > if (fd < 0) > err(1, "open %s", name); > memset(&buf, 0, sizeof(buf)); > if (stat(name, &buf) < 0) > err(1, "stat %s", name); > if ((buf.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != expected) > printf("%s: %o\n", name, > (int)buf.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)); > else > printf("%s: ok\n", name); > unlink(name); > } > exit(0); > } > > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > --- > fs/nfs/nfs4proc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > 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 (attrset[2] & FATTR4_WORD2_SECURITY_LABEL) > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 (attrset[2] & FATTR4_WORD2_SECURITY_LABEL)
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. #include <sys/types.h> #include <sys/stat.h> #include <err.h> #include <fcntl.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <unistd.h> /* * Demonstrate file creation bug on NFS v4 and linux kernel 4.4+ * * mktemp() is used on purpose. */ int main(int argc, char *argv[]) { const char *name = argv[1]; char tmp[] = "./tmpXXXXXXXXXX"; struct stat buf; mode_t expected; int fd, i, n = 40; umask(S_IWGRP | S_IWOTH); expected = 0666 & ~(S_IWGRP | S_IWOTH); if (argv[1] == NULL) name = mktemp(tmp); for (i = 0; i < n; i++) { fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0666); if (fd < 0) err(1, "open %s", name); memset(&buf, 0, sizeof(buf)); if (stat(name, &buf) < 0) err(1, "stat %s", name); if ((buf.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != expected) printf("%s: %o\n", name, (int)buf.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)); else printf("%s: ok\n", name); unlink(name); } exit(0); } Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> --- fs/nfs/nfs4proc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)