| Message ID | 172859586932.444407.11362678410508147927@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [nfsd-next] nfsd: refine and rename NFSD_MAY_LOCK | expand |
On Fri, 2024-10-11 at 08:31 +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@suse.de> > --- > 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@suse.de> > Date: Fri, 11 Oct 2024 08:23:08 +1100 > ^^^ This patch appears to be mangled. > > > - /* > - * 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 The patch itself makes sense though.
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@suse.de> > --- > 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@suse.de> > 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.
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@suse.de> 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: */
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@suse.de> --- 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(-) base-commit: 144cb1225cd863e1bd3ae3d577d86e1531afd932 prerequisite-patch-id: 7a049ff3732fdc61d6d4ff6f916f35341eddfa04