[PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Scott Mayhew]

A while back I had reported that an NFSv3 client could successfully
mount using '-o xprtsec=none' an export that had been exported with
'xprtsec=tls:mtls'.  By "successfully" I mean that the mount command
would succeed and the mount would show up in /proc/mounts.  Attempting
to do anything futher with the mount would be met with NFS3ERR_ACCES.

This was fixed (albeit accidentally) by bb4f07f2409c ("nfsd: Fix
NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT") and was
subsequently re-broken by 0813c5f01249 ("nfsd: fix access checking for
NLM under XPRTSEC policies").

Transport Layer Security isn't an RPC security flavor or pseudo-flavor,
so they shouldn't be conflated when determining whether the access
checks can be bypassed.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 fs/nfsd/export.c   | 60 ++++++++++++++++++++++++++++++++++++----------
 fs/nfsd/export.h   |  1 +
 fs/nfsd/nfs4proc.c |  6 ++++-
 fs/nfsd/nfs4xdr.c  |  3 +++
 fs/nfsd/nfsfh.c    |  8 +++++++
 fs/nfsd/vfs.c      |  3 +++
 6 files changed, 67 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index cadfc2bae60e..bc54b01bb516 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1082,19 +1082,27 @@ static struct svc_export *exp_find(struct cache_detail *cd,
 }
 
 /**
- * check_nfsd_access - check if access to export is allowed.
+ * check_xprtsec_policy - check if access to export is permitted by the
+ * 			  xprtsec policy
  * @exp: svc_export that is being accessed.
  * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
- * @may_bypass_gss: reduce strictness of authorization check
+ *
+ * This logic should not be combined with check_nfsd_access, as the rules
+ * for bypassing GSS are not the same as for bypassing the xprtsec policy
+ * check:
+ * 	- NFSv3 FSINFO and GETATTR can bypass the GSS for the root dentry,
+ * 	  but that doesn't mean they can bypass the xprtsec poolicy check
+ * 	- NLM can bypass the GSS check on exports exported with the
+ * 	  NFSEXP_NOAUTHNLM flag
+ * 	- NLM can always bypass the xprtsec policy check since TLS isn't
+ * 	  implemented for the sidecar protocols
  *
  * Return values:
  *   %nfs_ok if access is granted, or
- *   %nfserr_wrongsec if access is denied
+ *   %nfserr_acces or %nfserr_wrongsec if access is denied
  */
-__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
-			 bool may_bypass_gss)
+__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp)
 {
-	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
 	struct svc_xprt *xprt;
 
 	/*
@@ -1110,22 +1118,49 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
 
 	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) {
 		if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags))
-			goto ok;
+			return nfs_ok;
 	}
 	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_TLS) {
 		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
 		    !test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
-			goto ok;
+			return nfs_ok;
 	}
 	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_MTLS) {
 		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
 		    test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
-			goto ok;
+			return nfs_ok;
 	}
-	if (!may_bypass_gss)
-		goto denied;
 
-ok:
+	return rqstp->rq_vers < 4 ? nfserr_acces : nfserr_wrongsec;
+}
+
+/**
+ * check_nfsd_access - check if access to export is allowed.
+ * @exp: svc_export that is being accessed.
+ * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
+ * @may_bypass_gss: reduce strictness of authorization check
+ *
+ * Return values:
+ *   %nfs_ok if access is granted, or
+ *   %nfserr_wrongsec if access is denied
+ */
+__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
+			 bool may_bypass_gss)
+{
+	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
+	struct svc_xprt *xprt;
+
+	/*
+	 * If rqstp is NULL, this is a LOCALIO request which will only
+	 * ever use a filehandle/credential pair for which access has
+	 * been affirmed (by ACCESS or OPEN NFS requests) over the
+	 * wire. So there is no need for further checks here.
+	 */
+	if (!rqstp)
+		return nfs_ok;
+
+	xprt = rqstp->rq_xprt;
+
 	/* legacy gss-only clients are always OK: */
 	if (exp->ex_client == rqstp->rq_gssclient)
 		return nfs_ok;
@@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
 		}
 	}
 
-denied:
 	return nfserr_wrongsec;
 }
 
diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
index b9c0adb3ce09..c5a45f4b72be 100644
--- a/fs/nfsd/export.h
+++ b/fs/nfsd/export.h
@@ -101,6 +101,7 @@ struct svc_expkey {
 
 struct svc_cred;
 int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp);
+__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp);
 __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
 			 bool may_bypass_gss);
 
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 71b428efcbb5..71e9a170f7bf 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2902,8 +2902,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 				clear_current_stateid(cstate);
 
 			if (current_fh->fh_export &&
-					need_wrongsec_check(rqstp))
+					need_wrongsec_check(rqstp)) {
+				op->status = check_xprtsec_policy(current_fh->fh_export, rqstp);
+				if (op->status)
+					goto encode_op;
 				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
+			}
 		}
 encode_op:
 		if (op->status == nfserr_replay_me) {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index ea91bad4eee2..48d55c13c918 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3859,6 +3859,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
 			nfserr = nfserrno(err);
 			goto out_put;
 		}
+		nfserr = check_xprtsec_policy(exp, cd->rd_rqstp);
+		if (nfserr)
+			goto out_put;
 		nfserr = check_nfsd_access(exp, cd->rd_rqstp, false);
 		if (nfserr)
 			goto out_put;
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 74cf1f4de174..1ffc33662582 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -364,6 +364,14 @@ __fh_verify(struct svc_rqst *rqstp,
 	if (error)
 		goto out;
 
+	if (access & NFSD_MAY_NLM)
+		/* NLM is allowed to bypass the xprtssec policy check */
+		goto out;
+
+	error = check_xprtsec_policy(exp, rqstp);
+	if (error)
+		goto out;
+
 	if ((access & NFSD_MAY_NLM) && (exp->ex_flags & NFSEXP_NOAUTHNLM))
 		/* NLM is allowed to fully bypass authentication */
 		goto out;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 98ab55ba3ced..1b66aff1d750 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -323,6 +323,9 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
 	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
 	if (err)
 		return err;
+	err = check_xprtsec_policy(exp, rqstp);
+	if (err)
+		goto out;
 	err = check_nfsd_access(exp, rqstp, false);
 	if (err)
 		goto out;
-- 
2.48.1

Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Scott Mayhew]

