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 <[email protected]>
---
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)
--
2.9.3
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 [email protected]
> 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 [email protected]
>> 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 [email protected]
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
--
Matthieu Herrb
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 [email protected]
>>>> 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 <[email protected]>
> ---
> 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)
>