On Fri, Oct 11, 2024 at 08:31:09AM +1100, NeilBrown 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.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> 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 4c5deea0e953..885a58ca33d8 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;From 226d19d36029605a16d837d1a94a6f81b6e06681 Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@xxxxxxx>
> Date: Fri, 11 Oct 2024 08:23:08 +1100
>
>
>
> - /*
> - * 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))
> + /* 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 c625966cfcf3..b7abf1cba9e2 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: 144cb1225cd863e1bd3ae3d577d86e1531afd932
> prerequisite-patch-id: 7a049ff3732fdc61d6d4ff6f916f35341eddfa04
> --
> 2.46.0
>
This makes a lot of sense.
Let's get my nfsd4_lock() patch squared away and applied to
nfsd-next, then I can apply a v2 of this one on top of that.
--
Chuck Lever