On Thu, 31 Jul 2025, Scott Mayhew wrote:

> A while back I had reported that an NFSv3 client could successfully
> mount using '-o xprtsec=none' an export that had been exported with
> 'xprtsec=tls:mtls'.  By "successfully" I mean that the mount command
> would succeed and the mount would show up in /proc/mounts.  Attempting
> to do anything futher with the mount would be met with NFS3ERR_ACCES.
> 
> This was fixed (albeit accidentally) by bb4f07f2409c ("nfsd: Fix
> NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT") and was
> subsequently re-broken by 0813c5f01249 ("nfsd: fix access checking for
> NLM under XPRTSEC policies").
> 
> Transport Layer Security isn't an RPC security flavor or pseudo-flavor,
> so they shouldn't be conflated when determining whether the access
> checks can be bypassed.
> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  fs/nfsd/export.c   | 60 ++++++++++++++++++++++++++++++++++++----------
>  fs/nfsd/export.h   |  1 +
>  fs/nfsd/nfs4proc.c |  6 ++++-
>  fs/nfsd/nfs4xdr.c  |  3 +++
>  fs/nfsd/nfsfh.c    |  8 +++++++
>  fs/nfsd/vfs.c      |  3 +++
>  6 files changed, 67 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index cadfc2bae60e..bc54b01bb516 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1082,19 +1082,27 @@ static struct svc_export *exp_find(struct cache_detail *cd,
>  }
>  
>  /**
> - * check_nfsd_access - check if access to export is allowed.
> + * check_xprtsec_policy - check if access to export is permitted by the
> + * 			  xprtsec policy
>   * @exp: svc_export that is being accessed.
>   * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> - * @may_bypass_gss: reduce strictness of authorization check
> + *
> + * This logic should not be combined with check_nfsd_access, as the rules
> + * for bypassing GSS are not the same as for bypassing the xprtsec policy
> + * check:
> + * 	- NFSv3 FSINFO and GETATTR can bypass the GSS for the root dentry,
> + * 	  but that doesn't mean they can bypass the xprtsec poolicy check

typo

> + * 	- NLM can bypass the GSS check on exports exported with the
> + * 	  NFSEXP_NOAUTHNLM flag
> + * 	- NLM can always bypass the xprtsec policy check since TLS isn't
> + * 	  implemented for the sidecar protocols
>   *
>   * Return values:
>   *   %nfs_ok if access is granted, or
> - *   %nfserr_wrongsec if access is denied
> + *   %nfserr_acces or %nfserr_wrongsec if access is denied
>   */
> -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> -			 bool may_bypass_gss)
> +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp)
>  {
> -	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
>  	struct svc_xprt *xprt;
>  
>  	/*
> @@ -1110,22 +1118,49 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) {
>  		if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_TLS) {
>  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
>  		    !test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_MTLS) {
>  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
>  		    test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
> -	if (!may_bypass_gss)
> -		goto denied;
>  
> -ok:
> +	return rqstp->rq_vers < 4 ? nfserr_acces : nfserr_wrongsec;
> +}
> +
> +/**
> + * check_nfsd_access - check if access to export is allowed.
> + * @exp: svc_export that is being accessed.
> + * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> + * @may_bypass_gss: reduce strictness of authorization check
> + *
> + * Return values:
> + *   %nfs_ok if access is granted, or
> + *   %nfserr_wrongsec if access is denied
> + */
> +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> +			 bool may_bypass_gss)
> +{
> +	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> +	struct svc_xprt *xprt;
> +
> +	/*
> +	 * If rqstp is NULL, this is a LOCALIO request which will only
> +	 * ever use a filehandle/credential pair for which access has
> +	 * been affirmed (by ACCESS or OPEN NFS requests) over the
> +	 * wire. So there is no need for further checks here.
> +	 */
> +	if (!rqstp)
> +		return nfs_ok;
> +
> +	xprt = rqstp->rq_xprt;
> +
>  	/* legacy gss-only clients are always OK: */
>  	if (exp->ex_client == rqstp->rq_gssclient)
>  		return nfs_ok;
> @@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  		}
>  	}
>  
> -denied:
>  	return nfserr_wrongsec;
>  }
>  
> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> index b9c0adb3ce09..c5a45f4b72be 100644
> --- a/fs/nfsd/export.h
> +++ b/fs/nfsd/export.h
> @@ -101,6 +101,7 @@ struct svc_expkey {
>  
>  struct svc_cred;
>  int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp);
> +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp);
>  __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  			 bool may_bypass_gss);
>  
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 71b428efcbb5..71e9a170f7bf 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2902,8 +2902,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  				clear_current_stateid(cstate);
>  
>  			if (current_fh->fh_export &&
> -					need_wrongsec_check(rqstp))
> +					need_wrongsec_check(rqstp)) {
> +				op->status = check_xprtsec_policy(current_fh->fh_export, rqstp);
> +				if (op->status)
> +					goto encode_op;
>  				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
> +			}
>  		}
>  encode_op:
>  		if (op->status == nfserr_replay_me) {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index ea91bad4eee2..48d55c13c918 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3859,6 +3859,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
>  			nfserr = nfserrno(err);
>  			goto out_put;
>  		}
> +		nfserr = check_xprtsec_policy(exp, cd->rd_rqstp);
> +		if (nfserr)
> +			goto out_put;
>  		nfserr = check_nfsd_access(exp, cd->rd_rqstp, false);
>  		if (nfserr)
>  			goto out_put;
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 74cf1f4de174..1ffc33662582 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -364,6 +364,14 @@ __fh_verify(struct svc_rqst *rqstp,
>  	if (error)
>  		goto out;
>  
> +	if (access & NFSD_MAY_NLM)
> +		/* NLM is allowed to bypass the xprtssec policy check */
> +		goto out;

This would automatically bypass the auth check too.  I'll send a v2.

> +
> +	error = check_xprtsec_policy(exp, rqstp);
> +	if (error)
> +		goto out;
> +
>  	if ((access & NFSD_MAY_NLM) && (exp->ex_flags & NFSEXP_NOAUTHNLM))
>  		/* NLM is allowed to fully bypass authentication */
>  		goto out;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 98ab55ba3ced..1b66aff1d750 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -323,6 +323,9 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
>  	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
>  	if (err)
>  		return err;
> +	err = check_xprtsec_policy(exp, rqstp);
> +	if (err)
> +		goto out;
>  	err = check_nfsd_access(exp, rqstp, false);
>  	if (err)
>  		goto out;
> -- 
> 2.48.1
> 
> 

Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[NeilBrown]

On Fri, 01 Aug 2025, Scott Mayhew wrote:
> A while back I had reported that an NFSv3 client could successfully
> mount using '-o xprtsec=none' an export that had been exported with
> 'xprtsec=tls:mtls'.  By "successfully" I mean that the mount command
> would succeed and the mount would show up in /proc/mounts.  Attempting
> to do anything futher with the mount would be met with NFS3ERR_ACCES.
> 
> This was fixed (albeit accidentally) by bb4f07f2409c ("nfsd: Fix
> NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT") and was
> subsequently re-broken by 0813c5f01249 ("nfsd: fix access checking for
> NLM under XPRTSEC policies").
> 
> Transport Layer Security isn't an RPC security flavor or pseudo-flavor,
> so they shouldn't be conflated when determining whether the access
> checks can be bypassed.

Clearly delineating the two makes a lot of sense - thanks for doing this.

> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  fs/nfsd/export.c   | 60 ++++++++++++++++++++++++++++++++++++----------
>  fs/nfsd/export.h   |  1 +
>  fs/nfsd/nfs4proc.c |  6 ++++-
>  fs/nfsd/nfs4xdr.c  |  3 +++
>  fs/nfsd/nfsfh.c    |  8 +++++++
>  fs/nfsd/vfs.c      |  3 +++
>  6 files changed, 67 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index cadfc2bae60e..bc54b01bb516 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1082,19 +1082,27 @@ static struct svc_export *exp_find(struct cache_detail *cd,
>  }
>  
>  /**
> - * check_nfsd_access - check if access to export is allowed.
> + * check_xprtsec_policy - check if access to export is permitted by the
> + * 			  xprtsec policy
>   * @exp: svc_export that is being accessed.
>   * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> - * @may_bypass_gss: reduce strictness of authorization check
> + *
> + * This logic should not be combined with check_nfsd_access, as the rules
> + * for bypassing GSS are not the same as for bypassing the xprtsec policy
> + * check:
> + * 	- NFSv3 FSINFO and GETATTR can bypass the GSS for the root dentry,
> + * 	  but that doesn't mean they can bypass the xprtsec poolicy check
> + * 	- NLM can bypass the GSS check on exports exported with the
> + * 	  NFSEXP_NOAUTHNLM flag
> + * 	- NLM can always bypass the xprtsec policy check since TLS isn't
> + * 	  implemented for the sidecar protocols

Despite this detailed difference, of the 4 times that
check_xprtsec_policy() and check_nfsd_access() are called, three times
they are simply called one after the other and the other time you got
the logic wrong :-) (as you subsequently noted).

So I wonder if, having pulled them apart, they could be recombined to
maintain the simplicity but add clarity.
It would be good if the code made it abundantly clear how and why that
fourth case (in __fh_verify) is different from the other three.

Looking forward to v2
Thanks,
NeilBrown

Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[kernel test robot]

Hi Scott,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brauner-vfs/vfs.all]
[also build test WARNING on linus/master v6.16 next-20250801]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Scott-Mayhew/nfsd-decouple-the-xprtsec-policy-check-from-check_nfsd_access/20250801-051621
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20250731211441.1908508-1-smayhew%40redhat.com
patch subject: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()
config: i386-buildonly-randconfig-003-20250801 (https://download.01.org/0day-ci/archive/20250801/202508011835.YXIfu0oy-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250801/202508011835.YXIfu0oy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508011835.YXIfu0oy-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/nfsd/export.c: In function 'exp_rootfh':
   fs/nfsd/export.c:1029:34: warning: variable 'inode' set but not used [-Wunused-but-set-variable]
    1029 |         struct inode            *inode;
         |                                  ^~~~~
   fs/nfsd/export.c: In function 'check_nfsd_access':
>> fs/nfsd/export.c:1153:26: warning: variable 'xprt' set but not used [-Wunused-but-set-variable]
    1153 |         struct svc_xprt *xprt;
         |                          ^~~~


vim +/xprt +1153 fs/nfsd/export.c

  1138	
  1139	/**
  1140	 * check_nfsd_access - check if access to export is allowed.
  1141	 * @exp: svc_export that is being accessed.
  1142	 * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
  1143	 * @may_bypass_gss: reduce strictness of authorization check
  1144	 *
  1145	 * Return values:
  1146	 *   %nfs_ok if access is granted, or
  1147	 *   %nfserr_wrongsec if access is denied
  1148	 */
  1149	__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
  1150				 bool may_bypass_gss)
  1151	{
  1152		struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> 1153		struct svc_xprt *xprt;
  1154	
  1155		/*
  1156		 * If rqstp is NULL, this is a LOCALIO request which will only
  1157		 * ever use a filehandle/credential pair for which access has
  1158		 * been affirmed (by ACCESS or OPEN NFS requests) over the
  1159		 * wire. So there is no need for further checks here.
  1160		 */
  1161		if (!rqstp)
  1162			return nfs_ok;
  1163	
  1164		xprt = rqstp->rq_xprt;
  1165	
  1166		/* legacy gss-only clients are always OK: */
  1167		if (exp->ex_client == rqstp->rq_gssclient)
  1168			return nfs_ok;
  1169		/* ip-address based client; check sec= export option: */
  1170		for (f = exp->ex_flavors; f < end; f++) {
  1171			if (f->pseudoflavor == rqstp->rq_cred.cr_flavor)
  1172				return nfs_ok;
  1173		}
  1174		/* defaults in absence of sec= options: */
  1175		if (exp->ex_nflavors == 0) {
  1176			if (rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL ||
  1177			    rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
  1178				return nfs_ok;
  1179		}
  1180	
  1181		/* If the compound op contains a spo_must_allowed op,
  1182		 * it will be sent with integrity/protection which
  1183		 * will have to be expressly allowed on mounts that
  1184		 * don't support it
  1185		 */
  1186	
  1187		if (nfsd4_spo_must_allow(rqstp))
  1188			return nfs_ok;
  1189	
  1190		/* Some calls may be processed without authentication
  1191		 * on GSS exports. For example NFS2/3 calls on root
  1192		 * directory, see section 2.3.2 of rfc 2623.
  1193		 * For "may_bypass_gss" check that export has really
  1194		 * enabled some flavor with authentication (GSS or any
  1195		 * other) and also check that the used auth flavor is
  1196		 * without authentication (none or sys).
  1197		 */
  1198		if (may_bypass_gss && (
  1199		     rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL ||
  1200		     rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)) {
  1201			for (f = exp->ex_flavors; f < end; f++) {
  1202				if (f->pseudoflavor >= RPC_AUTH_DES)
  1203					return 0;
  1204			}
  1205		}
  1206	
  1207		return nfserr_wrongsec;
  1208	}
  1209	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Jeff Layton]

On Thu, 2025-07-31 at 17:14 -0400, Scott Mayhew wrote:
> A while back I had reported that an NFSv3 client could successfully
> mount using '-o xprtsec=none' an export that had been exported with
> 'xprtsec=tls:mtls'.  By "successfully" I mean that the mount command
> would succeed and the mount would show up in /proc/mounts.  Attempting
> to do anything futher with the mount would be met with NFS3ERR_ACCES.
> 
> This was fixed (albeit accidentally) by bb4f07f2409c ("nfsd: Fix
> NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT") and was
> subsequently re-broken by 0813c5f01249 ("nfsd: fix access checking for
> NLM under XPRTSEC policies").
> 
> Transport Layer Security isn't an RPC security flavor or pseudo-flavor,
> so they shouldn't be conflated when determining whether the access
> checks can be bypassed.
> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  fs/nfsd/export.c   | 60 ++++++++++++++++++++++++++++++++++++----------
>  fs/nfsd/export.h   |  1 +
>  fs/nfsd/nfs4proc.c |  6 ++++-
>  fs/nfsd/nfs4xdr.c  |  3 +++
>  fs/nfsd/nfsfh.c    |  8 +++++++
>  fs/nfsd/vfs.c      |  3 +++
>  6 files changed, 67 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index cadfc2bae60e..bc54b01bb516 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1082,19 +1082,27 @@ static struct svc_export *exp_find(struct cache_detail *cd,
>  }
>  
>  /**
> - * check_nfsd_access - check if access to export is allowed.
> + * check_xprtsec_policy - check if access to export is permitted by the
> + * 			  xprtsec policy
>   * @exp: svc_export that is being accessed.
>   * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> - * @may_bypass_gss: reduce strictness of authorization check
> + *
> + * This logic should not be combined with check_nfsd_access, as the rules
> + * for bypassing GSS are not the same as for bypassing the xprtsec policy
> + * check:
> + * 	- NFSv3 FSINFO and GETATTR can bypass the GSS for the root dentry,
> + * 	  but that doesn't mean they can bypass the xprtsec poolicy check

nit: "policy"

> + * 	- NLM can bypass the GSS check on exports exported with the
> + * 	  NFSEXP_NOAUTHNLM flag
> + * 	- NLM can always bypass the xprtsec policy check since TLS isn't
> + * 	  implemented for the sidecar protocols
>   *
>   * Return values:
>   *   %nfs_ok if access is granted, or
> - *   %nfserr_wrongsec if access is denied
> + *   %nfserr_acces or %nfserr_wrongsec if access is denied
>   */
> -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> -			 bool may_bypass_gss)
> +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp)
>  {
> -	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
>  	struct svc_xprt *xprt;
>  
>  	/*
> @@ -1110,22 +1118,49 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) {
>  		if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_TLS) {
>  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
>  		    !test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_MTLS) {
>  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
>  		    test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
> -	if (!may_bypass_gss)
> -		goto denied;
>  
> -ok:
> +	return rqstp->rq_vers < 4 ? nfserr_acces : nfserr_wrongsec;
> +}
> +
> +/**
> + * check_nfsd_access - check if access to export is allowed.
> + * @exp: svc_export that is being accessed.
> + * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> + * @may_bypass_gss: reduce strictness of authorization check
> + *
> + * Return values:
> + *   %nfs_ok if access is granted, or
> + *   %nfserr_wrongsec if access is denied
> + */
> +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> +			 bool may_bypass_gss)
> +{
> +	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> +	struct svc_xprt *xprt;
> +
> +	/*
> +	 * If rqstp is NULL, this is a LOCALIO request which will only
> +	 * ever use a filehandle/credential pair for which access has
> +	 * been affirmed (by ACCESS or OPEN NFS requests) over the
> +	 * wire. So there is no need for further checks here.
> +	 */
> +	if (!rqstp)
> +		return nfs_ok;
> +
> +	xprt = rqstp->rq_xprt;
> +
>  	/* legacy gss-only clients are always OK: */
>  	if (exp->ex_client == rqstp->rq_gssclient)
>  		return nfs_ok;
> @@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  		}
>  	}
>  
> -denied:
>  	return nfserr_wrongsec;
>  }
>  
> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> index b9c0adb3ce09..c5a45f4b72be 100644
> --- a/fs/nfsd/export.h
> +++ b/fs/nfsd/export.h
> @@ -101,6 +101,7 @@ struct svc_expkey {
>  
>  struct svc_cred;
>  int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp);
> +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp);
>  __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  			 bool may_bypass_gss);
>  
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 71b428efcbb5..71e9a170f7bf 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2902,8 +2902,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  				clear_current_stateid(cstate);
>  
>  			if (current_fh->fh_export &&
> -					need_wrongsec_check(rqstp))
> +					need_wrongsec_check(rqstp)) {
> +				op->status = check_xprtsec_policy(current_fh->fh_export, rqstp);
> +				if (op->status)
> +					goto encode_op;
>  				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
> +			}
>  		}
>  encode_op:
>  		if (op->status == nfserr_replay_me) {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index ea91bad4eee2..48d55c13c918 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3859,6 +3859,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
>  			nfserr = nfserrno(err);
>  			goto out_put;
>  		}
> +		nfserr = check_xprtsec_policy(exp, cd->rd_rqstp);
> +		if (nfserr)
> +			goto out_put;
>  		nfserr = check_nfsd_access(exp, cd->rd_rqstp, false);
>  		if (nfserr)
>  			goto out_put;
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 74cf1f4de174..1ffc33662582 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -364,6 +364,14 @@ __fh_verify(struct svc_rqst *rqstp,
>  	if (error)
>  		goto out;
>  
> +	if (access & NFSD_MAY_NLM)
> +		/* NLM is allowed to bypass the xprtssec policy check */
> +		goto out;
> +
> +	error = check_xprtsec_policy(exp, rqstp);
> +	if (error)
> +		goto out;
> +
>  	if ((access & NFSD_MAY_NLM) && (exp->ex_flags & NFSEXP_NOAUTHNLM))
>  		/* NLM is allowed to fully bypass authentication */
>  		goto out;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 98ab55ba3ced..1b66aff1d750 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -323,6 +323,9 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
>  	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
>  	if (err)
>  		return err;
> +	err = check_xprtsec_policy(exp, rqstp);
> +	if (err)
> +		goto out;
>  	err = check_nfsd_access(exp, rqstp, false);
>  	if (err)
>  		goto out;

The rest looks fine to me (though I wouldn't object to adding a helper
that calls both functions, like Neil suggested).

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Chuck Lever]

