[RFC PATCH 0/6] Split up refactoring of fh_verify()

[RFC PATCH 0/6] Split up refactoring of fh_verify()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

These six patches are meant to replace:

  nfsd: factor out __fh_verify to allow NULL rqstp to be passed
  nfsd: add nfsd_file_acquire_local()

I've removed the @nfs_vers parameter to __fh_verify(), and tried
something a little different with the trace points. Please check
my math.

These have been compile-tested, but no more than that. Interested in
comments.

Chuck Lever (2):
  NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()
  NFSD: Short-circuit fh_verify tracepoints for LOCALIO

NeilBrown (4):
  NFSD: Handle @rqstp == NULL in check_nfsd_access()
  NFSD: Refactor nfsd_setuser_and_check_port()
  nfsd: factor out __fh_verify to allow NULL rqstp to be passed
  nfsd: add nfsd_file_acquire_local()

 fs/nfsd/export.c    |  29 ++++--
 fs/nfsd/filecache.c |  61 +++++++++++--
 fs/nfsd/filecache.h |   3 +
 fs/nfsd/lockd.c     |   6 +-
 fs/nfsd/nfsfh.c     | 210 ++++++++++++++++++++++++++------------------
 fs/nfsd/nfsfh.h     |   5 ++
 fs/nfsd/trace.h     |  18 ++--
 7 files changed, 223 insertions(+), 109 deletions(-)

-- 
2.45.2

[RFC PATCH 1/6] NFSD: Handle @rqstp == NULL in check_nfsd_access()

[cel]

From: NeilBrown <neilb@suse.de>

LOCALIO-initiated open operations are not running in an nfsd thread
and thus do not have an associated svc_rqst context.

Signed-off-by: NeilBrown <neilb@suse.de>
Co-developed-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/export.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 7bb4f2075ac5..46a4d989c850 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1074,10 +1074,29 @@ static struct svc_export *exp_find(struct cache_detail *cd,
 	return exp;
 }
 
+/**
+ * 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).
+ *
+ * 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)
 {
 	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
-	struct svc_xprt *xprt = rqstp->rq_xprt;
+	struct svc_xprt *xprt;
+
+	/*
+	 * The target use case for rqstp being NULL is LOCALIO, which
+	 * currently only supports AUTH_UNIX. The behavior for LOCALIO
+	 * is therefore the same as the AUTH_UNIX check below.
+	 */
+	if (!rqstp)
+		return nfs_ok;
+
+	xprt = rqstp->rq_xprt;
 
 	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) {
 		if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags))
@@ -1098,17 +1117,17 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
 ok:
 	/* legacy gss-only clients are always OK: */
 	if (exp->ex_client == rqstp->rq_gssclient)
