Re: [PATCH v13 04/19] nfsd: factor out __fh_verify to allow NULL rqstp to be passed

[Mike Snitzer <snitzer@kernel.org>]

On Tue, Aug 27, 2024 at 03:26:36PM -0400, Chuck Lever wrote:
> On Tue, Aug 27, 2024 at 02:58:21PM -0400, Mike Snitzer wrote:
> > On Sun, Aug 25, 2024 at 11:32:50AM -0400, Chuck Lever wrote:
> > > On Fri, Aug 23, 2024 at 02:14:02PM -0400, Mike Snitzer wrote:
> > > > 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().
> > > > 
> > > > The final places where __fh_verify unconditionally dereferences rqstp
> > > > involve checking if 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.
> > > > So have these always succeed when rqstp is NULL.
> > > > 
> > > > 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>
> > > 
> > > IMO this patch needs to be split up. There are several changes that
> > > need separate explanation and rationale, and the changes here need
> > > to be individually bisectable.
> > > 
> > > If you prefer, I can provide a patch series that replaces this one
> > > patch, or Neil could provide it if he wants.
> > 
> > I'm probably not the best person to take a crack at splitting this
> > patch up.
> > 
> > Neil initially had factored out N patches, but they weren't fully
> > baked so when I had to fix the code up it became a challenge to
> > sprinkle fixes across the N patches.  Because they were all
> > pretty interdependent.
> > 
> > In the end, all these changes are in service to allowing for the
> > possibility that the rqstp not available (NULL).  So it made sense to
> > fold them together.
> > 
> > I really don't see how factoring these changes out to N patches makes
> > them useful for bisection (you need all of them to test the case when
> > rqstp is NULL, and a partial application of changes to track down a
> > rqstp-full regression really isn't such a concern given fh_verify()
> > fully passes all args to __fh_verify so status-quo preserved).
> > 
> > All said, if your intuition and experience makes you feel splitting
> > this patch up is needed then I'm fine with it and I welcome your or
> > Neil's contribtion.  It is fiddley work though, so having had my own
> > challenges with the code when these changes were split out makes me
> > hesitant to jump on splitting them out again.
> > 
> > Hope I've explained myself clearly... not being confrontational,
> > dismissive or anything else. ;)
> 
> True, this isn't an enormous single patch, but you gotta draw that
> line somewhere.
> 
> The goal is to make these changes as small and atomic as possible so
> it becomes easy to spot a mistake or bisect to find unintended
> behavior introduced in the non-LOCALIO case. This is a code path
> that is heavily utilized by NFSD so it pays to take some defensive
> precautions.
> 
> Generally I find that a typo or hidden assumption magically appears
> when I've engaged in this kind of refactoring exercise. I've got a
> good toolchain that should make this quick work.

Awesome.

> > > > --- a/fs/nfsd/export.c
> > > > +++ b/fs/nfsd/export.c
> > > > @@ -1077,7 +1077,13 @@ static struct svc_export *exp_find(struct cache_detail *cd,
> > > >  __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;
> > > > +
> > > > +	if (!rqstp)
> > > > +		/* Always allow LOCALIO */
> > > > +		return 0;
> > > > +
> > > > +	xprt = rqstp->rq_xprt;
> > > 
> > > check_nfsd_access() is a public API, so it needs a kdoc comment.
> > > 
> > > These changes should be split into a separate patch with a clear
> > > rationale of why "Always allow LOCALIO" is secure and appropriate
> > > to do.
> > 
> > Separate patch aside, I'll try to improve that comment.
> 
> Post the comment update here and I can work that in.

Here is the incremental patch:

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index fe36f441d1d9..8b54f66f0e0f 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1074,14 +1074,26 @@ 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).
+ *
+ * Returns 0 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;
 
-	if (!rqstp)
-		/* Always allow LOCALIO */
+	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;
+	}
 
 	xprt = rqstp->rq_xprt;