On 7/31/25 5:14 PM, Scott Mayhew wrote:
> A while back I had reported that an NFSv3 client could successfully
> mount using '-o xprtsec=none' an export that had been exported with
> 'xprtsec=tls:mtls'.  By "successfully" I mean that the mount command
> would succeed and the mount would show up in /proc/mounts.  Attempting
> to do anything futher with the mount would be met with NFS3ERR_ACCES.

I agree, though it doesn't compromise access to file data, that's not
the most desirable behavior.

Can you find your report on lore, and a Link: to it here in the patch
description?


> This was fixed (albeit accidentally) by bb4f07f2409c ("nfsd: Fix
> NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT") and was
> subsequently re-broken by 0813c5f01249 ("nfsd: fix access checking for
> NLM under XPRTSEC policies").
> 
> Transport Layer Security isn't an RPC security flavor or pseudo-flavor,
> so they shouldn't be conflated when determining whether the access
> checks can be bypassed.

Fixes: 9280c5774314 ("NFSD: Handle new xprtsec= export option")


> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  fs/nfsd/export.c   | 60 ++++++++++++++++++++++++++++++++++++----------
>  fs/nfsd/export.h   |  1 +
>  fs/nfsd/nfs4proc.c |  6 ++++-
>  fs/nfsd/nfs4xdr.c  |  3 +++
>  fs/nfsd/nfsfh.c    |  8 +++++++
>  fs/nfsd/vfs.c      |  3 +++
>  6 files changed, 67 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index cadfc2bae60e..bc54b01bb516 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1082,19 +1082,27 @@ static struct svc_export *exp_find(struct cache_detail *cd,
>  }
>  
>  /**
> - * check_nfsd_access - check if access to export is allowed.
> + * check_xprtsec_policy - check if access to export is permitted by the
> + * 			  xprtsec policy
>   * @exp: svc_export that is being accessed.
>   * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> - * @may_bypass_gss: reduce strictness of authorization check
> + *
> + * This logic should not be combined with check_nfsd_access, as the rules
> + * for bypassing GSS are not the same as for bypassing the xprtsec policy
> + * check:
> + * 	- NFSv3 FSINFO and GETATTR can bypass the GSS for the root dentry,
> + * 	  but that doesn't mean they can bypass the xprtsec poolicy check
> + * 	- NLM can bypass the GSS check on exports exported with the
> + * 	  NFSEXP_NOAUTHNLM flag
> + * 	- NLM can always bypass the xprtsec policy check since TLS isn't
> + * 	  implemented for the sidecar protocols
>   *
>   * Return values:
>   *   %nfs_ok if access is granted, or
> - *   %nfserr_wrongsec if access is denied
> + *   %nfserr_acces or %nfserr_wrongsec if access is denied
>   */
> -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> -			 bool may_bypass_gss)
> +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp)
>  {
> -	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
>  	struct svc_xprt *xprt;
>  
>  	/*
> @@ -1110,22 +1118,49 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) {
>  		if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_TLS) {
>  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
>  		    !test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_MTLS) {
>  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
>  		    test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> -			goto ok;
> +			return nfs_ok;
>  	}
> -	if (!may_bypass_gss)
> -		goto denied;
>  
> -ok:
> +	return rqstp->rq_vers < 4 ? nfserr_acces : nfserr_wrongsec;

We recently spilled a lot of electrons trying to get these version
checks out of the generic security checking paths. For one thing, this
particular check is valid only for the NFS program.

Returning nfserr_wrongsec unconditionally, as check_nfsd_access now
does, should be sufficient.


> +}
> +
> +/**
> + * check_nfsd_access - check if access to export is allowed.
> + * @exp: svc_export that is being accessed.
> + * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> + * @may_bypass_gss: reduce strictness of authorization check
> + *
> + * Return values:
> + *   %nfs_ok if access is granted, or
> + *   %nfserr_wrongsec if access is denied
> + */
> +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> +			 bool may_bypass_gss)
> +{
> +	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> +	struct svc_xprt *xprt;
> +
> +	/*
> +	 * If rqstp is NULL, this is a LOCALIO request which will only
> +	 * ever use a filehandle/credential pair for which access has
> +	 * been affirmed (by ACCESS or OPEN NFS requests) over the
> +	 * wire. So there is no need for further checks here.
> +	 */
> +	if (!rqstp)
> +		return nfs_ok;

Is this true of all of check_nfsd_access's callers, or only of
__fh_verify ?


> +
> +	xprt = rqstp->rq_xprt;
> +
>  	/* legacy gss-only clients are always OK: */
>  	if (exp->ex_client == rqstp->rq_gssclient)
>  		return nfs_ok;
> @@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  		}
>  	}
>  
> -denied:
>  	return nfserr_wrongsec;
>  }
>  
> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> index b9c0adb3ce09..c5a45f4b72be 100644
> --- a/fs/nfsd/export.h
> +++ b/fs/nfsd/export.h
> @@ -101,6 +101,7 @@ struct svc_expkey {
>  
>  struct svc_cred;
>  int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp);
> +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp);
>  __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>  			 bool may_bypass_gss);
>  
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 71b428efcbb5..71e9a170f7bf 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2902,8 +2902,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  				clear_current_stateid(cstate);
>  
>  			if (current_fh->fh_export &&
> -					need_wrongsec_check(rqstp))
> +					need_wrongsec_check(rqstp)) {
> +				op->status = check_xprtsec_policy(current_fh->fh_export, rqstp);
> +				if (op->status)
> +					goto encode_op;
>  				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
> +			}
>  		}
>  encode_op:
>  		if (op->status == nfserr_replay_me) {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index ea91bad4eee2..48d55c13c918 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3859,6 +3859,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
>  			nfserr = nfserrno(err);
>  			goto out_put;
>  		}
> +		nfserr = check_xprtsec_policy(exp, cd->rd_rqstp);
> +		if (nfserr)
> +			goto out_put;
>  		nfserr = check_nfsd_access(exp, cd->rd_rqstp, false);
>  		if (nfserr)
>  			goto out_put;
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 74cf1f4de174..1ffc33662582 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -364,6 +364,14 @@ __fh_verify(struct svc_rqst *rqstp,
>  	if (error)
>  		goto out;
>  
> +	if (access & NFSD_MAY_NLM)
> +		/* NLM is allowed to bypass the xprtssec policy check */
		/* because lockd currently does not support xprtsec */> +		goto out;
Every check_xprtsec_policy / check_nfsd_access call site now has two
function calls, resulting in duplicate code.

Why not leave check_nfsd_access() in place, but replace it's guts with
two helpers, and then call the two helpers directly here in __fh_verify?


> +
> +	error = check_xprtsec_policy(exp, rqstp);
> +	if (error)
> +		goto out;
> +
>  	if ((access & NFSD_MAY_NLM) && (exp->ex_flags & NFSEXP_NOAUTHNLM))
>  		/* NLM is allowed to fully bypass authentication */
>  		goto out;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 98ab55ba3ced..1b66aff1d750 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -323,6 +323,9 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
>  	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
>  	if (err)
>  		return err;
> +	err = check_xprtsec_policy(exp, rqstp);
> +	if (err)
> +		goto out;
>  	err = check_nfsd_access(exp, rqstp, false);
>  	if (err)
>  		goto out;


-- 
Chuck Lever

Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Scott Mayhew]

