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
>
