Re: [PATCH 3/3] nfsd: move error choice for incorrect object types to version-specific code.

[Jeff Layton <jlayton@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7036 bytes --]

On Mon, 2024-07-29 at 11:47 +1000, NeilBrown wrote:
> If an NFS operation expect a particular sort of object (file, dir, link,
> etc) but gets a file handle for a different sort of object, it must
> return an error.  The actual error varies among version in no-trivial
> ways.
> 
> For v2 and v3 there are ISDIR and NOTDIR errors, and for any else, only
> INVAL is suitable.
> 
> For v4.0 there is also NFS4ERR_SYMLINK which should be used if a SYMLINK
> was found when not expected.  This take precedence over NOTDIR.
> 
> For v4.1+ there is also NFS4ERR_WRONG_TYPE which should be used in
> preference to EINVAL when none of the specific error codes apply.
> 
> When nfsd_mode_check() finds a symlink where it expect a directory it
> needs to return an error code that can be converted to NOTDIR for v2 or
> v3 but will be SYMLINK for v4.  It must be different from the error
> code returns when it finds a symlink but expects a regular file - that
> must be converted to EINVAL or SYMLINK.
> 
> So we introduce an internal error code nfserr_symlink_not_dir which each
> version converts as appropriate.
> 
> We also allow nfserr_wrong_type to be returned by
> nfsd_check_obj_is_reg() in nfsv4 code) and nfsd_mode_check().  This a
> behavioural change as nfsd_check_obj_is_reg() would previously return
> nfserr_symiink for non-directory objects that aren't regular files.  Now
> it will return nfserr_wrong_type for objects that aren't regular,
> directory, symlink (so char-special, block-special, sockets), which is
> mapped to nfserr_inval for NFSv4.0.  This should not be a problem as the
> behaviour is supported by RFCs.
> 
> As a result of these changes, nfsd_mode_check() doesn't need an rqstp
> arg any more.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs3proc.c |  8 ++++++++
>  fs/nfsd/nfs4proc.c | 24 ++++++++++++++++--------
>  fs/nfsd/nfsd.h     |  5 +++++
>  fs/nfsd/nfsfh.c    | 16 +++++++---------
>  fs/nfsd/nfsproc.c  |  7 +++++++
>  5 files changed, 43 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 31bd9bcf8687..ac7ee24415a3 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -38,6 +38,14 @@ static __be32 map_status(__be32 status)
>  	case nfserr_file_open:
>  		status = nfserr_acces;
>  		break;
> +
> +	case nfserr_symlink_not_dir:
> +		status = nfserr_notdir;
> +		break;
> +	case nfserr_symlink:
> +	case nfserr_wrong_type:
> +		status = nfserr_inval;
> +		break;
>  	}
>  	return status;
>  }
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 46bd20fe5c0f..cc715438e77a 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -166,14 +166,9 @@ static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
>  		return nfs_ok;
>  	if (S_ISDIR(mode))
>  		return nfserr_isdir;
> -	/*
> -	 * Using err_symlink as our catch-all case may look odd; but
> -	 * there's no other obvious error for this case in 4.0, and we
> -	 * happen to know that it will cause the linux v4 client to do
> -	 * the right thing on attempts to open something other than a
> -	 * regular file.
> -	 */
> -	return nfserr_symlink;
> +	if (S_ISLNK(mode))
> +		return nfserr_symlink;
> +	return nfserr_wrong_type;
>  }
>  
>  static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh *resfh)
> @@ -184,6 +179,17 @@ static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate
>  			&resfh->fh_handle);
>  }
>  
> +static __be32 map_status(__be32 status, int minor)
> +{
> +	if (status == nfserr_wrong_type &&
> +	    minor == 0)
> +		/* RFC5661 - 15.1.2.9 */
> +		status = nfserr_inval;
> +
> +	if (status == nfserr_symlink_not_dir)
> +		status = nfserr_symlink;
> +	return status;
> +}
>  static inline bool nfsd4_create_is_exclusive(int createmode)
>  {
>  	return createmode == NFS4_CREATE_EXCLUSIVE ||
> @@ -2798,6 +2804,8 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  			nfsd4_encode_replay(resp->xdr, op);
>  			status = op->status = op->replay->rp_status;
>  		} else {
> +			op->status = map_status(op->status,
> +						cstate->minorversion);
>  			nfsd4_encode_operation(resp, op);
>  			status = op->status;
>  		}
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 593c34fd325a..3c8c8da063b0 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -349,6 +349,11 @@ enum {
>  	NFSERR_REPLAY_CACHE,
>  #define	nfserr_replay_cache	cpu_to_be32(NFSERR_REPLAY_CACHE)
>  
> +/* symlink found where dir expected - handled differently to
> + * other symlink found errors by NFSv3.
> + */
> +	NFSERR_SYMLINK_NOT_DIR,
> +#define	nfserr_symlink_not_dir	cpu_to_be32(NFSERR_SYMLINK_NOT_DIR)
>  };
>  
>  /* Check for dir entries '.' and '..' */
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 0130103833e5..8cd70f93827c 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -62,8 +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,
> -		umode_t requested)
> +nfsd_mode_check(struct dentry *dentry, umode_t requested)
>  {
>  	umode_t mode = d_inode(dentry)->i_mode & S_IFMT;
>  
> @@ -76,17 +75,16 @@ nfsd_mode_check(struct svc_rqst *rqstp, struct dentry *dentry,
>  		}
>  		return nfs_ok;
>  	}
> -	/*
> -	 * 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 (mode == S_IFLNK) {
> +		if (requested == S_IFDIR)
> +			return nfserr_symlink_not_dir;
>  		return nfserr_symlink;
> +	}
>  	if (requested == S_IFDIR)
>  		return nfserr_notdir;
>  	if (mode == S_IFDIR)
>  		return nfserr_isdir;
> -	return nfserr_inval;
> +	return nfserr_wrong_type;
>  }
>  
>  static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, int flags)
> @@ -362,7 +360,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
>  	if (error)
>  		goto out;
>  
> -	error = nfsd_mode_check(rqstp, dentry, type);
> +	error = nfsd_mode_check(dentry, type);
>  	if (error)
>  		goto out;
>  
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index cb7099c6dc78..3d65ab558091 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -25,6 +25,13 @@ static __be32 map_status(__be32 status)
>  	case nfserr_file_open:
>  		status = nfserr_acces;
>  		break;
> +	case nfserr_symlink_not_dir:
> +		status = nfserr_notdir;
> +		break;
> +	case nfserr_symlink:
> +	case nfserr_wrong_type:
> +		status = nfserr_inval;
> +		break;
>  	}
>  	return status;
>  }

Hi Neil,

I'm seeing a set of failures in pynfs with this patch (json results
attached). I haven't looked in detail yet, but we should probably drop
this one for now.
-- 
Jeff Layton <jlayton@kernel.org>

[-- Attachment #2: 6.11.0-rc2-g1c134bcd2ef0-v4.0.json --]
[-- Type: application/json, Size: 122987 bytes --]