diff mbox series

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

Message ID 172361193894.6062.4098495640528994632@noble.neil.brown.name (mailing list archive)
State New
Headers show
Series [v2] SQUASH: nfsd: move error choice for incorrect object types to version-specific code. | expand

Commit Message

NeilBrown Aug. 14, 2024, 5:05 a.m. UTC
[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.
We handling this difference in-place in nfsd_check_obj_isreg() as there
is nothing to be gained by delaying the choice to nfsd4_map_status().

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().

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4proc.c | 31 +++++++++----------------------
 fs/nfsd/nfs4xdr.c  | 19 +++++++++++++++++++
 2 files changed, 28 insertions(+), 22 deletions(-)

Comments

Jeff Layton Aug. 14, 2024, 11:52 a.m. UTC | #1
On Wed, 2024-08-14 at 15:05 +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.
> We handling this difference in-place in nfsd_check_obj_isreg() as there
> is nothing to be gained by delaying the choice to nfsd4_map_status().
> 
> 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().
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4proc.c | 31 +++++++++----------------------
>  fs/nfsd/nfs4xdr.c  | 19 +++++++++++++++++++
>  2 files changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 2e355085e443..fc68af757080 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -158,7 +158,7 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
>  	return fh_verify(rqstp, current_fh, S_IFREG, accmode);
>  }
>  
> -static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
> +static __be32 nfsd_check_obj_isreg(struct svc_fh *fh, u32 minor_version)
>  {
>  	umode_t mode = d_inode(fh->fh_dentry)->i_mode;
>  
> @@ -168,7 +168,13 @@ static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
>  		return nfserr_isdir;
>  	if (S_ISLNK(mode))
>  		return nfserr_symlink;
> -	return nfserr_wrong_type;
> +
> +	/* RFC 7530 - 16.16.6 */
> +	if (minor_version == 0)
> +		return nfserr_symlink;
> +	else
> +		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)
> @@ -179,23 +185,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 ||
> @@ -478,7 +467,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
>  	}
>  	if (status)
>  		goto out;
> -	status = nfsd_check_obj_isreg(*resfh);
> +	status = nfsd_check_obj_isreg(*resfh, cstate->minorversion);
>  	if (status)
>  		goto out;
>  
> @@ -2815,8 +2804,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..1c3067f46be7 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5729,6 +5729,23 @@ __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_symlink_not_dir:
> +		status = nfserr_symlink;
> +		break;
> +	}
> +	return status;
> +}
> +
>  void
>  nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  {
> @@ -5796,6 +5813,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)

LGTM

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Chuck Lever III Aug. 14, 2024, 1:31 p.m. UTC | #2
On Wed, Aug 14, 2024 at 03:05:38PM +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.
> We handling this difference in-place in nfsd_check_obj_isreg() as there
> is nothing to be gained by delaying the choice to nfsd4_map_status().
> 
> 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().
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4proc.c | 31 +++++++++----------------------
>  fs/nfsd/nfs4xdr.c  | 19 +++++++++++++++++++
>  2 files changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 2e355085e443..fc68af757080 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -158,7 +158,7 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
>  	return fh_verify(rqstp, current_fh, S_IFREG, accmode);
>  }
>  
> -static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
> +static __be32 nfsd_check_obj_isreg(struct svc_fh *fh, u32 minor_version)
>  {
>  	umode_t mode = d_inode(fh->fh_dentry)->i_mode;
>  
> @@ -168,7 +168,13 @@ static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
>  		return nfserr_isdir;
>  	if (S_ISLNK(mode))
>  		return nfserr_symlink;
> -	return nfserr_wrong_type;
> +
> +	/* RFC 7530 - 16.16.6 */
> +	if (minor_version == 0)
> +		return nfserr_symlink;
> +	else
> +		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)
> @@ -179,23 +185,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 ||
> @@ -478,7 +467,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
>  	}
>  	if (status)
>  		goto out;
> -	status = nfsd_check_obj_isreg(*resfh);
> +	status = nfsd_check_obj_isreg(*resfh, cstate->minorversion);
>  	if (status)
>  		goto out;
>  
> @@ -2815,8 +2804,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..1c3067f46be7 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5729,6 +5729,23 @@ __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_symlink_not_dir:
> +		status = nfserr_symlink;
> +		break;
> +	}
> +	return status;
> +}
> +
>  void
>  nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  {
> @@ -5796,6 +5813,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)
> -- 
> 2.44.0
> 

Applied, squashed, and pushed to nfsd-next for v6.12.
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 2e355085e443..fc68af757080 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -158,7 +158,7 @@  do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 	return fh_verify(rqstp, current_fh, S_IFREG, accmode);
 }
 
-static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
+static __be32 nfsd_check_obj_isreg(struct svc_fh *fh, u32 minor_version)
 {
 	umode_t mode = d_inode(fh->fh_dentry)->i_mode;
 
@@ -168,7 +168,13 @@  static __be32 nfsd_check_obj_isreg(struct svc_fh *fh)
 		return nfserr_isdir;
 	if (S_ISLNK(mode))
 		return nfserr_symlink;
-	return nfserr_wrong_type;
+
+	/* RFC 7530 - 16.16.6 */
+	if (minor_version == 0)
+		return nfserr_symlink;
+	else
+		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)
@@ -179,23 +185,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 ||
@@ -478,7 +467,7 @@  do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	}
 	if (status)
 		goto out;
-	status = nfsd_check_obj_isreg(*resfh);
+	status = nfsd_check_obj_isreg(*resfh, cstate->minorversion);
 	if (status)
 		goto out;
 
@@ -2815,8 +2804,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..1c3067f46be7 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -5729,6 +5729,23 @@  __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_symlink_not_dir:
+		status = nfserr_symlink;
+		break;
+	}
+	return status;
+}
+
 void
 nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
 {
@@ -5796,6 +5813,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)