Re: [PATCH 3/6] nfsd: pass nfs_vers explicitly to __fh_verify()

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

On Mon, Jul 01, 2024 at 12:53:18PM +1000, NeilBrown wrote:
> Rather then depending on rqstp->rq_vers to determine nfs version, pass
> it in explicitly.  This removes another dependency on rqstp and ensures
> the correct version is checked.  The rqstp can be for an NLM request and
> while some code tests that, other code does not.

This is my only other major quibble with this series, which
otherwise looks like it will shape up to be a nice set of clean-ups.

I'd rather avoid having program- and version-specific logic in these
utilities. It makes it a little more difficult for us to shave out
support for older NFS versions using Kconfig options, for example.

Have you thought of any alternatives to passing an "RPC version"
argument? I will also ponder.


> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfsfh.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 760684fa4b50..adc731bb171e 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -62,7 +62,7 @@ static int nfsd_acceptable(void *expv, struct dentry *dentry)
>   * the write call).
>   */
>  static inline __be32
> -nfsd_mode_check(struct svc_rqst *rqstp, struct dentry *dentry,
> +nfsd_mode_check(int nfs_vers, struct dentry *dentry,
>  		umode_t requested)
>  {
>  	umode_t mode = d_inode(dentry)->i_mode & S_IFMT;
> @@ -80,7 +80,7 @@ nfsd_mode_check(struct svc_rqst *rqstp, struct dentry *dentry,
>  	 * v4 has an error more specific than err_notdir which we should
>  	 * return in preference to err_notdir:
>  	 */
> -	if (rqstp->rq_vers == 4 && mode == S_IFLNK)
> +	if (nfs_vers == 4 && mode == S_IFLNK)
>  		return nfserr_symlink;
>  	if (requested == S_IFDIR)
>  		return nfserr_notdir;
> @@ -117,8 +117,9 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
>  	return nfserrno(nfsd_setuser(cred, exp));
>  }
>  
> -static inline __be32 check_pseudo_root(struct svc_rqst *rqstp,
> -	struct dentry *dentry, struct svc_export *exp)
> +static inline __be32 check_pseudo_root(int nfs_vers,
> +				       struct dentry *dentry,
> +				       struct svc_export *exp)
>  {
>  	if (!(exp->ex_flags & NFSEXP_V4ROOT))
>  		return nfs_ok;
> @@ -128,7 +129,7 @@ static inline __be32 check_pseudo_root(struct svc_rqst *rqstp,
>  	 * in v4-specific code, in which case v2/v3 clients could bypass
>  	 * them.
>  	 */
> -	if (!nfsd_v4client(rqstp))
> +	if (nfs_vers != 4)
>  		return nfserr_stale;
>  	/*
>  	 * We're exposing only the directories and symlinks that have to be
> @@ -153,7 +154,7 @@ static inline __be32 check_pseudo_root(struct svc_rqst *rqstp,
>   * fh_dentry.
>   */
>  static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn,
> -				 struct svc_cred *cred,
> +				 struct svc_cred *cred, int nfs_vers,
>  				 struct svc_fh *fhp)
>  {
>  	struct knfsd_fh	*fh = &fhp->fh_handle;
> @@ -166,9 +167,9 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn,
>  	__be32 error;
>  
>  	error = nfserr_stale;
> -	if (rqstp->rq_vers > 2)
> +	if (nfs_vers > 2)
>  		error = nfserr_badhandle;
> -	if (rqstp->rq_vers == 4 && fh->fh_size == 0)
> +	if (nfs_vers == 4 && fh->fh_size == 0)
>  		return nfserr_nofilehandle;
>  
>  	if (fh->fh_version != 1)
> @@ -241,7 +242,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn,
>  	 * Look up the dentry using the NFS file handle.
>  	 */
>  	error = nfserr_stale;
> -	if (rqstp->rq_vers > 2)
> +	if (nfs_vers > 2)
>  		error = nfserr_badhandle;
>  
>  	fileid_type = fh->fh_fileid_type;
> @@ -281,7 +282,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn,
>  	fhp->fh_dentry = dentry;
>  	fhp->fh_export = exp;
>  
> -	switch (rqstp->rq_vers) {
> +	switch (nfs_vers) {
>  	case 4:
>  		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR)
>  			fhp->fh_no_atomic_attr = true;
> @@ -330,6 +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,
>  	    struct svc_fh *fhp, umode_t type, int access)
>  {
>  	struct svc_export *exp = NULL;
> @@ -337,7 +339,7 @@ __fh_verify(struct svc_rqst *rqstp,
>  	__be32		error;
>  
>  	if (!fhp->fh_dentry) {
> -		error = nfsd_set_fh_dentry(rqstp, nn, cred, fhp);
> +		error = nfsd_set_fh_dentry(rqstp, nn, cred, nfs_vers, fhp);
>  		if (error)
>  			goto out;
>  	}
> @@ -362,7 +364,7 @@ __fh_verify(struct svc_rqst *rqstp,
>  	 *	  (for example, if different id-squashing options are in
>  	 *	  effect on the new filesystem).
>  	 */
> -	error = check_pseudo_root(rqstp, dentry, exp);
> +	error = check_pseudo_root(nfs_vers, dentry, exp);
>  	if (error)
>  		goto out;
>  
> @@ -370,7 +372,7 @@ __fh_verify(struct svc_rqst *rqstp,
>  	if (error)
>  		goto out;
>  
> -	error = nfsd_mode_check(rqstp, dentry, type);
> +	error = nfsd_mode_check(nfs_vers, dentry, type);
>  	if (error)
>  		goto out;
>  
> @@ -407,12 +409,16 @@ __fh_verify(struct svc_rqst *rqstp,
>  __be32
>  fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
>  {
> +	int nfs_vers;
> +	if (rqstp->rq_prog == NFS_PROGRAM)
> +		nfs_vers = rqstp->rq_vers;
> +	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,
> +			   &rqstp->rq_cred, nfs_vers,
>  			   fhp, type, access);
>  }
>  
> -
>  /*
>   * Compose a file handle for an NFS reply.
>   *
> -- 
> 2.44.0
> 

-- 
Chuck Lever