On Fri, 01 Aug 2025, Chuck Lever wrote:

> On 7/31/25 5:14 PM, Scott Mayhew wrote:
> > A while back I had reported that an NFSv3 client could successfully
> > mount using '-o xprtsec=none' an export that had been exported with
> > 'xprtsec=tls:mtls'.  By "successfully" I mean that the mount command
> > would succeed and the mount would show up in /proc/mounts.  Attempting
> > to do anything futher with the mount would be met with NFS3ERR_ACCES.
> 
> I agree, though it doesn't compromise access to file data, that's not
> the most desirable behavior.
> 
> Can you find your report on lore, and a Link: to it here in the patch
> description?

Will do.
> 
> 
> > This was fixed (albeit accidentally) by bb4f07f2409c ("nfsd: Fix
> > NFSD_MAY_BYPASS_GSS and NFSD_MAY_BYPASS_GSS_ON_ROOT") and was
> > subsequently re-broken by 0813c5f01249 ("nfsd: fix access checking for
> > NLM under XPRTSEC policies").
> > 
> > Transport Layer Security isn't an RPC security flavor or pseudo-flavor,
> > so they shouldn't be conflated when determining whether the access
> > checks can be bypassed.
> 
> Fixes: 9280c5774314 ("NFSD: Handle new xprtsec= export option")
> 
> 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  fs/nfsd/export.c   | 60 ++++++++++++++++++++++++++++++++++++----------
> >  fs/nfsd/export.h   |  1 +
> >  fs/nfsd/nfs4proc.c |  6 ++++-
> >  fs/nfsd/nfs4xdr.c  |  3 +++
> >  fs/nfsd/nfsfh.c    |  8 +++++++
> >  fs/nfsd/vfs.c      |  3 +++
> >  6 files changed, 67 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index cadfc2bae60e..bc54b01bb516 100644
> > --- a/fs/nfsd/export.c
> > +++ b/fs/nfsd/export.c
> > @@ -1082,19 +1082,27 @@ static struct svc_export *exp_find(struct cache_detail *cd,
> >  }
> >  
> >  /**
> > - * check_nfsd_access - check if access to export is allowed.
> > + * check_xprtsec_policy - check if access to export is permitted by the
> > + * 			  xprtsec policy
> >   * @exp: svc_export that is being accessed.
> >   * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> > - * @may_bypass_gss: reduce strictness of authorization check
> > + *
> > + * This logic should not be combined with check_nfsd_access, as the rules
> > + * for bypassing GSS are not the same as for bypassing the xprtsec policy
> > + * check:
> > + * 	- NFSv3 FSINFO and GETATTR can bypass the GSS for the root dentry,
> > + * 	  but that doesn't mean they can bypass the xprtsec poolicy check
> > + * 	- NLM can bypass the GSS check on exports exported with the
> > + * 	  NFSEXP_NOAUTHNLM flag
> > + * 	- NLM can always bypass the xprtsec policy check since TLS isn't
> > + * 	  implemented for the sidecar protocols
> >   *
> >   * Return values:
> >   *   %nfs_ok if access is granted, or
> > - *   %nfserr_wrongsec if access is denied
> > + *   %nfserr_acces or %nfserr_wrongsec if access is denied
> >   */
> > -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> > -			 bool may_bypass_gss)
> > +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp)
> >  {
> > -	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> >  	struct svc_xprt *xprt;
> >  
> >  	/*
> > @@ -1110,22 +1118,49 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >  
> >  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) {
> >  		if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags))
> > -			goto ok;
> > +			return nfs_ok;
> >  	}
> >  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_TLS) {
> >  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
> >  		    !test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> > -			goto ok;
> > +			return nfs_ok;
> >  	}
> >  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_MTLS) {
> >  		if (test_bit(XPT_TLS_SESSION, &xprt->xpt_flags) &&
> >  		    test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> > -			goto ok;
> > +			return nfs_ok;
> >  	}
> > -	if (!may_bypass_gss)
> > -		goto denied;
> >  
> > -ok:
> > +	return rqstp->rq_vers < 4 ? nfserr_acces : nfserr_wrongsec;
> 
> We recently spilled a lot of electrons trying to get these version
> checks out of the generic security checking paths. For one thing, this
> particular check is valid only for the NFS program.
> 
> Returning nfserr_wrongsec unconditionally, as check_nfsd_access now
> does, should be sufficient.

Okay.
> 
> 
> > +}
> > +
> > +/**
> > + * check_nfsd_access - check if access to export is allowed.
> > + * @exp: svc_export that is being accessed.
> > + * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> > + * @may_bypass_gss: reduce strictness of authorization check
> > + *
> > + * Return values:
> > + *   %nfs_ok if access is granted, or
> > + *   %nfserr_wrongsec if access is denied
> > + */
> > +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> > +			 bool may_bypass_gss)
> > +{
> > +	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> > +	struct svc_xprt *xprt;
> > +
> > +	/*
> > +	 * If rqstp is NULL, this is a LOCALIO request which will only
> > +	 * ever use a filehandle/credential pair for which access has
> > +	 * been affirmed (by ACCESS or OPEN NFS requests) over the
> > +	 * wire. So there is no need for further checks here.
> > +	 */
> > +	if (!rqstp)
> > +		return nfs_ok;
> 
> Is this true of all of check_nfsd_access's callers, or only of
> __fh_verify ?
> 
Looking at the commit where this check was added, and looking at the
other callers, it looks like this is only true of __fh_verify().

I'm splitting up check_nfsd_access() into two helpers has you suggested,
having __fh_verify() call the helpers directly while having the other
callers continue to use check_nfsd_access().

Should I add an argument to the helpers indicate when they have been
called directly?  Something like 'bool maybe_localio', which can
then be incorporated into the above check, e.g.

        if (!rqstp) {
                if (maybe_localio) {
                        return nfs_ok;
                } else {
                        WARN_ON_ONCE(1);
                        return nfserr_wrongsec;
                }
        }



> 
> > +
> > +	xprt = rqstp->rq_xprt;
> > +
> >  	/* legacy gss-only clients are always OK: */
> >  	if (exp->ex_client == rqstp->rq_gssclient)
> >  		return nfs_ok;
> > @@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >  		}
> >  	}
> >  
> > -denied:
> >  	return nfserr_wrongsec;
> >  }
> >  
> > diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> > index b9c0adb3ce09..c5a45f4b72be 100644
> > --- a/fs/nfsd/export.h
> > +++ b/fs/nfsd/export.h
> > @@ -101,6 +101,7 @@ struct svc_expkey {
> >  
> >  struct svc_cred;
> >  int nfsexp_flags(struct svc_cred *cred, struct svc_export *exp);
> > +__be32 check_xprtsec_policy(struct svc_export *exp, struct svc_rqst *rqstp);
> >  __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >  			 bool may_bypass_gss);
> >  
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 71b428efcbb5..71e9a170f7bf 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -2902,8 +2902,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
> >  				clear_current_stateid(cstate);
> >  
> >  			if (current_fh->fh_export &&
> > -					need_wrongsec_check(rqstp))
> > +					need_wrongsec_check(rqstp)) {
> > +				op->status = check_xprtsec_policy(current_fh->fh_export, rqstp);
> > +				if (op->status)
> > +					goto encode_op;
> >  				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
> > +			}
> >  		}
> >  encode_op:
> >  		if (op->status == nfserr_replay_me) {
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index ea91bad4eee2..48d55c13c918 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -3859,6 +3859,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
> >  			nfserr = nfserrno(err);
> >  			goto out_put;
> >  		}
> > +		nfserr = check_xprtsec_policy(exp, cd->rd_rqstp);
> > +		if (nfserr)
> > +			goto out_put;
> >  		nfserr = check_nfsd_access(exp, cd->rd_rqstp, false);
> >  		if (nfserr)
> >  			goto out_put;
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 74cf1f4de174..1ffc33662582 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -364,6 +364,14 @@ __fh_verify(struct svc_rqst *rqstp,
> >  	if (error)
> >  		goto out;
> >  
> > +	if (access & NFSD_MAY_NLM)
> > +		/* NLM is allowed to bypass the xprtssec policy check */
> 		/* because lockd currently does not support xprtsec */> +		goto out;
> Every check_xprtsec_policy / check_nfsd_access call site now has two
> function calls, resulting in duplicate code.
> 
> Why not leave check_nfsd_access() in place, but replace it's guts with
> two helpers, and then call the two helpers directly here in __fh_verify?

I'll create the two helpers as you suggest.

I'll still need to check the access flags for NFSD_MAY_NLM before
calling the xprtsec helper though (I'll make sure I do it in the right
place this time).

-Scott
> 
> 
> > +
> > +	error = check_xprtsec_policy(exp, rqstp);
> > +	if (error)
> > +		goto out;
> > +
> >  	if ((access & NFSD_MAY_NLM) && (exp->ex_flags & NFSEXP_NOAUTHNLM))
> >  		/* NLM is allowed to fully bypass authentication */
> >  		goto out;
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 98ab55ba3ced..1b66aff1d750 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -323,6 +323,9 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
> >  	err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry);
> >  	if (err)
> >  		return err;
> > +	err = check_xprtsec_policy(exp, rqstp);
> > +	if (err)
> > +		goto out;
> >  	err = check_nfsd_access(exp, rqstp, false);
> >  	if (err)
> >  		goto out;
> 
> 
> -- 
> Chuck Lever
> 

Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Chuck Lever]

