On 3/20/25 4:46 PM, Olga Kornievskaia wrote:
> On Thu, Mar 20, 2025 at 1:32 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>>
>> On 3/20/25 12:29 PM, Olga Kornievskaia wrote:
>>> On Thu, Mar 20, 2025 at 9:58 AM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>> index 4021b047eb18..95d973136079 100644
>>>>> --- a/fs/nfsd/vfs.c
>>>>> +++ b/fs/nfsd/vfs.c
>>>>> @@ -2600,6 +2600,10 @@ nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
>>>>
>>>> More context shows there is an NFSD_MAY_OWNER_OVERRIDE check just
>>>> before the new logic added by this patch:
>>>>
>>>> if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
>>>>
>>>>> uid_eq(inode->i_uid, current_fsuid()))
>>>>> return 0;
>>>>>
>>>>> + /* If this is NLM, require read or ownership permissions */
>>>>> + if (acc & NFSD_MAY_NLM)
>>>>> + acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
>>>>> +
>>>>
>>>> So I'm wondering if this new test needs to come /before/ the existing
>>>> MAY_OWNER_OVERRIDE check here? If not, the comment you add here might
>>>> explain why it is correct to place the new logic afterwards.
>>>
>>> Why would you think this check should go before?
>>
>> Because this code is confusing.
>>
>> So, for NLM, MAY_OWNER_OVERRIDE is already set for the uid_eq check.
>> The order of the new code is not consequential. But it still catches
>> the eye.
>
> I just re-checked the original code and the re(set)ing of acc was done
> before the MAY_OWNER_OVERRIDE check. So changing my patch as you
> suggest would be consistent with how things worked before. I will do
> that.
>
>>> NFS_MAY_OWNER_OVERRIDE is actually already set by nlm_fopen() when it
>>> arrives in check_nfsd_access() .
>>
>> This code is /clearing/ the other flags too. That's a little subtle.
>>
>> The new comment should explain why only these two flags are needed for
>> the subsequent code.
>
> inode_permission() only care about READ, WRITE (EXEC)? NFSD_MAY_READ
> is supposed to be the same as MAY_READ etc. Thus I believe passing
> other values is of no consequence so to be honest I don't understand
> why NFSD_MAY_OWNER_OVERRIDE is needed here. But what is of consequence
> is (not) passing a NFSD_MAY_WRITE (which is what gets passed in by
> nlm_fopen()).
>
>> That is, explain not only why NFSD_MAY_READ is
>> getting set, but also why NFSD_MAY_NLM and NFSD_MAY_BYPASS_GSS are
>> getting cleared.
>>
>> Or, if clearing those flags was unintentional, make the code read:
>>
>> acc |= NFSD_MAY_READ;
>>
>> I don't understand this code well enough to figure out why nlm_fopen()
>> shouldn't just set NFSD_MAY_READ. Therefore: better code comment needed.
>
> My comment came from the comment that was removed by commit
> 4cc9b9f2bf4df. I don't have a good understanding of the code to
> provide a "better" comment.
>
> nlm_fopen assigns the access based on the requested lock type. an
> exclusive lock i'm assuming gets translated to the "write" =
> NFSD_MAY_WRITE. But the the user might not have write access to the
> file but still be allowed to request an exclusive lock on it, right?
Agreed.
> @@ -2531,16 +2531,6 @@ nfsd_permission(struct svc_cred *cred, struct
> svc_export *exp,
> if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode))
> return nfserr_perm;
>
> - if (acc & NFSD_MAY_LOCK) {
> - /* If we cannot rely on authentication in NLM requests,
> - * just allow locks, otherwise require read permission, or
> - * ownership
> - */
> - if (exp->ex_flags & NFSEXP_NOAUTHNLM)
> - return 0;
> - else
> - acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
> - }
> /*
> * The file owner always gets access permission for accesses that
> * would normally be checked at open time. This is to make
The original comment isn't significantly more illuminating, true.
This seems clearer to me, but folks who know this code better might
feel it is a bit obvious:
/*
* For the purpose of permission checking NLM requests,
* the locker must have READ access or own the file.
*/
>>> Prior to the 4cc9b9f2bf4df inode_permission() would get NFSD_MAY_READ
>>> | NFSD_MAY_OWNER_OVERRIDE in access argument. After, it gets
>>> NFSD_MAY_WRITE | NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE |
>>> NFSD_MAY_BYPASS_GSS. It made inode_permission() fail the call. Isn't
>>> NLM asking for write permissions on the file for locking?
>>>
>>> My attempt was to return the code to previous functionality which
>>> worked (and was only explicitly asking for read permissions on the
>>> file).
>>
>> So the patch is reverting part of 4cc9b9f2bf4df; perhaps that should be
>> called out in the patch description (up to you). However, the proposed
>> fix doesn't make this code easier to understand, and that's probably my
>> main quibble.
>>
>>
>>> Unless you think more is needed here: I would resubmit with 3 patches
>>> for each of the chunks and corresponding problems.
>>
>> Agreed, and I don't think I have any substantial change requests for the
>> first two fixes.
>>
>>
>>>>> /* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
>>>>> err = inode_permission(&nop_mnt_idmap, inode,
>>>>> acc & (MAY_READ | MAY_WRITE | MAY_EXEC));
>>>>
>>>>
>>>> --
>>>> Chuck Lever
>>>>
>>
>>
>> --
>> Chuck Lever
>>
>
--
Chuck Lever
