Re: [PATCH] nfsd: decouple the xprtsec policy check from check_nfsd_access()

[Scott Mayhew <smayhew@redhat.com>]

On Tue, 05 Aug 2025, Chuck Lever wrote:

> On 8/5/25 10:32 AM, Scott Mayhew wrote:
> > On Fri, 01 Aug 2025, Chuck Lever wrote:
> > 
> >> On 7/31/25 5:14 PM, Scott Mayhew wrote:
> 
> >>> +}
> >>> +
> >>> +/**
> >>> + * 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).
> >>> + * @may_bypass_gss: reduce strictness of authorization check
> >>> + *
> >>> + * 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,
> >>> +			 bool may_bypass_gss)
> >>> +{
> >>> +	struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors;
> >>> +	struct svc_xprt *xprt;
> >>> +
> >>> +	/*
> >>> +	 * If rqstp is NULL, this is a LOCALIO request which will only
> >>> +	 * ever use a filehandle/credential pair for which access has
> >>> +	 * been affirmed (by ACCESS or OPEN NFS requests) over the
> >>> +	 * wire. So there is no need for further checks here.
> >>> +	 */
> >>> +	if (!rqstp)
> >>> +		return nfs_ok;
> >>
> >> Is this true of all of check_nfsd_access's callers, or only of
> >> __fh_verify ?
> >>
> > Looking at the commit where this check was added, and looking at the
> > other callers, it looks like this is only true of __fh_verify().
> > 
> > I'm splitting up check_nfsd_access() into two helpers has you suggested,
> > having __fh_verify() call the helpers directly while having the other
> > callers continue to use check_nfsd_access().
> > 
> > Should I add an argument to the helpers indicate when they have been
> > called directly?  Something like 'bool maybe_localio', which can
> > then be incorporated into the above check, e.g.
> > 
> >         if (!rqstp) {
> >                 if (maybe_localio) {
> >                         return nfs_ok;
> >                 } else {
> >                         WARN_ON_ONCE(1);
> >                         return nfserr_wrongsec;
> >                 }
> >         }
> 
> If __fh_verify is the only call site that can invoke these helpers with
> rqstp == NULL, then __fh_verify seems like the place to do this check,
> not in the helpers. But maybe I've misunderstood something?

No, that makes sense.  Thanks.
> 
> 
> >>> +
> >>> +	xprt = rqstp->rq_xprt;
> >>> +
> >>>  	/* legacy gss-only clients are always OK: */
> >>>  	if (exp->ex_client == rqstp->rq_gssclient)
> >>>  		return nfs_ok;
> >>> @@ -1167,7 +1202,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >>>  		}
> >>>  	}
> >>>  
> >>> -denied:
> >>>  	return nfserr_wrongsec;
> >>>  }
> >>>  
> 
> 
> -- 
> Chuck Lever
>