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@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.
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?
