Re: [PATCH v2] nfsd: refine and rename NFSD_MAY_LOCK

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

On 3/17/25 1:48 PM, Olga Kornievskaia wrote:
> On Wed, Mar 12, 2025 at 8:09 PM NeilBrown <neilb@suse.de> wrote:
>>
>> On Thu, 13 Mar 2025, Olga Kornievskaia wrote:
>>> On Wed, Mar 12, 2025 at 9:22 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>
>>>> On Tue, Mar 11, 2025 at 5:42 PM NeilBrown <neilb@suse.de> wrote:
>>>>>
>>>>> On Wed, 12 Mar 2025, Olga Kornievskaia wrote:
>>>>>> On Tue, Mar 11, 2025 at 11:28 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>>>
>>>>>>> On Thu, Oct 17, 2024 at 5:42 PM NeilBrown <neilb@suse.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> NFSD_MAY_LOCK means a few different things.
>>>>>>>> - it means that GSS is not required.
>>>>>>>> - it means that with NFSEXP_NOAUTHNLM, authentication is not required
>>>>>>>> - it means that OWNER_OVERRIDE is allowed.
>>>>>>>>
>>>>>>>> None of these are specific to locking, they are specific to the NLM
>>>>>>>> protocol.
>>>>>>>> So:
>>>>>>>>  - rename to NFSD_MAY_NLM
>>>>>>>>  - set NFSD_MAY_OWNER_OVERRIDE and NFSD_MAY_BYPASS_GSS in nlm_fopen()
>>>>>>>>    so that NFSD_MAY_NLM doesn't need to imply these.
>>>>>>>>  - move the test on NFSEXP_NOAUTHNLM out of nfsd_permission() and
>>>>>>>>    into fh_verify where other special-case tests on the MAY flags
>>>>>>>>    happen.  nfsd_permission() can be called from other places than
>>>>>>>>    fh_verify(), but none of these will have NFSD_MAY_NLM.
>>>>>>>
>>>>>>> This patch breaks NLM when used in combination with TLS.
>>>>>>
>>>>>> I was too quick to link this to TLS. It's presence of security policy
>>>>>> so sec=krb* causes the same problems.
>>>>>>
>>>>>>>  If exports
>>>>>>> have xprtsec=tls:mtls and mount is done with tls/mtls, the server
>>>>>>> won't give any locks and client will get "no locks available" error.
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> No change from previous patch - the corruption in the email has been
>>>>>>>> avoided (I hope).
>>>>>>>>
>>>>>>>>
>>>>>>>>  fs/nfsd/lockd.c | 13 +++++++++++--
>>>>>>>>  fs/nfsd/nfsfh.c | 12 ++++--------
>>>>>>>>  fs/nfsd/trace.h |  2 +-
>>>>>>>>  fs/nfsd/vfs.c   | 12 +-----------
>>>>>>>>  fs/nfsd/vfs.h   |  2 +-
>>>>>>>>  5 files changed, 18 insertions(+), 23 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
>>>>>>>> index 46a7f9b813e5..edc9f75dc75c 100644
>>>>>>>> --- a/fs/nfsd/lockd.c
>>>>>>>> +++ b/fs/nfsd/lockd.c
>>>>>>>> @@ -38,11 +38,20 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
>>>>>>>>         memcpy(&fh.fh_handle.fh_raw, f->data, f->size);
>>>>>>>>         fh.fh_export = NULL;
>>>>>>>>
>>>>>>>> +       /*
>>>>>>>> +        * Allow BYPASS_GSS as some client implementations use AUTH_SYS
>>>>>>>> +        * for NLM even when GSS is used for NFS.
>>>>>>>> +        * Allow OWNER_OVERRIDE as permission might have been changed
>>>>>>>> +        * after the file was opened.
>>>>>>>> +        * Pass MAY_NLM so that authentication can be completely bypassed
>>>>>>>> +        * if NFSEXP_NOAUTHNLM is set.  Some older clients use AUTH_NULL
>>>>>>>> +        * for NLM requests.
>>>>>>>> +        */
>>>>>>>>         access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
>>>>>>>> -       access |= NFSD_MAY_LOCK;
>>>>>>>> +       access |= NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE | NFSD_MAY_BYPASS_GSS;
>>>>>>>>         nfserr = nfsd_open(rqstp, &fh, S_IFREG, access, filp);
>>>>>>>>         fh_put(&fh);
>>>>>>>> -       /* We return nlm error codes as nlm doesn't know
>>>>>>>> +       /* We return nlm error codes as nlm doesn't know
>>>>>>>>          * about nfsd, but nfsd does know about nlm..
>>>>>>>>          */
>>>>>>>>         switch (nfserr) {
>>>>>>>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>>>>>>>> index 40533f7c7297..6a831cb242df 100644
>>>>>>>> --- a/fs/nfsd/nfsfh.c
>>>>>>>> +++ b/fs/nfsd/nfsfh.c
>>>>>>>> @@ -363,13 +363,10 @@ __fh_verify(struct svc_rqst *rqstp,
>>>>>>>>         if (error)
>>>>>>>>                 goto out;
>>>>>>>>
>>>>>>>> -       /*
>>>>>>>> -        * pseudoflavor restrictions are not enforced on NLM,
>>>>>>>> -        * which clients virtually always use auth_sys for,
>>>>>>>> -        * even while using RPCSEC_GSS for NFS.
>>>>>>>> -        */
>>>>>>>> -       if (access & NFSD_MAY_LOCK)
>>>>>>>> -               goto skip_pseudoflavor_check;
>>>>>>>> +       if ((access & NFSD_MAY_NLM) && (exp->ex_flags & NFSEXP_NOAUTHNLM))
>>>>>>
>>>>>> I think this should either be an OR or the fact that "nlm but only
>>>>>> with insecurelock export option and not other" is the only way to
>>>>>> bypass checking is wrong. I think it's just a check for NLM that
>>>>>> stays.
>>>>>
>>>>> I don't think that NLM gets a complete bypass unless no_auth_nlm is set.
>>>>> For the case you are describing, I think NFSD_MAY_BYPASS_GSS is supposed
>>>>> to make it work.
>>>>>
>>>>> I assume the NLM request is arriving with AUTH_SYS authentication?
>>>>
>>>> It does.
>>>>
>>>> Just to give you a practical example. exports have
>>>> (rw,...,sec=krb5:krb5i:krb5p). Client does mount with sec=krb5. Then
>>>> does an flock() on the file. What's more I have just now hit Kasan's
>>>> out-of-bounds warning on that. I'll have to see if that exists on 6.14
>>>> (as I'm debugging the matter on the commit of the patch itself and
>>>> thus on 6.12-rc now).
>>>>
>>>> I will layout more reasoning but what allowed NLM to work was this
>>>> -       /*
>>>> -        * pseudoflavor restrictions are not enforced on NLM,
>>>> -        * which clients virtually always use auth_sys for,
>>>> -        * even while using RPCSEC_GSS for NFS.
>>>> -        */
>>>> -       if (access & NFSD_MAY_LOCK)
>>>> -               goto skip_pseudoflavor_check;
>>>>
>>>> but I don't know why the replacement doesn't work.
>>>
>>> As I mentioned the patch removed the skip_pseudoflavor check (that for
>>> NLM) would have bypassed calling check_nfsd_access(). Instead, the
>>> problem is that even though may_bypass_gss is set to true it call into
>>> nfsd4_spo_must_allow(rqstp) which now wrongly assumes there is
>>> compound state (struct nfsd4_compound_state *cstate = &resp->cstate;)
>>> ... (but this is NLM). So it proceed to deference it if
>>> (!cstate->minorversion) causing the KASAN to do the out-of-bound error
>>> that I mentioned. It most of the time now cause a crash. But on the
>>> off non-deterministic times when it completes it fails.
>>>
>>> I really don't think calling into check_nfsd_access() is appropriate for NLM.
>>
>> Why not?  What is there is inherently inappropriate for NLM?
> 
> I spoke too soon. It's not about calling into check_nfsd_acces()
> (though it's a problem for v3 because there isn't a compound state!).
> The real problem is "access" content that's being passed into the
> nfsd_permission() function.
> 
> I don't fully understand the logic. But before this patch, "access"
> (acc) was (re)set to "NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE" before
> calling into inode_permission():
> @@ -2534,16 +2534,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
> 
> now it's doesn't get "reset" and passes all of what was set in nlm_fopen()
> 
>        access = (mode == O_WRONLY) ? NFSD_MAY_WRITE : NFSD_MAY_READ;
> -       access |= NFSD_MAY_LOCK;
> +       access |= NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE | NFSD_MAY_BYPASS_GSS;
> 
> which ends up being "write" instead of a read and inode_permission returned -13.
> 
> Here's my proposed fix for one the problems in the patch.
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 4021b047eb18..eb139962ac4c 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -2600,6 +2600,9 @@ nfsd_permission(struct svc_cred *cred, struct
> svc_export *exp,
>             uid_eq(inode->i_uid, current_fsuid()))
>                 return 0;
> 
> +       if (acc & NFSD_MAY_NLM)
> +               acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
> +
>         /* 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));
> 
> Now the 2nd problem. You mentioned checking for version before calling
> nfsd4_spo_must_allow for v3. So something like this perhaps but not
> sure if that's right?
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 0363720280d4..0106da76da89 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1148,8 +1148,9 @@ __be32 check_nfsd_access(struct svc_export *exp,
> struct svc_rqst *rqstp,
>          * don't support it
>          */
> 
> -       if (nfsd4_spo_must_allow(rqstp))
> -               return nfs_ok;
> +       if (rqstp->rq_prog == 100003)

<shudder>

I guess we're stuck with this because this function is used for both
NFS and NLM file handles.

Let's use a symbolic constant here, at least.


> +               if (nfsd4_spo_must_allow(rqstp))
> +                       return nfs_ok;
> 
>         /* Some calls may be processed without authentication
>          * on GSS exports. For example NFS2/3 calls on root
> 
> 
> In the end, I question, why not revert the original patch instead?
> 
>> I agree that calling nfsd4_spo_must_allow(rqstp) isn't appropriate for
>> NLM, but it isn't appropriate for v2 or v3 either.  We should add
>> version checks either before it is called or before the minorversion
>> check that it already has.
>>
>> NeilBrown
>>
>>
>>>
>>>>
>>>>> So check_nfsd_access() is being called with may_bypass_gss and this:
>>>>>
>>>>>         if (may_bypass_gss && (
>>>>>              rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL ||
>>>>>              rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)) {
>>>>>                 for (f = exp->ex_flavors; f < end; f++) {
>>>>>                         if (f->pseudoflavor >= RPC_AUTH_DES)
>>>>>                                 return 0;
>>>>>                 }
>>>>>         }
>>>>>
>>>>> in check_nfsd_access() should succeed.
>>>>> Can you add some tracing and see what is happening in here?
>>>>> Maybe the "goto denied" earlier in the function is being reached.  I
>>>>> don't fully understand the TLS code yet - maybe it needs some test on
>>>>> may_bypass_gss.
>>>>>
>>>>> Thanks,
>>>>> NeilBrown
>>>>>
>>>>>
>>>>>>
>>>>>>>> +               /* NLM is allowed to fully bypass authentication */
>>>>>>>> +               goto out;
>>>>>>>> +
>>>>>>>>         if (access & NFSD_MAY_BYPASS_GSS)
>>>>>>>>                 may_bypass_gss = true;
>>>>>>>>         /*
>>>>>>>> @@ -385,7 +382,6 @@ __fh_verify(struct svc_rqst *rqstp,
>>>>>>>>         if (error)
>>>>>>>>                 goto out;
>>>>>>>>
>>>>>>>> -skip_pseudoflavor_check:
>>>>>>>>         /* Finally, check access permissions. */
>>>>>>>>         error = nfsd_permission(cred, exp, dentry, access);
>>>>>>>>  out:
>>>>>>>> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
>>>>>>>> index b8470d4cbe99..3448e444d410 100644
>>>>>>>> --- a/fs/nfsd/trace.h
>>>>>>>> +++ b/fs/nfsd/trace.h
>>>>>>>> @@ -79,7 +79,7 @@ DEFINE_NFSD_XDR_ERR_EVENT(cant_encode);
>>>>>>>>                 { NFSD_MAY_READ,                "READ" },               \
>>>>>>>>                 { NFSD_MAY_SATTR,               "SATTR" },              \
>>>>>>>>                 { NFSD_MAY_TRUNC,               "TRUNC" },              \
>>>>>>>> -               { NFSD_MAY_LOCK,                "LOCK" },               \
>>>>>>>> +               { NFSD_MAY_NLM,                 "NLM" },                \
>>>>>>>>                 { NFSD_MAY_OWNER_OVERRIDE,      "OWNER_OVERRIDE" },     \
>>>>>>>>                 { NFSD_MAY_LOCAL_ACCESS,        "LOCAL_ACCESS" },       \
>>>>>>>>                 { NFSD_MAY_BYPASS_GSS_ON_ROOT,  "BYPASS_GSS_ON_ROOT" }, \
>>>>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>>>>> index 51f5a0b181f9..2610638f4301 100644
>>>>>>>> --- a/fs/nfsd/vfs.c
>>>>>>>> +++ b/fs/nfsd/vfs.c
>>>>>>>> @@ -2509,7 +2509,7 @@ nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
>>>>>>>>                 (acc & NFSD_MAY_EXEC)?  " exec"  : "",
>>>>>>>>                 (acc & NFSD_MAY_SATTR)? " sattr" : "",
>>>>>>>>                 (acc & NFSD_MAY_TRUNC)? " trunc" : "",
>>>>>>>> -               (acc & NFSD_MAY_LOCK)?  " lock"  : "",
>>>>>>>> +               (acc & NFSD_MAY_NLM)?   " nlm"  : "",
>>>>>>>>                 (acc & NFSD_MAY_OWNER_OVERRIDE)? " owneroverride" : "",
>>>>>>>>                 inode->i_mode,
>>>>>>>>                 IS_IMMUTABLE(inode)?    " immut" : "",
>>>>>>>> @@ -2534,16 +2534,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
>>>>>>>> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
>>>>>>>> index 854fb95dfdca..f9b09b842856 100644
>>>>>>>> --- a/fs/nfsd/vfs.h
>>>>>>>> +++ b/fs/nfsd/vfs.h
>>>>>>>> @@ -20,7 +20,7 @@
>>>>>>>>  #define NFSD_MAY_READ                  0x004 /* == MAY_READ */
>>>>>>>>  #define NFSD_MAY_SATTR                 0x008
>>>>>>>>  #define NFSD_MAY_TRUNC                 0x010
>>>>>>>> -#define NFSD_MAY_LOCK                  0x020
>>>>>>>> +#define NFSD_MAY_NLM                   0x020 /* request is from lockd */
>>>>>>>>  #define NFSD_MAY_MASK                  0x03f
>>>>>>>>
>>>>>>>>  /* extra hints to permission and open routines: */
>>>>>>>>
>>>>>>>> base-commit: c4e418a53fe30d8e1da68f5aabca352b682fd331
>>>>>>>> --
>>>>>>>> 2.46.0
>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>>


-- 
Chuck Lever