Re: [PATCH 4/6] nfsd: pass client explicitly to __fh_verify()

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-07-01 at 12:53 +1000, NeilBrown wrote:
> Rather than using rqstp->rq_client pass the client explicitly to
> __fh_verify and thence to rqst_exp_find().  If rqst_exp_find is given
> an
> explicit client it doesn't try ->rq_gssclient.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/export.c   | 15 ++++++++++-----
>  fs/nfsd/export.h   |  2 +-
>  fs/nfsd/nfs4proc.c |  2 +-
>  fs/nfsd/nfsfh.c    | 11 ++++++-----
>  4 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index a35f06b610d0..ccfe8c528bcb 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1165,21 +1165,26 @@ rqst_exp_get_by_name(struct svc_rqst *rqstp,
> struct path *path)
>  }
>  

While you're in here, care to write a kerneldoc for rqst_exp_find? The
arguments to this are getting pretty complex, so it might help clarify
things.

>  struct svc_export *
> -rqst_exp_find(struct svc_rqst *rqstp,  struct nfsd_net *nn,
> +rqst_exp_find(struct svc_rqst *rqstp, struct nfsd_net *nn,
> +	      struct auth_domain *client,
>  	      int fsid_type, u32 *fsidv)
>  {
>  	struct svc_export *gssexp, *exp = ERR_PTR(-ENOENT);
>  	struct cache_detail *cd;
> +	bool try_gss = rqstp && !client;
>  
>  	if (!nn)
>  		nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>  	cd = nn->svc_export_cache;
>  
> -	if (rqstp->rq_client == NULL)
> +	if (!client && rqstp)
> +		client = rqstp->rq_client;
> +
> +	if (client == NULL)
>  		goto gss;
>  
>  	/* First try the auth_unix client: */
> -	exp = exp_find(cd, rqstp->rq_client, fsid_type,
> +	exp = exp_find(cd, client, fsid_type,
>  		       fsidv, &rqstp->rq_chandle);

The checks you've added make it appear like rqstp can be NULL, but
above you're still dereferencing it to get the ->rq_chandle. That seems
problematic?

>  	if (PTR_ERR(exp) == -ENOENT)
>  		goto gss;
> @@ -1190,7 +1195,7 @@ rqst_exp_find(struct svc_rqst *rqstp,  struct
> nfsd_net *nn,
>  		return exp;
>  gss:
>  	/* Otherwise, try falling back on gss client */
> -	if (rqstp->rq_gssclient == NULL)
> +	if (!try_gss || rqstp->rq_gssclient == NULL)
>  		return exp;
>  	gssexp = exp_find(cd, rqstp->rq_gssclient, fsid_type, fsidv,
>  						&rqstp->rq_chandle);
> @@ -1224,7 +1229,7 @@ struct svc_export
> *rqst_find_fsidzero_export(struct svc_rqst *rqstp)
>  
>  	mk_fsid(FSID_NUM, fsidv, 0, 0, 0, NULL);
>  
> -	return rqst_exp_find(rqstp, NULL, FSID_NUM, fsidv);
> +	return rqst_exp_find(rqstp, NULL, NULL, FSID_NUM, fsidv);
>  }
>  
>  /*
> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> index 2dbd15704a86..accad9d231fd 100644
> --- a/fs/nfsd/export.h
> +++ b/fs/nfsd/export.h
> @@ -130,6 +130,6 @@ static inline struct svc_export *exp_get(struct
> svc_export *exp)
>  }
>  struct nfsd_net;
>  struct svc_export * rqst_exp_find(struct svc_rqst *, struct nfsd_net
> *,
> -				  int, u32 *);
> +				  struct auth_domain *, int, u32 *);
>  
>  #endif /* NFSD_EXPORT_H */
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 30335cdf9e6c..8430c197c900 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2231,7 +2231,7 @@ nfsd4_getdeviceinfo(struct svc_rqst *rqstp,
>  		return nfserr_noent;
>  	}
>  
> -	exp = rqst_exp_find(rqstp, NULL, map->fsid_type, map->fsid);
> +	exp = rqst_exp_find(rqstp, NULL, NULL, map->fsid_type, map-
> >fsid);
>  	if (IS_ERR(exp)) {
>  		dprintk("%s: could not find device id\n", __func__);
>  		return nfserr_noent;
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index adc731bb171e..ea3d98c43a9d 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -155,7 +155,7 @@ static inline __be32 check_pseudo_root(int
> nfs_vers,
>   */
>  static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct
> nfsd_net *nn,
>  				 struct svc_cred *cred, int
> nfs_vers,
> -				 struct svc_fh *fhp)
> +				 struct auth_domain *client, struct
> svc_fh *fhp)
>  {
>  	struct knfsd_fh	*fh = &fhp->fh_handle;
>  	struct fid *fid = NULL;
> @@ -199,7 +199,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst
> *rqstp, struct nfsd_net *nn,
>  	data_left -= len;
>  	if (data_left < 0)
>  		return error;
> -	exp = rqst_exp_find(rqstp, nn, fh->fh_fsid_type, fh-
> >fh_fsid);
> +	exp = rqst_exp_find(rqstp, nn, client, fh->fh_fsid_type, fh-
> >fh_fsid);
>  	fid = (struct fid *)(fh->fh_fsid + len);
>  
>  	error = nfserr_stale;
> @@ -331,7 +331,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst
> *rqstp, struct nfsd_net *nn,
>  static __be32
>  __fh_verify(struct svc_rqst *rqstp,
>  	    struct nfsd_net *nn, struct svc_cred *cred,
> -	    int nfs_vers,
> +	    int nfs_vers, struct auth_domain *client,
>  	    struct svc_fh *fhp, umode_t type, int access)
>  {
>  	struct svc_export *exp = NULL;
> @@ -339,7 +339,8 @@ __fh_verify(struct svc_rqst *rqstp,
>  	__be32		error;
>  
>  	if (!fhp->fh_dentry) {
> -		error = nfsd_set_fh_dentry(rqstp, nn, cred,
> nfs_vers, fhp);
> +		error = nfsd_set_fh_dentry(rqstp, nn, cred,
> nfs_vers, client,
> +					   fhp);
>  		if (error)
>  			goto out;
>  	}
> @@ -415,7 +416,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh
> *fhp, umode_t type, int access)
>  	else /* must be NLM */
>  		nfs_vers = rqstp->rq_vers == 4 ? 3 : 2;
>  	return __fh_verify(rqstp, net_generic(SVC_NET(rqstp),
> nfsd_net_id),
> -			   &rqstp->rq_cred, nfs_vers,
> +			   &rqstp->rq_cred, nfs_vers, rqstp-
> >rq_client,
>  			   fhp, type, access);
>  }
>  

-- 
Jeff Layton <jlayton@kernel.org>