Re: [PATCH] NFSv4.2: Fix file creating with O_EXCL get a bad mode

[Kinglong Mee <kinglongmee@gmail.com>]

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
>>>
>>
>