-		return 0;
+		return nfs_ok;
 	/* ip-address based client; check sec= export option: */
 	for (f = exp->ex_flavors; f < end; f++) {
 		if (f->pseudoflavor == rqstp->rq_cred.cr_flavor)
-			return 0;
+			return nfs_ok;
 	}
 	/* defaults in absence of sec= options: */
 	if (exp->ex_nflavors == 0) {
 		if (rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL ||
 		    rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
-			return 0;
+			return nfs_ok;
 	}
 
 	/* If the compound op contains a spo_must_allowed op,
@@ -1118,7 +1137,7 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
 	 */
 
 	if (nfsd4_spo_must_allow(rqstp))
-		return 0;
+		return nfs_ok;
 
 denied:
 	return nfserr_wrongsec;
-- 
2.45.2

Re: [RFC PATCH 1/6] NFSD: Handle @rqstp == NULL in check_nfsd_access()

[NeilBrown]

On Wed, 28 Aug 2024, cel@kernel.org wrote:
> From: NeilBrown <neilb@suse.de>
> 
> LOCALIO-initiated open operations are not running in an nfsd thread
> and thus do not have an associated svc_rqst context.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> Co-developed-by: Mike Snitzer <snitzer@kernel.org>
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/export.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 7bb4f2075ac5..46a4d989c850 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1074,10 +1074,29 @@ static struct svc_export *exp_find(struct cache_detail *cd,
>  	return exp;
>  }
>  
> +/**
> + * 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).
> + *
> + * 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)
>  {
>  	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> -	struct svc_xprt *xprt = rqstp->rq_xprt;
> +	struct svc_xprt *xprt;
> +
> +	/*
> +	 * The target use case for rqstp being NULL is LOCALIO, which
> +	 * currently only supports AUTH_UNIX. The behavior for LOCALIO
> +	 * is therefore the same as the AUTH_UNIX check below.

The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0.
To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need
to confirm that 
  exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE
and either
  exp->ex_nflavours == 0
or
  one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX

I'm not sure that is all really necessary, but if not then we probably
need a better comment...

NeilBrown


> +	 */
> +	if (!rqstp)
> +		return nfs_ok;
> +
> +	xprt = rqstp->rq_xprt;
>  
>  	if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) {
>  		if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags))
> @@ -1098,17 +1117,17 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
>  ok:
>  	/* legacy gss-only clients are always OK: */
>  	if (exp->ex_client == rqstp->rq_gssclient)
> -		return 0;
> +		return nfs_ok;
>  	/* ip-address based client; check sec= export option: */
>  	for (f = exp->ex_flavors; f < end; f++) {
>  		if (f->pseudoflavor == rqstp->rq_cred.cr_flavor)
> -			return 0;
> +			return nfs_ok;
>  	}
>  	/* defaults in absence of sec= options: */
>  	if (exp->ex_nflavors == 0) {
>  		if (rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL ||
>  		    rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
> -			return 0;
> +			return nfs_ok;
>  	}
>  
>  	/* If the compound op contains a spo_must_allowed op,
> @@ -1118,7 +1137,7 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
>  	 */
>  
>  	if (nfsd4_spo_must_allow(rqstp))
> -		return 0;
> +		return nfs_ok;
>  
>  denied:
>  	return nfserr_wrongsec;
> -- 
> 2.45.2
> 
> 

Re: [RFC PATCH 1/6] NFSD: Handle @rqstp == NULL in check_nfsd_access()

[Mike Snitzer]

On Wed, Aug 28, 2024 at 11:12:00AM +1000, NeilBrown wrote:
> On Wed, 28 Aug 2024, cel@kernel.org wrote:
> > From: NeilBrown <neilb@suse.de>
> > 
> > LOCALIO-initiated open operations are not running in an nfsd thread
> > and thus do not have an associated svc_rqst context.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > Co-developed-by: Mike Snitzer <snitzer@kernel.org>
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/export.c | 29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index 7bb4f2075ac5..46a4d989c850 100644
> > --- a/fs/nfsd/export.c
> > +++ b/fs/nfsd/export.c
> > @@ -1074,10 +1074,29 @@ static struct svc_export *exp_find(struct cache_detail *cd,
> >  	return exp;
> >  }
> >  
> > +/**
> > + * 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).
> > + *
> > + * 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)
> >  {
> >  	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> > -	struct svc_xprt *xprt = rqstp->rq_xprt;
> > +	struct svc_xprt *xprt;
> > +
> > +	/*
> > +	 * The target use case for rqstp being NULL is LOCALIO, which
> > +	 * currently only supports AUTH_UNIX. The behavior for LOCALIO
> > +	 * is therefore the same as the AUTH_UNIX check below.
> 
> The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0.
> To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need
> to confirm that 
>   exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE
> and either
>   exp->ex_nflavours == 0
> or
>   one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX
> 
> I'm not sure that is all really necessary, but if not then we probably
> need a better comment...

Think extra checks aren't needed (unless you think a NULL rqstp
_without_ the use of LOCALIO possible?  which could trigger a false
positive granting of access? seems unlikely but...)

Mike

Re: [RFC PATCH 1/6] NFSD: Handle @rqstp == NULL in check_nfsd_access()

[NeilBrown]

On Wed, 28 Aug 2024, Mike Snitzer wrote:
> On Wed, Aug 28, 2024 at 11:12:00AM +1000, NeilBrown wrote:
> > On Wed, 28 Aug 2024, cel@kernel.org wrote:
> > > From: NeilBrown <neilb@suse.de>
> > > 
> > > LOCALIO-initiated open operations are not running in an nfsd thread
> > > and thus do not have an associated svc_rqst context.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > Co-developed-by: Mike Snitzer <snitzer@kernel.org>
> > > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > >  fs/nfsd/export.c | 29 ++++++++++++++++++++++++-----
> > >  1 file changed, 24 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > > index 7bb4f2075ac5..46a4d989c850 100644
> > > --- a/fs/nfsd/export.c
> > > +++ b/fs/nfsd/export.c
> > > @@ -1074,10 +1074,29 @@ static struct svc_export *exp_find(struct cache_detail *cd,
> > >  	return exp;
> > >  }
> > >  
> > > +/**
> > > + * 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).
> > > + *
> > > + * 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)
> > >  {
> > >  	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> > > -	struct svc_xprt *xprt = rqstp->rq_xprt;
> > > +	struct svc_xprt *xprt;
> > > +
> > > +	/*
> > > +	 * The target use case for rqstp being NULL is LOCALIO, which
> > > +	 * currently only supports AUTH_UNIX. The behavior for LOCALIO
> > > +	 * is therefore the same as the AUTH_UNIX check below.
> > 
> > The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0.
> > To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need
> > to confirm that 
> >   exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE
> > and either
> >   exp->ex_nflavours == 0
> > or
> >   one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX
> > 
> > I'm not sure that is all really necessary, but if not then we probably
> > need a better comment...
> 
> Think extra checks aren't needed (unless you think a NULL rqstp
> _without_ the use of LOCALIO possible?  which could trigger a false
> positive granting of access? seems unlikely but...)
> 

I agree they aren't needed.  I think we need to have a clear
understanding of why that aren't needed, and to write that understanding
down.  So that if some day someone wants to change this code, they can
understand the consequences.

Maybe the answer is that LOCALIO would never ask for access that isn't
allowed, so there is no need to check.

Maybe the client can determine the relevant xpt_flags from the client
end of the session, so it can pass them reliably to check_nfsd_access().

I don't know what is best, but I think we should have a comment
justifying the short-circuit, and I don't think the current proposed
comment does that correctly.

Thanks,
NeilBrown

Re: [RFC PATCH 1/6] NFSD: Handle @rqstp == NULL in check_nfsd_access()

[Chuck Lever III]

> On Aug 28, 2024, at 2:30 AM, NeilBrown <neilb@suse.de> wrote:
> 
> On Wed, 28 Aug 2024, Mike Snitzer wrote:
>> On Wed, Aug 28, 2024 at 11:12:00AM +1000, NeilBrown wrote:
>>> 
>>> The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0.
>>> To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need
>>> to confirm that 
>>>  exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE
>>> and either
>>>  exp->ex_nflavours == 0
>>> or
>>>  one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX
>>> 
>>> I'm not sure that is all really necessary, but if not then we probably
>>> need a better comment...
>> 
>> Think extra checks aren't needed (unless you think a NULL rqstp
>> _without_ the use of LOCALIO possible?  which could trigger a false
>> positive granting of access? seems unlikely but...)
>> 
> 
> I agree they aren't needed.  I think we need to have a clear
> understanding of why that aren't needed, and to write that understanding
> down.  So that if some day someone wants to change this code, they can
> understand the consequences.

> I don't know what is best, but I think we should have a comment
> justifying the short-circuit, and I don't think the current proposed
> comment does that correctly.

My goal is to document that understanding here, as you stated.
I will leave it to you and Mike to adjust the wording to your
liking.


--
Chuck Lever

Re: [RFC PATCH 1/6] NFSD: Handle @rqstp == NULL in check_nfsd_access()

[Mike Snitzer]

On Wed, Aug 28, 2024 at 04:30:05PM +1000, NeilBrown wrote:
> On Wed, 28 Aug 2024, Mike Snitzer wrote:
> > On Wed, Aug 28, 2024 at 11:12:00AM +1000, NeilBrown wrote:
> > > On Wed, 28 Aug 2024, cel@kernel.org wrote:
> > > > From: NeilBrown <neilb@suse.de>
> > > > 
> > > > LOCALIO-initiated open operations are not running in an nfsd thread
> > > > and thus do not have an associated svc_rqst context.
> > > > 
> > > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > > Co-developed-by: Mike Snitzer <snitzer@kernel.org>
> > > > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > ---
> > > >  fs/nfsd/export.c | 29 ++++++++++++++++++++++++-----
> > > >  1 file changed, 24 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > > > index 7bb4f2075ac5..46a4d989c850 100644
> > > > --- a/fs/nfsd/export.c
> > > > +++ b/fs/nfsd/export.c
> > > > @@ -1074,10 +1074,29 @@ static struct svc_export *exp_find(struct cache_detail *cd,
> > > >  	return exp;
> > > >  }
> > > >  
> > > > +/**
> > > > + * 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).
> > > > + *
> > > > + * 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)
> > > >  {
> > > >  	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> > > > -	struct svc_xprt *xprt = rqstp->rq_xprt;
> > > > +	struct svc_xprt *xprt;
> > > > +
> > > > +	/*
> > > > +	 * The target use case for rqstp being NULL is LOCALIO, which
> > > > +	 * currently only supports AUTH_UNIX. The behavior for LOCALIO
> > > > +	 * is therefore the same as the AUTH_UNIX check below.
> > > 
> > > The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0.
> > > To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need
> > > to confirm that 
> > >   exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE
> > > and either
> > >   exp->ex_nflavours == 0
> > > or
> > >   one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX
> > > 
> > > I'm not sure that is all really necessary, but if not then we probably
> > > need a better comment...
> > 
> > Think extra checks aren't needed (unless you think a NULL rqstp
> > _without_ the use of LOCALIO possible?  which could trigger a false
> > positive granting of access? seems unlikely but...)
> > 
> 
> I agree they aren't needed.  I think we need to have a clear
> understanding of why that aren't needed, and to write that understanding
> down.  So that if some day someone wants to change this code, they can
> understand the consequences.
> 
> Maybe the answer is that LOCALIO would never ask for access that isn't
> allowed, so there is no need to check.
> 
> Maybe the client can determine the relevant xpt_flags from the client
> end of the session, so it can pass them reliably to check_nfsd_access().
> 
> I don't know what is best, but I think we should have a comment
> justifying the short-circuit, and I don't think the current proposed
> comment does that correctly.

Just to recap, this is what you had originally, which Chuck correctly
said needed improvement:

 __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
 {
        struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
        struct svc_xprt *xprt;

        if (!rqstp)
                /* Always allow LOCALIO */
                return 0;

I offered my suggestion and Chuck then tweaked it:

 __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
 {
        struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
        struct svc_xprt *xprt;

-       if (!rqstp) {
-               /*
-                * The target use case for rqstp being NULL is LOCALIO,
-                * which only supports AUTH_UNIX, so always allow LOCALIO
-                * because the other checks below aren't applicable.
-                */
-               return 0;
-       }
+       /*
+        * The target use case for rqstp being NULL is LOCALIO, which
+        * currently only supports AUTH_UNIX. The behavior for LOCALIO
+        * is therefore the same as the AUTH_UNIX check below.
+        */
+       if (!rqstp)
+               return nfs_ok;

Now you're saying that comment needs to be more precise... ;)

localio only supports AUTH_UNIX, and the client verifies that is the
method being used:

void nfs_local_probe(struct nfs_client *clp)
{
        nfs_uuid_t nfs_uuid;

        /* Disallow localio if disabled via sysfs or AUTH_SYS isn't used */
        if (!localio_enabled ||
            clp->cl_rpcclient->cl_auth->au_flavor != RPC_AUTH_UNIX) {
                nfs_local_disable(clp);
                return;
        }
	...

So I honestly feel like Chuck's latest revision is perfectly fine.
I disagree that "The behavior for LOCALIO is therefore the same as
the AUTH_UNIX check below." is inaccurate.  The precondition from the
client (used to establish localio and cause rqstp to be NULL in
check_nfsd_access) is effectively comparable no?

Re: [RFC PATCH 1/6] NFSD: Handle @rqstp == NULL in check_nfsd_access()

[NeilBrown]

On Wed, 28 Aug 2024, Mike Snitzer wrote:
> 
> So I honestly feel like Chuck's latest revision is perfectly fine.
> I disagree that "The behavior for LOCALIO is therefore the same as
> the AUTH_UNIX check below." is inaccurate.  The precondition from the
> client (used to establish localio and cause rqstp to be NULL in
> check_nfsd_access) is effectively comparable no?
> 

I don't think the correctness of the code is at all related to
AUTH_UNIX.
Suppose we did add support for krb5 somehow - would we really need a
different test?  I think not.

I think that the reason the code is correct and safe is that we trust
the client.  We *know* the client will only present an filehandle which
it has received over the wire and which it verified - with the
accompanying credential - it was allowed to access.

Maybe that is what you meant by "The precondition from the client".  I
agree that does give us sufficient safety.  I don't think AUTH_UNIX is
relevant.

/*
 * If rqstp is NULL, this is a LOCALIO request which will only ever use
 * 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.
 */


(It wasn't quite this clear to me when I wrote previously - but I always
 seems to think more clearly in the mornings!)

Thanks,
NeilBrown

Re: [RFC PATCH 1/6] NFSD: Handle @rqstp == NULL in check_nfsd_access()

[Mike Snitzer]

On Thu, Aug 29, 2024 at 07:05:45AM +1000, NeilBrown wrote:
> On Wed, 28 Aug 2024, Mike Snitzer wrote:
> > 
> > So I honestly feel like Chuck's latest revision is perfectly fine.
> > I disagree that "The behavior for LOCALIO is therefore the same as
> > the AUTH_UNIX check below." is inaccurate.  The precondition from the
> > client (used to establish localio and cause rqstp to be NULL in
> > check_nfsd_access) is effectively comparable no?
> > 
> 
> I don't think the correctness of the code is at all related to
> AUTH_UNIX.
> Suppose we did add support for krb5 somehow - would we really need a
> different test?  I think not.
> 
> I think that the reason the code is correct and safe is that we trust
> the client.  We *know* the client will only present an filehandle which
> it has received over the wire and which it verified - with the
> accompanying credential - it was allowed to access.
> 
> Maybe that is what you meant by "The precondition from the client".  I
> agree that does give us sufficient safety.  I don't think AUTH_UNIX is
> relevant.
> 
> /*
>  * If rqstp is NULL, this is a LOCALIO request which will only ever use
>  * 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.
>  */

Makes sense, and thanks!

> (It wasn't quite this clear to me when I wrote previously - but I always
>  seems to think more clearly in the mornings!)

I haven't been sleeping enough.. tonight, tonight I will!!! ;)

Mike

[RFC PATCH 2/6] NFSD: Refactor nfsd_setuser_and_check_port()

[cel]

From: NeilBrown <neilb@suse.de>

There are several places where __fh_verify unconditionally dereferences
rqstp to check that the connection is suitably secure.  They look at
rqstp->rq_xprt which is not meaningful in the target use case of
"localio" NFS in which the client talks directly to the local server.

Prepare these to always succeed when rqstp is NULL.

Signed-off-by: NeilBrown <neilb@suse.de>
Co-developed-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfsfh.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 50d23d56f403..4b964a71a504 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -87,23 +87,24 @@ nfsd_mode_check(struct dentry *dentry, umode_t requested)
 	return nfserr_wrong_type;
 }
 
-static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, int flags)
+static bool nfsd_originating_port_ok(struct svc_rqst *rqstp,
+				     struct svc_cred *cred,
+				     struct svc_export *exp)
 {
-	if (flags & NFSEXP_INSECURE_PORT)
+	if (nfsexp_flags(cred, exp) & NFSEXP_INSECURE_PORT)
 		return true;
 	/* We don't require gss requests to use low ports: */
-	if (rqstp->rq_cred.cr_flavor >= RPC_AUTH_GSS)
+	if (cred->cr_flavor >= RPC_AUTH_GSS)
 		return true;
 	return test_bit(RQ_SECURE, &rqstp->rq_flags);
 }
 
 static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
+					  struct svc_cred *cred,
 					  struct svc_export *exp)
 {
-	int flags = nfsexp_flags(&rqstp->rq_cred, exp);
-
 	/* Check if the request originated from a secure port. */
-	if (!nfsd_originating_port_ok(rqstp, flags)) {
+	if (rqstp && !nfsd_originating_port_ok(rqstp, cred, exp)) {
 		RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
 		dprintk("nfsd: request from insecure port %s!\n",
 		        svc_print_addr(rqstp, buf, sizeof(buf)));
@@ -111,7 +112,7 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
 	}
 
 	/* Set user creds for this exportpoint */
-	return nfserrno(nfsd_setuser(&rqstp->rq_cred, exp));
+	return nfserrno(nfsd_setuser(cred, exp));
 }
 
 static inline __be32 check_pseudo_root(struct dentry *dentry,
@@ -219,7 +220,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 		put_cred(override_creds(new));
 		put_cred(new);
 	} else {
-		error = nfsd_setuser_and_check_port(rqstp, exp);
+		error = nfsd_setuser_and_check_port(rqstp, &rqstp->rq_cred, exp);
 		if (error)
 			goto out;
 	}
@@ -358,7 +359,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
 	if (error)
 		goto out;
 
-	error = nfsd_setuser_and_check_port(rqstp, exp);
+	error = nfsd_setuser_and_check_port(rqstp, &rqstp->rq_cred, exp);
 	if (error)
 		goto out;
 
-- 
2.45.2

[RFC PATCH 3/6] NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Currently, fh_verify() makes some daring assumptions about which
version of file handle the caller wants, based on the things it can
find in the passed-in rqstp. The about-to-be-introduced LOCALIO use
case sometimes has no svc_rqst context, so this logic won't work in
that case.

Instead, examine the passed-in file handle. It's .max_size field
should carry information to allow nfsd_set_fh_dentry() to initialize
the file handle appropriately.

lockd appears to be the only kernel consumer that does not set the
file handle .max_size when during initialization.

write_filehandle() is the other question mark, as it looks possible
to specify a maxsize between NFS_FHSIZE and NFS3_FHSIZE here.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/lockd.c |  6 ++++--
 fs/nfsd/nfsfh.c | 11 +++++++----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
index 46a7f9b813e5..e636d2a1e664 100644
--- a/fs/nfsd/lockd.c
+++ b/fs/nfsd/lockd.c
@@ -32,8 +32,10 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
 	int		access;
 	struct svc_fh	fh;
 
-	/* must initialize before using! but maxsize doesn't matter */
-	fh_init(&fh,0);
+	if (rqstp->rq_vers == 4)
+		fh_init(&fh, NFS3_FHSIZE);
+	else
+		fh_init(&fh, NFS_FHSIZE);
 	fh.fh_handle.fh_size = f->size;
 	memcpy(&fh.fh_handle.fh_raw, f->data, f->size);
 	fh.fh_export = NULL;
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 4b964a71a504..77acc26e8b02 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -267,25 +267,28 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 	fhp->fh_dentry = dentry;
 	fhp->fh_export = exp;
 
-	switch (rqstp->rq_vers) {
-	case 4:
+	switch (fhp->fh_maxsize) {
+	case NFS4_FHSIZE:
 		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR)
 			fhp->fh_no_atomic_attr = true;
 		fhp->fh_64bit_cookies = true;
 		break;
-	case 3:
+	case NFS3_FHSIZE:
 		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOWCC)
 			fhp->fh_no_wcc = true;
 		fhp->fh_64bit_cookies = true;
 		if (exp->ex_flags & NFSEXP_V4ROOT)
 			goto out;
 		break;
-	case 2:
+	case NFS_FHSIZE:
 		fhp->fh_no_wcc = true;
 		if (EX_WGATHER(exp))
 			fhp->fh_use_wgather = true;
 		if (exp->ex_flags & NFSEXP_V4ROOT)
 			goto out;
+		break;
+	case 0:
+		WARN_ONCE(1, "Uninitialized file handle");
 	}
 
 	return 0;
