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

[Chuck Lever <chuck.lever@oracle.com>]

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?


>>> +
>>> +	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