Re: [PATCH v14 08/25] nfsd: factor out __fh_verify to allow NULL rqstp to be passed

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Aug 29, 2024 at 10:39:33AM -0400, Jeff Layton wrote:
> On Wed, 2024-08-28 at 21:04 -0400, Mike Snitzer wrote:
> > From: NeilBrown <neilb@suse.de>
> > 
> > __fh_verify() offers an interface like fh_verify() but doesn't require
> > a struct svc_rqst *, instead it also takes the specific parts as
> > explicit required arguments.  So it is safe to call __fh_verify() with
> > a NULL rqstp, but the net, cred, and client args must not be NULL.
> > 
> > __fh_verify() does not use SVC_NET(), nor does the functions it calls.
> > 
> > Rather than using rqstp->rq_client pass the client and gssclient
> > explicitly to __fh_verify and then to nfsd_set_fh_dentry().
> > 
> > Lastly, 4 associated tracepoints are only used if rqstp is not NULL
> > (this is a stop-gap that should be properly fixed so localio also
> > benefits from the utility these tracepoints provide when debugging
> > fh_verify issues).
> > 
> 
> nit: this last paragraph doesn't apply anymore with the inclusion of
> the previous patch

I thought that too, but then I considered it further and it is still
applicable, just that the previous patch is the one dealing with it.
I think it still worthwhile to mention the lack of fh_verify tracing
for localio in this header,

> 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > Co-developed-by: Mike Snitzer <snitzer@kernel.org>
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfsfh.c | 90 +++++++++++++++++++++++++++++--------------------
> >  1 file changed, 53 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 77acc26e8b02..80c06e170e9a 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -142,7 +142,11 @@ static inline __be32 check_pseudo_root(struct dentry *dentry,
> >   * dentry.  On success, the results are used to set fh_export and
> >   * fh_dentry.
> >   */
> > -static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
> > +static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
> > +				 struct svc_cred *cred,
> > +				 struct auth_domain *client,
> > +				 struct auth_domain *gssclient,
> > +				 struct svc_fh *fhp)
> >  {
> >  	struct knfsd_fh	*fh = &fhp->fh_handle;
> >  	struct fid *fid = NULL;
> > @@ -184,8 +188,8 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
> >  	data_left -= len;
> >  	if (data_left < 0)
> >  		return error;
> > -	exp = rqst_exp_find(&rqstp->rq_chandle, SVC_NET(rqstp),
> > -			    rqstp->rq_client, rqstp->rq_gssclient,
> > +	exp = rqst_exp_find(rqstp ? &rqstp->rq_chandle : NULL,
> > +			    net, client, gssclient,
> >  			    fh->fh_fsid_type, fh->fh_fsid);
> >  	fid = (struct fid *)(fh->fh_fsid + len);
> >  
> > @@ -220,7 +224,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
> >  		put_cred(override_creds(new));
> >  		put_cred(new);
> >  	} else {
> > -		error = nfsd_setuser_and_check_port(rqstp, &rqstp->rq_cred, exp);
> > +		error = nfsd_setuser_and_check_port(rqstp, cred, exp);
> >  		if (error)
> >  			goto out;
> >  	}
> > @@ -297,43 +301,21 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
> >  	return error;
> >  }
> >  
> > -/**
> > - * fh_verify - filehandle lookup and access checking
> > - * @rqstp: pointer to current rpc request
> > - * @fhp: filehandle to be verified
> > - * @type: expected type of object pointed to by filehandle
> > - * @access: type of access needed to object
> > - *
> > - * Look up a dentry from the on-the-wire filehandle, check the client's
> > - * access to the export, and set the current task's credentials.
> > - *
> > - * Regardless of success or failure of fh_verify(), fh_put() should be
> > - * called on @fhp when the caller is finished with the filehandle.
> > - *
> > - * fh_verify() may be called multiple times on a given filehandle, for
> > - * example, when processing an NFSv4 compound.  The first call will look
> > - * up a dentry using the on-the-wire filehandle.  Subsequent calls will
> > - * skip the lookup and just perform the other checks and possibly change
> > - * the current task's credentials.
> > - *
> > - * @type specifies the type of object expected using one of the S_IF*
> > - * constants defined in include/linux/stat.h.  The caller may use zero
> > - * to indicate that it doesn't care, or a negative integer to indicate
> > - * that it expects something not of the given type.
> > - *
> > - * @access is formed from the NFSD_MAY_* constants defined in
> > - * fs/nfsd/vfs.h.
> > - */
> > -__be32
> > -fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> > +static __be32
> > +__fh_verify(struct svc_rqst *rqstp,
> > +	    struct net *net, struct svc_cred *cred,
> > +	    struct auth_domain *client,
> > +	    struct auth_domain *gssclient,
> > +	    struct svc_fh *fhp, umode_t type, int access)
> 
> I don't consider is a show-stopper, but it might be good to have a
> kerneldoc header on this, just because it has so many parameters.
> Having them clearly spelled out, and the rules around what must be set
> when rqstp is NULL would make it less likely we'll break those
> assumptions in the future.