-- 
2.45.2

Re: [RFC PATCH 3/6] NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()

[NeilBrown]

On Wed, 28 Aug 2024, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Currently, fh_verify() makes some daring assumptions about which
> version of file handle the caller wants, based on the things it can
> find in the passed-in rqstp. The about-to-be-introduced LOCALIO use
> case sometimes has no svc_rqst context, so this logic won't work in
> that case.
> 
> Instead, examine the passed-in file handle. It's .max_size field
> should carry information to allow nfsd_set_fh_dentry() to initialize
> the file handle appropriately.
> 
> lockd appears to be the only kernel consumer that does not set the
> file handle .max_size when during initialization.
> 
> write_filehandle() is the other question mark, as it looks possible
> to specify a maxsize between NFS_FHSIZE and NFS3_FHSIZE here.

The file handle used by lockd and the one created by write_filehandle
never need any of the version-specific fields.
Those fields affect things like write requests and getattr requests and
pre/post attributes.

I wonder if the filehandle is really the best place of these flag.
Having them in the file handle works really well for fh_fill_pre_attrs()
and reasonably well in other places, But it makes nfsd_set_fh_dentry a
little clumsy.  Maybe it would be better to moved them to
rqstp->rq_flags.  Or possibly in the "Catering to nfsd" section of
'struct svc_rqst'.

