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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
Attachment:
6.11.0-rc2-g1c134bcd2ef0-v4.0.json
Description: application/json