On 8/5/25 10:32 AM, Scott Mayhew wrote:
> On Fri, 01 Aug 2025, Chuck Lever wrote:
> 
>> On 7/31/25 5:14 PM, Scott Mayhew wrote:

>>> +}
>>> +
>>> +/**
>>> + * check_nfsd_access - check if access to export is allowed.
>>> + * @exp: svc_export that is being accessed.
>>> + * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
>>> + * @may_bypass_gss: reduce strictness of authorization check
>>> + *
>>> + * Return values:
>>> + *   %nfs_ok if access is granted, or
>>> + *   %nfserr_wrongsec if access is denied
>>> + */
>>> +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>>> +			 bool may_bypass_gss)
>>> +{
>>> +	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
>>> +	struct svc_xprt *xprt;
>>> +
>>> +	/*
>>> +	 * If rqstp is NULL, this is a LOCALIO request which will only
>>> +	 * ever use a filehandle/credential pair for which access has
>>> +	 * been affirmed (by ACCESS or OPEN NFS requests) over the
>>> +	 * wire. So there is no need for further checks here.
>>> +	 */
>>> +	if (!rqstp)
>>> +		return nfs_ok;
>>
>> Is this true of all of check_nfsd_access's callers, or only of
>> __fh_verify ?
>>
> Looking at the commit where this check was added, and looking at the
> other callers, it looks like this is only true of __fh_verify().
> 
> I'm splitting up check_nfsd_access() into two helpers has you suggested,
> having __fh_verify() call the helpers directly while having the other
> callers continue to use check_nfsd_access().
> 
> Should I add an argument to the helpers indicate when they have been
> called directly?  Something like 'bool maybe_localio', which can
> then be incorporated into the above check, e.g.
> 
>         if (!rqstp) {
>                 if (maybe_localio) {
>                         return nfs_ok;
>                 } else {
>                         WARN_ON_ONCE(1);
>                         return nfserr_wrongsec;
>                 }
>         }

If __fh_verify is the only call site that can invoke these helpers with
rqstp == NULL, then __fh_verify seems like the place to do this check,
not in the helpers. But maybe I've misunderstood something?


>>> +
>>> +	xprt = rqstp->rq_xprt;
>>> +
>>>  	/* legacy gss-only clients are always OK: */
>>>  	if (exp->ex_client == rqstp->rq_gssclient)
>>>  		return nfs_ok;
>>> @@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
>>>  		}
>>>  	}
>>>  
>>> -denied:
>>>  	return nfserr_wrongsec;
>>>  }
>>>  