NeilBrown


> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/lockd.c |  6 ++++--
>  fs/nfsd/nfsfh.c | 11 +++++++----
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> index 46a7f9b813e5..e636d2a1e664 100644
> --- a/fs/nfsd/lockd.c
> +++ b/fs/nfsd/lockd.c
> @@ -32,8 +32,10 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
>  	int		access;
>  	struct svc_fh	fh;
>  
> -	/* must initialize before using! but maxsize doesn't matter */
> -	fh_init(&fh,0);
> +	if (rqstp->rq_vers == 4)
> +		fh_init(&fh, NFS3_FHSIZE);
> +	else
> +		fh_init(&fh, NFS_FHSIZE);
>  	fh.fh_handle.fh_size = f->size;
>  	memcpy(&fh.fh_handle.fh_raw, f->data, f->size);
>  	fh.fh_export = NULL;
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 4b964a71a504..77acc26e8b02 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -267,25 +267,28 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
>  	fhp->fh_dentry = dentry;
>  	fhp->fh_export = exp;
>  
> -	switch (rqstp->rq_vers) {
> -	case 4:
> +	switch (fhp->fh_maxsize) {
> +	case NFS4_FHSIZE:
>  		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR)
>  			fhp->fh_no_atomic_attr = true;
>  		fhp->fh_64bit_cookies = true;
>  		break;
> -	case 3:
> +	case NFS3_FHSIZE:
>  		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOWCC)
>  			fhp->fh_no_wcc = true;
>  		fhp->fh_64bit_cookies = true;
>  		if (exp->ex_flags & NFSEXP_V4ROOT)
>  			goto out;
>  		break;
> -	case 2:
> +	case NFS_FHSIZE:
>  		fhp->fh_no_wcc = true;
>  		if (EX_WGATHER(exp))
>  			fhp->fh_use_wgather = true;
>  		if (exp->ex_flags & NFSEXP_V4ROOT)
>  			goto out;
> +		break;
> +	case 0:
> +		WARN_ONCE(1, "Uninitialized file handle");
>  	}
>  
>  	return 0;
> -- 
> 2.45.2
> 
> 

Re: [RFC PATCH 3/6] NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()

[Chuck Lever]

On Wed, Aug 28, 2024 at 03:02:42PM +1000, NeilBrown wrote:
> On Wed, 28 Aug 2024, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > Currently, fh_verify() makes some daring assumptions about which
> > version of file handle the caller wants, based on the things it can
> > find in the passed-in rqstp. The about-to-be-introduced LOCALIO use
> > case sometimes has no svc_rqst context, so this logic won't work in
> > that case.
> > 
> > Instead, examine the passed-in file handle. It's .max_size field
> > should carry information to allow nfsd_set_fh_dentry() to initialize
> > the file handle appropriately.
> > 
> > lockd appears to be the only kernel consumer that does not set the
> > file handle .max_size when during initialization.
> > 
> > write_filehandle() is the other question mark, as it looks possible
> > to specify a maxsize between NFS_FHSIZE and NFS3_FHSIZE here.
> 
> The file handle used by lockd and the one created by write_filehandle
> never need any of the version-specific fields.
> Those fields affect things like write requests and getattr requests and
> pre/post attributes.

