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

[Mike Snitzer <snitzer@kernel.org>]

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. ;)

> A few more specific comments below.
> 
> 
> > ---
> >  fs/nfsd/export.c |   8 ++-
> >  fs/nfsd/nfsfh.c  | 124 ++++++++++++++++++++++++++++-------------------
> >  2 files changed, 82 insertions(+), 50 deletions(-)
> > 
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index 7bb4f2075ac5..fe36f441d1d9 100644
> > --- 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.

Thanks,
Mike