Yeah, it does get backfilled in the next patch (which you just
reviewed so I'm just telling you something you know, this is just for
the benefit of others I guess).

The sequencing of the changes between this and the acquire_local patch
could be improved though.  SO if I need to do a v15 (I hope not!) I'll
clean it up ;)

Thanks,
Mike


> 
> >  {
> > -	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> >  	struct svc_export *exp = NULL;
> >  	struct dentry	*dentry;
> >  	__be32		error;
> >  
> >  	if (!fhp->fh_dentry) {
> > -		error = nfsd_set_fh_dentry(rqstp, fhp);
> > +		error = nfsd_set_fh_dentry(rqstp, net, cred, client,
> > +					   gssclient, fhp);
> >  		if (error)
> >  			goto out;
> >  	}
> > @@ -362,7 +344,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> >  	if (error)
> >  		goto out;
> >  
> > -	error = nfsd_setuser_and_check_port(rqstp, &rqstp->rq_cred, exp);
> > +	error = nfsd_setuser_and_check_port(rqstp, cred, exp);
> >  	if (error)
> >  		goto out;
> >  
> > @@ -392,7 +374,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> >  
> >  skip_pseudoflavor_check:
> >  	/* Finally, check access permissions. */
> > -	error = nfsd_permission(&rqstp->rq_cred, exp, dentry, access);
> > +	error = nfsd_permission(cred, exp, dentry, access);
> >  out:
> >  	trace_nfsd_fh_verify_err(rqstp, fhp, type, access, error);
> >  	if (error == nfserr_stale)
> > @@ -400,6 +382,40 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> >  	return error;
> >  }
> >  
> > +/**
> > + * fh_verify - filehandle lookup and access checking
> > + * @rqstp: pointer to current rpc request
> > + * @fhp: filehandle to be verified
> > + * @type: expected type of object pointed to by filehandle
> > + * @access: type of access needed to object
> > + *
> > + * Look up a dentry from the on-the-wire filehandle, check the client's
> > + * access to the export, and set the current task's credentials.
> > + *
> > + * Regardless of success or failure of fh_verify(), fh_put() should be
> > + * called on @fhp when the caller is finished with the filehandle.
> > + *
> > + * fh_verify() may be called multiple times on a given filehandle, for
> > + * example, when processing an NFSv4 compound.  The first call will look
> > + * up a dentry using the on-the-wire filehandle.  Subsequent calls will
> > + * skip the lookup and just perform the other checks and possibly change
> > + * the current task's credentials.
> > + *
> > + * @type specifies the type of object expected using one of the S_IF*
> > + * constants defined in include/linux/stat.h.  The caller may use zero
> > + * to indicate that it doesn't care, or a negative integer to indicate
> > + * that it expects something not of the given type.
> > + *
> > + * @access is formed from the NFSD_MAY_* constants defined in
> > + * fs/nfsd/vfs.h.
> > + */
> > +__be32
> > +fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> > +{
> > +	return __fh_verify(rqstp, SVC_NET(rqstp), &rqstp->rq_cred,
> > +			   rqstp->rq_client, rqstp->rq_gssclient,
> > +			   fhp, type, access);
> > +}
> >  
> >  /*
> >   * Compose a file handle for an NFS reply.
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>