Re: [PATCH 3/3] nfsd: reset access mask for NLM calls in nfsd_permission

[Chuck Lever <chuck.lever@oracle.com>]

On 3/27/25 9:43 PM, NeilBrown wrote:
> On Fri, 28 Mar 2025, Olga Kornievskaia wrote:
>> On Thu, Mar 27, 2025 at 7:54 PM NeilBrown <neilb@suse.de> wrote:
>>>
>>> On Sat, 22 Mar 2025, Olga Kornievskaia wrote:
>>>> NLM locking calls need to pass thru file permission checking
>>>> and for that prior to calling inode_permission() we need
>>>> to set appropriate access mask.
>>>>
>>>> Fixes: 4cc9b9f2bf4d ("nfsd: refine and rename NFSD_MAY_LOCK")
>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>>> ---
>>>>  fs/nfsd/vfs.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>> index 4021b047eb18..7928ae21509f 100644
>>>> --- a/fs/nfsd/vfs.c
>>>> +++ b/fs/nfsd/vfs.c
>>>> @@ -2582,6 +2582,13 @@ nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
>>>>       if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode))
>>>>               return nfserr_perm;
>>>>
>>>> +     /*
>>>> +      * For the purpose of permission checking of NLM requests,
>>>> +      * the locker must have READ access or own the file
>>>> +      */
>>>> +     if (acc & NFSD_MAY_NLM)
>>>> +             acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
>>>> +
>>>
>>> I don't agree with this change.
>>> The only time that NFSD_MAY_NLM is set, NFSD_MAY_OWNER_OVERRIDE is also
>>> set.  So that part of the change adds no value.
>>>
>>> This change only affects the case where a write lock is being requested.
>>> In that case acc will contains NFSD_MAY_WRITE but not NFSD_MAY_READ.
>>> This change will set NFSD_MAY_READ.  Is that really needed?
>>>
>>> Can you please describe the particular problem you saw that is fixed by
>>> this patch?  If there is a problem and we do need to add NFSD_MAY_READ,
>>> then I would rather it were done in nlm_fopen().
>>
>> set export policy with (sec=krb5:...) then mount with sec=krb5,vers=3,
>> then ask for an exclusive flock(), it would fail.
>>
>> The reason it fails is because nlm_fopen() translates lock to open
>> with WRITE. Prior to patch 4cc9b9f2bf4d, the access would be set to
>> acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE; before calling into
>> inode_permission(). The patch changed it and lead to lock no longer
>> being given out with sec=krb5 policy.
> 
> And do you have WRITE access to the file?
> 
> check_fmode_for_setlk() in fs/locks.c suggests that for F_WRLCK to be
> granted the file must be open for FMODE_WRITE.
> So when an exclusive lock request arrives via NLM, nlm_lookup_file()
> calls nlm_do_fopen() with a mode of O_WRONLY and that causes
> nfsd_permission() to check that the caller has write access to the file.
> 
> So if you are trying to get an exclusive lock to a file that you don't
> have write access to, then it should fail.
> If, however, you do have write access to the file - I cannot see why
> asking for NFSD_MAY_READ instead of NFSD_MAY_WRITE would help.

A little context:

3/3 partially reverts 4cc9b9f2bf4d. Setting exactly READ / OVERRIDE for
NLM requests is what nfsd_permission() had done for many years before
4cc9b9f2bf4d. Thus I regard this as a safe thing to do at the moment.

I agree, however, that it is mysterious why that should work at all, and
I'm fine with holding off on 3/3 until we have a clearer RCA.

Initially I thought changing nlm_fopen() would be a better approach,
but I think there are other consumers of the MAY flags set by
nlm_fopen() that could be impacted by such a change.


> NeilBrown
> 
> 
>>
>>
>>>
>>> Thanks,
>>> NeilBrown
>>>
>>>
>>>>       /*
>>>>        * The file owner always gets access permission for accesses that
>>>>        * would normally be checked at open time. This is to make
>>>> --
>>>> 2.47.1
>>>>
>>>>
>>>
>>
>>
> 


-- 
Chuck Lever