-- 
Chuck Lever

Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Scott Mayhew]

On Tue, 05 Aug 2025, Chuck Lever wrote:

> On 8/5/25 10:32 AM, Scott Mayhew wrote:
> > On Fri, 01 Aug 2025, Chuck Lever wrote:
> > 
> >> On 7/31/25 5:14 PM, Scott Mayhew wrote:
> 
> >>> +}
> >>> +
> >>> +/**
> >>> + * check_nfsd_access - check if access to export is allowed.
> >>> + * @exp: svc_export that is being accessed.
> >>> + * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO).
> >>> + * @may_bypass_gss: reduce strictness of authorization check
> >>> + *
> >>> + * Return values:
> >>> + *   %nfs_ok if access is granted, or
> >>> + *   %nfserr_wrongsec if access is denied
> >>> + */
> >>> +__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >>> +			 bool may_bypass_gss)
> >>> +{
> >>> +	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> >>> +	struct svc_xprt *xprt;
> >>> +
> >>> +	/*
> >>> +	 * If rqstp is NULL, this is a LOCALIO request which will only
> >>> +	 * ever use a filehandle/credential pair for which access has
> >>> +	 * been affirmed (by ACCESS or OPEN NFS requests) over the
> >>> +	 * wire. So there is no need for further checks here.
> >>> +	 */
> >>> +	if (!rqstp)
> >>> +		return nfs_ok;
> >>
> >> Is this true of all of check_nfsd_access's callers, or only of
> >> __fh_verify ?
> >>
> > Looking at the commit where this check was added, and looking at the
> > other callers, it looks like this is only true of __fh_verify().
> > 
> > I'm splitting up check_nfsd_access() into two helpers has you suggested,
> > having __fh_verify() call the helpers directly while having the other
> > callers continue to use check_nfsd_access().
> > 
> > Should I add an argument to the helpers indicate when they have been
> > called directly?  Something like 'bool maybe_localio', which can
> > then be incorporated into the above check, e.g.
> > 
> >         if (!rqstp) {
> >                 if (maybe_localio) {
> >                         return nfs_ok;
> >                 } else {
> >                         WARN_ON_ONCE(1);
> >                         return nfserr_wrongsec;
> >                 }
> >         }
> 
> If __fh_verify is the only call site that can invoke these helpers with
> rqstp == NULL, then __fh_verify seems like the place to do this check,
> not in the helpers. But maybe I've misunderstood something?

No, that makes sense.  Thanks.
> 
> 
> >>> +
> >>> +	xprt = rqstp->rq_xprt;
> >>> +
> >>>  	/* legacy gss-only clients are always OK: */
> >>>  	if (exp->ex_client == rqstp->rq_gssclient)
> >>>  		return nfs_ok;
> >>> @@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >>>  		}
> >>>  	}
> >>>  
> >>> -denied:
> >>>  	return nfserr_wrongsec;
> >>>  }
> >>>  
> 
> 
> -- 
> Chuck Lever
>