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

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

On Mon, Aug 12, 2024 at 02:06:25PM +1000, NeilBrown wrote:
> 
> [This should be squashed into the existing patch for the same name,
> with this commit message used instead of the current one.  It addresses
> the pynfs failures that Jeff found]
> 
> If an NFS operation expects 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 NFS version in non-trivial
> ways.
> 
> For v2 and v3 there are ISDIR and NOTDIR errors and, for NFSv4 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 expected 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.
> 
> nfsd_check_obj_isreg() is similar to nfsd_mode_check() except that it is
> only used by NFSv4 and only for OPEN.  NFSERR_INVAL is never a suitable
> error if the object is the wrong time.  For v4.0 we use nfserr_symlink
> for non-dirs even if not a symlink.  For v4.1 we have nfserr_wrong_type.
> To handle this difference we introduce an internal status code
> nfserr_wrong_type_open.
> 
> As a result of these changes, nfsd_mode_check() doesn't need an rqstp
> arg any more.
> 
> Note that NFSv4 operations are actually performed in the xdr code(!!!)
> so to the only place that we can map the status code successfully is in
> nfsd4_encode_operation().

Thanks for noting the RFC sections in the comments, that will be
very helpful for us forgetful old farts.

nfsd_check_obj_isreg's only caller is do_open_lookup, and the minor
version number is available there, via nfsd4_compound_state. If the
minor version is passed to nfsd_check_obj_isreg, it can correctly
choose to return nfserr_symlink or nfserr_wrong_type, yes? That
would eliminate the need for nfserr_wrong_type_open, perhaps.

What were the other error code leaks you found?


> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4proc.c | 21 +--------------------
>  fs/nfsd/nfs4xdr.c  | 26 ++++++++++++++++++++++++++
>  fs/nfsd/nfsd.h     |  6 ++++++
>  3 files changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 2e355085e443..e010d627d545 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -168,7 +168,7 @@ static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
>  		return nfserr_isdir;
>  	if (S_ISLNK(mode))
>  		return nfserr_symlink;
> -	return nfserr_wrong_type;
> +	return nfserr_wrong_type_open;
>  }
>  
>  static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh *resfh)
> @@ -179,23 +179,6 @@ static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate
>  			&resfh->fh_handle);
>  }
>  
> -static __be32 nfsd4_map_status(__be32 status, u32 minor)
> -{
> -	switch (status) {
> -	case nfs_ok:
> -		break;
> -	case nfserr_wrong_type:
> -		/* RFC 8881 - 15.1.2.9 */
> -		if (minor == 0)
> -			status = nfserr_inval;
> -		break;
> -	case nfserr_symlink_not_dir:
> -		status = nfserr_symlink;
> -		break;
> -	}
> -	return status;
> -}
> -
>  static inline bool nfsd4_create_is_exclusive(int createmode)
>  {
>  	return createmode == NFS4_CREATE_EXCLUSIVE ||
> @@ -2815,8 +2798,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  			nfsd4_encode_replay(resp->xdr, op);
>  			status = op->status = op->replay->rp_status;
>  		} else {
> -			op->status = nfsd4_map_status(op->status,
> -						      cstate->minorversion);
>  			nfsd4_encode_operation(resp, op);
>  			status = op->status;
>  		}
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 42b41d55d4ed..a0c1fc19aea4 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5729,6 +5729,30 @@ __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *resp, u32 respsize)
>  	return nfserr_rep_too_big;
>  }
>  
> +static __be32 nfsd4_map_status(__be32 status, u32 minor)
> +{
> +	switch (status) {
> +	case nfs_ok:
> +		break;
> +	case nfserr_wrong_type:
> +		/* RFC 8881 - 15.1.2.9 */
> +		if (minor == 0)
> +			status = nfserr_inval;
> +		break;
> +	case nfserr_wrong_type_open:
> +		/* RFC 7530 - 16.16.6 */
> +		if (minor == 0)
> +			status = nfserr_symlink;
> +		else
> +			status = nfserr_wrong_type;
> +		break;
> +	case nfserr_symlink_not_dir:
> +		status = nfserr_symlink;
> +		break;
> +	}
> +	return status;
> +}
> +
>  void
>  nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  {
> @@ -5796,6 +5820,8 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  						so->so_replay.rp_buf, len);
>  	}
>  status:
> +	op->status = nfsd4_map_status(op->status,
> +				      resp->cstate.minorversion);
>  	*p = op->status;
>  release:
>  	if (opdesc && opdesc->op_release)
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 4ccbf014a2c7..b4503e698ffd 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -359,6 +359,12 @@ enum {
>   */
>  	NFSERR_SYMLINK_NOT_DIR,
>  #define	nfserr_symlink_not_dir	cpu_to_be32(NFSERR_SYMLINK_NOT_DIR)
> +
> +/* non-{reg,dir,symlink} found by open - handled differently
> + * in v4.0 than v4.1.
> + */
> +	NFSERR_WRONG_TYPE_OPEN,
> +#define	nfserr_wrong_type_open	cpu_to_be32(NFSERR_WRONG_TYPE_OPEN)
>  };
>  
>  /* Check for dir entries '.' and '..' */
> -- 
> 2.44.0
> 

-- 
Chuck Lever