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

[Mike Snitzer <snitzer@kernel.org>]

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?