Then we could simply drop the "case 0:" arm and maybe the lockd hunk
from this patch.


> I wonder if the filehandle is really the best place of these flag.
> Having them in the file handle works really well for fh_fill_pre_attrs()
> and reasonably well in other places, But it makes nfsd_set_fh_dentry a
> little clumsy.

The file handle initialization code in here is indeed a bit awkward.
I didn't take it further in this patch set because I didn't have any
brilliant ideas, other than perhaps a version-specific
initialization call-out.

I don't think we need to go there to implement LOCALIO, but we could
save such a clean-up for another day.


> Maybe it would be better to moved them to
> rqstp->rq_flags.  Or possibly in the "Catering to nfsd" section of
> 'struct svc_rqst'.

Jeff and I have agreed to migrate NFS (or any upper layer protocol)
specifics out of the svc_rqst, where possible, simply to maintain
proper layering. I don't have any better ideas, though.


> NeilBrown
> 
> 
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/lockd.c |  6 ++++--
> >  fs/nfsd/nfsfh.c | 11 +++++++----
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> > index 46a7f9b813e5..e636d2a1e664 100644
> > --- a/fs/nfsd/lockd.c
> > +++ b/fs/nfsd/lockd.c
> > @@ -32,8 +32,10 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> >  	int		access;
> >  	struct svc_fh	fh;
> >  
> > -	/* must initialize before using! but maxsize doesn't matter */
> > -	fh_init(&fh,0);
> > +	if (rqstp->rq_vers == 4)
> > +		fh_init(&fh, NFS3_FHSIZE);
> > +	else
> > +		fh_init(&fh, NFS_FHSIZE);
> >  	fh.fh_handle.fh_size = f->size;
> >  	memcpy(&fh.fh_handle.fh_raw, f->data, f->size);
> >  	fh.fh_export = NULL;
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 4b964a71a504..77acc26e8b02 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -267,25 +267,28 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
> >  	fhp->fh_dentry = dentry;
> >  	fhp->fh_export = exp;
> >  
> > -	switch (rqstp->rq_vers) {
> > -	case 4:
> > +	switch (fhp->fh_maxsize) {
> > +	case NFS4_FHSIZE:
> >  		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR)
> >  			fhp->fh_no_atomic_attr = true;
> >  		fhp->fh_64bit_cookies = true;
> >  		break;
> > -	case 3:
> > +	case NFS3_FHSIZE:
> >  		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOWCC)
> >  			fhp->fh_no_wcc = true;
> >  		fhp->fh_64bit_cookies = true;
> >  		if (exp->ex_flags & NFSEXP_V4ROOT)
> >  			goto out;
> >  		break;
> > -	case 2:
> > +	case NFS_FHSIZE:
> >  		fhp->fh_no_wcc = true;
> >  		if (EX_WGATHER(exp))
> >  			fhp->fh_use_wgather = true;
> >  		if (exp->ex_flags & NFSEXP_V4ROOT)
> >  			goto out;
> > +		break;
> > +	case 0:
> > +		WARN_ONCE(1, "Uninitialized file handle");
> >  	}
> >  
> >  	return 0;
> > -- 
> > 2.45.2
> > 
> > 
> 

-- 
Chuck Lever

Re: [RFC PATCH 3/6] NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()

[Mike Snitzer]

On Wed, Aug 28, 2024 at 09:45:31AM -0400, Chuck Lever wrote:
> On Wed, Aug 28, 2024 at 03:02:42PM +1000, NeilBrown wrote:
> > On Wed, 28 Aug 2024, cel@kernel.org wrote:
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > > 
> > > Currently, fh_verify() makes some daring assumptions about which
> > > version of file handle the caller wants, based on the things it can
> > > find in the passed-in rqstp. The about-to-be-introduced LOCALIO use
> > > case sometimes has no svc_rqst context, so this logic won't work in
> > > that case.
> > > 
> > > Instead, examine the passed-in file handle. It's .max_size field
> > > should carry information to allow nfsd_set_fh_dentry() to initialize
> > > the file handle appropriately.
> > > 
> > > lockd appears to be the only kernel consumer that does not set the
> > > file handle .max_size when during initialization.
> > > 
> > > write_filehandle() is the other question mark, as it looks possible
> > > to specify a maxsize between NFS_FHSIZE and NFS3_FHSIZE here.
> > 
> > The file handle used by lockd and the one created by write_filehandle
> > never need any of the version-specific fields.
> > Those fields affect things like write requests and getattr requests and
> > pre/post attributes.
> 
> Then we could simply drop the "case 0:" arm and maybe the lockd hunk
> from this patch.

OK, will do.

[RFC PATCH 4/6] NFSD: Short-circuit fh_verify tracepoints for LOCALIO

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

LOCALIO will be able to call fh_verify() with a NULL rqstp. In this
case, the existing trace points need to be skipped because they
want to dereference the address fields in the passed-in rqstp.

Temporarily make these trace points conditional to avoid a seg
fault in this case. Putting the "rqstp != NULL" check in the trace
points themselves makes the check more efficient.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/trace.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 77bbd23aa150..d22027e23761 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -193,7 +193,7 @@ TRACE_EVENT(nfsd_compound_encode_err,
 		{ S_IFIFO,		"FIFO" }, \
 		{ S_IFSOCK,		"SOCK" })
 
