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@xxxxxxxxxx wrote:
> > > From: NeilBrown <neilb@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > > Co-developed-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > > Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > > ---
> > > 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
