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