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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
