On Wed, 12 Mar 2025, Olga Kornievskaia wrote: > On Tue, Mar 11, 2025 at 11:28 AM Olga Kornievskaia <aglo@xxxxxxxxx> wrote: > > > > On Thu, Oct 17, 2024 at 5:42 PM NeilBrown <neilb@xxxxxxx> 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@xxxxxxx> > > > --- > > > > > > 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? 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 > > > > > > >