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@xxxxxxx> 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@xxxxxxxxxx>
>>>> ---
>>>> 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
