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@xxxxxxx>
> > >
> > > __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@xxxxxxx>
> > > Co-developed-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> >
> > 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.
> > > --- 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.
--
Chuck Lever