-TRACE_EVENT(nfsd_fh_verify,
+TRACE_EVENT_CONDITION(nfsd_fh_verify,
 	TP_PROTO(
 		const struct svc_rqst *rqstp,
 		const struct svc_fh *fhp,
@@ -201,6 +201,7 @@ TRACE_EVENT(nfsd_fh_verify,
 		int access
 	),
 	TP_ARGS(rqstp, fhp, type, access),
+	TP_CONDITION(rqstp != NULL),
 	TP_STRUCT__entry(
 		__field(unsigned int, netns_ino)
 		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
@@ -239,7 +240,7 @@ TRACE_EVENT_CONDITION(nfsd_fh_verify_err,
 		__be32 error
 	),
 	TP_ARGS(rqstp, fhp, type, access, error),
-	TP_CONDITION(error),
+	TP_CONDITION(rqstp != NULL && error),
 	TP_STRUCT__entry(
 		__field(unsigned int, netns_ino)
 		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
@@ -295,12 +296,13 @@ DECLARE_EVENT_CLASS(nfsd_fh_err_class,
 		  __entry->status)
 )
 
-#define DEFINE_NFSD_FH_ERR_EVENT(name)		\
-DEFINE_EVENT(nfsd_fh_err_class, nfsd_##name,	\
-	TP_PROTO(struct svc_rqst *rqstp,	\
-		 struct svc_fh	*fhp,		\
-		 int		status),	\
-	TP_ARGS(rqstp, fhp, status))
+#define DEFINE_NFSD_FH_ERR_EVENT(name)			\
+DEFINE_EVENT_CONDITION(nfsd_fh_err_class, nfsd_##name,	\
+	TP_PROTO(struct svc_rqst *rqstp,		\
+		 struct svc_fh	*fhp,			\
+		 int		status),		\
+	TP_ARGS(rqstp, fhp, status),			\
+	TP_CONDITION(rqstp != NULL))
 
 DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
 DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
-- 
2.45.2

[RFC PATCH 5/6] nfsd: factor out __fh_verify to allow NULL rqstp to be passed

[cel]

From: NeilBrown <neilb@suse.de>

__fh_verify() offers an interface like fh_verify() but doesn't require
a struct svc_rqst *, instead it also takes the specific parts as
explicit required arguments.  So it is safe to call __fh_verify() with
a NULL rqstp, but the net, cred, and client args must not be NULL.

__fh_verify() does not use SVC_NET(), nor does the functions it calls.

Rather then depending on rqstp->rq_vers to determine nfs version, pass
it in explicitly.  This removes another dependency on rqstp and ensures
the correct version is checked.  The rqstp can be for an NLM request and
while some code tests that, other code does not.

Rather than using rqstp->rq_client pass the client and gssclient
explicitly to __fh_verify and then to nfsd_set_fh_dentry().

Lastly, 4 associated tracepoints are only used if rqstp is not NULL
(this is a stop-gap that should be properly fixed so localio also
benefits from the utility these tracepoints provide when debugging
fh_verify issues).

Signed-off-by: NeilBrown <neilb@suse.de>
Co-developed-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfsfh.c | 168 ++++++++++++++++++++++++++----------------------
 1 file changed, 92 insertions(+), 76 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 77acc26e8b02..80c06e170e9a 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -142,7 +142,11 @@ static inline __be32 check_pseudo_root(struct dentry *dentry,
  * dentry.  On success, the results are used to set fh_export and
  * fh_dentry.
  */
-static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
+static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
+				 struct svc_cred *cred,
+				 struct auth_domain *client,
+				 struct auth_domain *gssclient,
+				 struct svc_fh *fhp)
 {
 	struct knfsd_fh	*fh = &fhp->fh_handle;
 	struct fid *fid = NULL;
@@ -184,8 +188,8 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 	data_left -= len;
 	if (data_left < 0)
 		return error;
-	exp = rqst_exp_find(&rqstp->rq_chandle, SVC_NET(rqstp),
-			    rqstp->rq_client, rqstp->rq_gssclient,
+	exp = rqst_exp_find(rqstp ? &rqstp->rq_chandle : NULL,
+			    net, client, gssclient,
 			    fh->fh_fsid_type, fh->fh_fsid);
 	fid = (struct fid *)(fh->fh_fsid + len);
 
@@ -220,7 +224,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 		put_cred(override_creds(new));
 		put_cred(new);
 	} else {
-		error = nfsd_setuser_and_check_port(rqstp, &rqstp->rq_cred, exp);
+		error = nfsd_setuser_and_check_port(rqstp, cred, exp);
 		if (error)
 			goto out;
 	}
@@ -297,6 +301,87 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 	return error;
 }
 
+static __be32
+__fh_verify(struct svc_rqst *rqstp,
+	    struct net *net, struct svc_cred *cred,
+	    struct auth_domain *client,
+	    struct auth_domain *gssclient,
+	    struct svc_fh *fhp, umode_t type, int access)
+{
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+	struct svc_export *exp = NULL;
+	struct dentry	*dentry;
+	__be32		error;
+
+	if (!fhp->fh_dentry) {
+		error = nfsd_set_fh_dentry(rqstp, net, cred, client,
+					   gssclient, fhp);
+		if (error)
+			goto out;
+	}
+	dentry = fhp->fh_dentry;
+	exp = fhp->fh_export;
+
+	trace_nfsd_fh_verify(rqstp, fhp, type, access);
+
+	/*
+	 * We still have to do all these permission checks, even when
+	 * fh_dentry is already set:
+	 * 	- fh_verify may be called multiple times with different
+	 * 	  "access" arguments (e.g. nfsd_proc_create calls
+	 * 	  fh_verify(...,NFSD_MAY_EXEC) first, then later (in
+	 * 	  nfsd_create) calls fh_verify(...,NFSD_MAY_CREATE).
+	 *	- in the NFSv4 case, the filehandle may have been filled
+	 *	  in by fh_compose, and given a dentry, but further
+	 *	  compound operations performed with that filehandle
+	 *	  still need permissions checks.  In the worst case, a
+	 *	  mountpoint crossing may have changed the export
+	 *	  options, and we may now need to use a different uid
+	 *	  (for example, if different id-squashing options are in
+	 *	  effect on the new filesystem).
+	 */
+	error = check_pseudo_root(dentry, exp);
+	if (error)
+		goto out;
+
+	error = nfsd_setuser_and_check_port(rqstp, cred, exp);
+	if (error)
+		goto out;
+
+	error = nfsd_mode_check(dentry, type);
+	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 || access & NFSD_MAY_BYPASS_GSS)
+		goto skip_pseudoflavor_check;
+	/*
+	 * Clients may expect to be able to use auth_sys during mount,
+	 * even if they use gss for everything else; see section 2.3.2
+	 * of rfc 2623.
+	 */
+	if (access & NFSD_MAY_BYPASS_GSS_ON_ROOT
+			&& exp->ex_path.dentry == dentry)
+		goto skip_pseudoflavor_check;
+
+	error = check_nfsd_access(exp, rqstp);
+	if (error)
+		goto out;
+
+skip_pseudoflavor_check:
+	/* Finally, check access permissions. */
+	error = nfsd_permission(cred, exp, dentry, access);
+out:
+	trace_nfsd_fh_verify_err(rqstp, fhp, type, access, error);
+	if (error == nfserr_stale)
+		nfsd_stats_fh_stale_inc(nn, exp);
+	return error;
+}
+
 /**
  * fh_verify - filehandle lookup and access checking
  * @rqstp: pointer to current rpc request
@@ -327,80 +412,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 __be32
 fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
 {
-	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
-	struct svc_export *exp = NULL;
-	struct dentry	*dentry;
-	__be32		error;
-
-	if (!fhp->fh_dentry) {
-		error = nfsd_set_fh_dentry(rqstp, fhp);
-		if (error)
-			goto out;
-	}
-	dentry = fhp->fh_dentry;
-	exp = fhp->fh_export;
-
-	trace_nfsd_fh_verify(rqstp, fhp, type, access);
-
-	/*
-	 * We still have to do all these permission checks, even when
-	 * fh_dentry is already set:
-	 * 	- fh_verify may be called multiple times with different
-	 * 	  "access" arguments (e.g. nfsd_proc_create calls
-	 * 	  fh_verify(...,NFSD_MAY_EXEC) first, then later (in
-	 * 	  nfsd_create) calls fh_verify(...,NFSD_MAY_CREATE).
-	 *	- in the NFSv4 case, the filehandle may have been filled
-	 *	  in by fh_compose, and given a dentry, but further
-	 *	  compound operations performed with that filehandle
-	 *	  still need permissions checks.  In the worst case, a
-	 *	  mountpoint crossing may have changed the export
-	 *	  options, and we may now need to use a different uid
-	 *	  (for example, if different id-squashing options are in
-	 *	  effect on the new filesystem).
-	 */
-	error = check_pseudo_root(dentry, exp);
-	if (error)
-		goto out;
-
-	error = nfsd_setuser_and_check_port(rqstp, &rqstp->rq_cred, exp);
-	if (error)
-		goto out;
-
-	error = nfsd_mode_check(dentry, type);
-	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 || access & NFSD_MAY_BYPASS_GSS)
-		goto skip_pseudoflavor_check;
-	/*
-	 * Clients may expect to be able to use auth_sys during mount,
-	 * even if they use gss for everything else; see section 2.3.2
-	 * of rfc 2623.
-	 */
-	if (access & NFSD_MAY_BYPASS_GSS_ON_ROOT
-			&& exp->ex_path.dentry == dentry)
-		goto skip_pseudoflavor_check;
-
-	error = check_nfsd_access(exp, rqstp);
-	if (error)
-		goto out;
-
-skip_pseudoflavor_check:
-	/* Finally, check access permissions. */
-	error = nfsd_permission(&rqstp->rq_cred, exp, dentry, access);
-out:
-	trace_nfsd_fh_verify_err(rqstp, fhp, type, access, error);
-	if (error == nfserr_stale)
-		nfsd_stats_fh_stale_inc(nn, exp);
-	return error;
+	return __fh_verify(rqstp, SVC_NET(rqstp), &rqstp->rq_cred,
+			   rqstp->rq_client, rqstp->rq_gssclient,
+			   fhp, type, access);
 }
 
-
 /*
  * Compose a file handle for an NFS reply.
  *
-- 
2.45.2

[RFC PATCH 6/6] nfsd: add nfsd_file_acquire_local()

[cel]

From: NeilBrown <neilb@suse.de>

nfsd_file_acquire_local() can be used to look up a file by filehandle
without having a struct svc_rqst.  This can be used by NFS LOCALIO to
allow the NFS client to bypass the NFS protocol to directly access a
file provided by the NFS server which is running in the same kernel.

In nfsd_file_do_acquire() care is taken to always use fh_verify() if
rqstp is not NULL (as is the case for non-LOCALIO callers).  Otherwise
the non-LOCALIO callers will not supply the correct and required
arguments to __fh_verify (e.g. gssclient isn't passed).

Also, use GC for nfsd_file returned by nfsd_file_acquire_local.  GC
offers performance improvements if/when a file is reopened before
launderette cleans it from the filecache's LRU.

Suggested-by: Jeff Layton <jlayton@kernel.org> # use filecache's GC
Signed-off-by: NeilBrown <neilb@suse.de>
Co-developed-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/filecache.c | 61 +++++++++++++++++++++++++++++++++++++++------
 fs/nfsd/filecache.h |  3 +++
 fs/nfsd/nfsfh.c     | 18 ++++++++++++-
 fs/nfsd/nfsfh.h     |  5 ++++
 4 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 9e9d246f993c..40f19e9af0ba 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -982,12 +982,14 @@ nfsd_file_is_cached(struct inode *inode)
 }
 
 static __be32
-nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
+nfsd_file_do_acquire(struct svc_rqst *rqstp, struct net *net,
+		     struct svc_cred *cred,
+		     struct auth_domain *client,
+		     struct svc_fh *fhp,
 		     unsigned int may_flags, struct file *file,
 		     struct nfsd_file **pnf, bool want_gc)
 {
 	unsigned char need = may_flags & NFSD_FILE_MAY_MASK;
-	struct net *net = SVC_NET(rqstp);
 	struct nfsd_file *new, *nf;
 	bool stale_retry = true;
 	bool open_retry = true;
@@ -996,8 +998,13 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	int ret;
 
 retry:
-	status = fh_verify(rqstp, fhp, S_IFREG,
-				may_flags|NFSD_MAY_OWNER_OVERRIDE);
+	if (rqstp) {
+		status = fh_verify(rqstp, fhp, S_IFREG,
+				   may_flags|NFSD_MAY_OWNER_OVERRIDE);
+	} else {
+		status = __fh_verify(NULL, net, cred, client, NULL, fhp,
+				     S_IFREG, may_flags|NFSD_MAY_OWNER_OVERRIDE);
+	}
 	if (status != nfs_ok)
 		return status;
 	inode = d_inode(fhp->fh_dentry);
@@ -1143,7 +1150,8 @@ __be32
 nfsd_file_acquire_gc(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		     unsigned int may_flags, struct nfsd_file **pnf)
 {
-	return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, true);
+	return nfsd_file_do_acquire(rqstp, SVC_NET(rqstp), NULL, NULL,
+				    fhp, may_flags, NULL, pnf, true);
 }
 
 /**
@@ -1167,7 +1175,45 @@ __be32
 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		  unsigned int may_flags, struct nfsd_file **pnf)
 {
-	return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, false);
+	return nfsd_file_do_acquire(rqstp, SVC_NET(rqstp), NULL, NULL,
+				    fhp, may_flags, NULL, pnf, false);
+}
+
+/**
+ * nfsd_file_acquire_local - Get a struct nfsd_file with an open file for localio
+ * @net: The network namespace in which to perform a lookup
+ * @cred: the user credential with which to validate access
+ * @client: the auth_domain for LOCALIO lookup
+ * @fhp: the NFS filehandle of the file to be opened
+ * @may_flags: NFSD_MAY_ settings for the file
+ * @pnf: OUT: new or found "struct nfsd_file" object
+ *
+ * This file lookup interface provide access to a file given the
+ * filehandle and credential.  No connection-based authorisation
+ * is performed and in that way it is quite different to other
+ * file access mediated by nfsd.  It allows a kernel module such as the NFS
+ * client to reach across network and filesystem namespaces to access
+ * a file.  The security implications of this should be carefully
+ * considered before use.
+ *
+ * The nfsd_file object returned by this API is reference-counted
+ * and garbage-collected. The object is retained for a few
+ * seconds after the final nfsd_file_put() in case the caller
+ * wants to re-use it.
+ *
+ * Return values:
+ *   %nfs_ok - @pnf points to an nfsd_file with its reference
+ *   count boosted.
+ *
+ * On error, an nfsstat value in network byte order is returned.
+ */
+__be32
+nfsd_file_acquire_local(struct net *net, struct svc_cred *cred,
+			struct auth_domain *client, struct svc_fh *fhp,
+			unsigned int may_flags, struct nfsd_file **pnf)
+{
+	return nfsd_file_do_acquire(NULL, net, cred, client,
+				    fhp, may_flags, NULL, pnf, true);
 }
 
 /**
@@ -1193,7 +1239,8 @@ nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			 unsigned int may_flags, struct file *file,
 			 struct nfsd_file **pnf)
 {
-	return nfsd_file_do_acquire(rqstp, fhp, may_flags, file, pnf, false);
+	return nfsd_file_do_acquire(rqstp, SVC_NET(rqstp), NULL, NULL,
+				    fhp, may_flags, file, pnf, false);
 }
 
 /*
diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
index 3fbec24eea6c..26ada78b8c1e 100644
--- a/fs/nfsd/filecache.h
+++ b/fs/nfsd/filecache.h
@@ -66,5 +66,8 @@ __be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 __be32 nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		  unsigned int may_flags, struct file *file,
 		  struct nfsd_file **nfp);
+__be32 nfsd_file_acquire_local(struct net *net, struct svc_cred *cred,
+			       struct auth_domain *client, struct svc_fh *fhp,
+			       unsigned int may_flags, struct nfsd_file **pnf);
 int nfsd_file_cache_stats_show(struct seq_file *m, void *v);
 #endif /* _FS_NFSD_FILECACHE_H */
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 80c06e170e9a..41be0f15182d 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -301,7 +301,23 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
 	return error;
 }
 
-static __be32
+/**
+ * __fh_verify - filehandle lookup and access checking
+ * @rqstp: RPC transaction context, or NULL
+ * @net: net namespace in which to perform the export lookup
+ * @cred: RPC user credential
+ * @client: RPC auth domain
+ * @gssclient: RPC GSS auth domain
+ * @fhp: filehandle to be verified
+ * @type: expected type of object pointed to by filehandle
+ * @access: type of access needed to object
+ *
+ * This internal API can be used by callers who do not have an RPC
+ * transaction context (ie are not running in an nfsd thread).
+ *
+ * See fh_verify() for further descriptions of @fhp, @type, and @access.
+ */
+__be32
 __fh_verify(struct svc_rqst *rqstp,
 	    struct net *net, struct svc_cred *cred,
 	    struct auth_domain *client,
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 8d46e203d139..8dd653ba4100 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -217,6 +217,11 @@ extern char * SVCFH_fmt(struct svc_fh *fhp);
  * Function prototypes
  */
 __be32	fh_verify(struct svc_rqst *, struct svc_fh *, umode_t, int);
+__be32	__fh_verify(struct svc_rqst *rqstp,
+		    struct net *net, struct svc_cred *cred,
+		    struct auth_domain *client,
+		    struct auth_domain *gssclient,
+		    struct svc_fh *fhp, umode_t type, int access);
 __be32	fh_compose(struct svc_fh *, struct svc_export *, struct dentry *, struct svc_fh *);
 __be32	fh_update(struct svc_fh *);
 void	fh_put(struct svc_fh *);
-- 
2.45.2

Re: [RFC PATCH 0/6] Split up refactoring of fh_verify()

[Mike Snitzer]

On Tue, Aug 27, 2024 at 08:44:39PM -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> These six patches are meant to replace:
> 
>   nfsd: factor out __fh_verify to allow NULL rqstp to be passed
>   nfsd: add nfsd_file_acquire_local()
> 
> I've removed the @nfs_vers parameter to __fh_verify(), and tried
> something a little different with the trace points. Please check
> my math.
> 
> These have been compile-tested, but no more than that. Interested in
> comments.

Reviewed quickly just now, nicely done!

I'll go over it very carefully now by replacing the 2 patches you
noted and updating the localio patches thaa follow.  I noticed some
typos and mentioning nfs_vers usage in a header despite you having
removed the need to pass it. So I'll fix up those nits along the way.

But I just wanted to immediately say: thanks!

> Chuck Lever (2):
>   NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()
>   NFSD: Short-circuit fh_verify tracepoints for LOCALIO
> 
> NeilBrown (4):
>   NFSD: Handle @rqstp == NULL in check_nfsd_access()
>   NFSD: Refactor nfsd_setuser_and_check_port()
>   nfsd: factor out __fh_verify to allow NULL rqstp to be passed
>   nfsd: add nfsd_file_acquire_local()
> 
>  fs/nfsd/export.c    |  29 ++++--
>  fs/nfsd/filecache.c |  61 +++++++++++--
>  fs/nfsd/filecache.h |   3 +
>  fs/nfsd/lockd.c     |   6 +-
>  fs/nfsd/nfsfh.c     | 210 ++++++++++++++++++++++++++------------------
>  fs/nfsd/nfsfh.h     |   5 ++
>  fs/nfsd/trace.h     |  18 ++--
>  7 files changed, 223 insertions(+), 109 deletions(-)
> 
> -- 
> 2.45.2
> 

Re: [RFC PATCH 0/6] Split up refactoring of fh_verify()

[Mike Snitzer]

On Tue, Aug 27, 2024 at 09:08:32PM -0400, Mike Snitzer wrote:
> On Tue, Aug 27, 2024 at 08:44:39PM -0400, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > These six patches are meant to replace:
> > 
> >   nfsd: factor out __fh_verify to allow NULL rqstp to be passed
> >   nfsd: add nfsd_file_acquire_local()
> > 
> > I've removed the @nfs_vers parameter to __fh_verify(), and tried
> > something a little different with the trace points. Please check
> > my math.
> > 
> > These have been compile-tested, but no more than that. Interested in
> > comments.
> 
> Reviewed quickly just now, nicely done!
> 
> I'll go over it very carefully now by replacing the 2 patches you
> noted and updating the localio patches thaa follow.  I noticed some
> typos and mentioning nfs_vers usage in a header despite you having
> removed the need to pass it. So I'll fix up those nits along the way.
> 
> But I just wanted to immediately say: thanks!

I reviewed and incorporated your 6 patches and successfully tested the
result (by issuing IO with and without LOCALIO enabled with both NFS
v3 and NFS v4.2).

I've pushed the latest code to my branch:
https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=nfs-localio-for-next

I'm approaching the end of addressing all the v13 review feedback, but
still have to sort through a couple issues that Neil pointed out about
nfs_local_disable() racing with nfsd_open_local_fh().

But in case you're curious here is a preview of the changelog for
what'll be in v14 (hopefully will post by end of day tomorrow):

- Extended the nn->nfsd_serv reference lifetime to be identical to the
  nfsd_file (until after localio's IO is complete), suggested by Neil.
  This is made easier by introducing a 'struct nfs_localio_ctx' that
  contains both the 'nfsd_file' and 'nfsd_net' associated with
  localio.

- Switched nfs_common's 'nfs_to' symbol management locking from using
  mutex to spinlock, suggested by Neil.

- Eliminated nfs_local_file_open() by folding it into
  nfs_local_open_fh(), suggested by Neil.

- Update nfs_uuid_is_local() to get reference on the net, drop it in
  nfs_local_disable(), suggested by Neil.

- Pushed saving/restoring of client's cred down from
  nfsd_open_local_fh() to nfsd_file_acquire_local(), suggested by
  Neil.

- Updated NFSD_LOCALIO in fs/nfsd/Kconfig to explicitly 'default n'
  and improve description, suggested by Chuck. Also made the same
  updates to NFS_LOCALIO in fs/nfs/Kconfig.

- Split out a separate preliminary patch that introduces
  nfsd_serv_try_get() and nfsd_serv_put() and the associated
  percpu_ref, suggested by Chuck.

- Improved "Always allow LOCALIO" comment in check_nfsd_access() and
  added Kdoc above check_nfsd_access(), suggested by Chuck.

- Moved rpcauth_map_clnt_to_svc_cred_local from net/sunrpc/auth.c to
  net/sunrpc/svcauth.c and renamed it to
  svcauth_map_clnt_to_svc_cred_local. Also added kdoc. Suggested by
  Chuck.

- Added Chuck's Acked-by to 2 patches

- Incorporated Chuck's 6 patches that split up and improved the
  __fh_verify and nfsd_file_acquire_local patches.