On Thu, 08 Aug 2024, Jeff Layton wrote:
> On Thu, 2024-08-08 at 21:40 +1000, NeilBrown wrote:
> > On Thu, 08 Aug 2024, Jeff Layton wrote:
> > > 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.
> >
> > Most of the complaints are because pynfs is expecting the incorrect
> > error code that nfsd currently returns, rather than the correct one that
> > my patch makes it return.
> >
> > There is one where the internal error code of 10101 leaks out. That is
> > NFSERR_SYMLNK_NOT_DIR.
> > That certainly requires investigation.
> >
>
> I'm not sure these error code changes are correct, now that I look. For
> instance:
>
> {
> "classname": "st_readlink",
> "code": "RDLK2r",
> "failure": {
> "err": "Traceback (most recent call last):\n File \"/data/pynfs/nfs4.0/lib/testmod.py\", line 234, in run\n self.runtest(self, environment)\n File \"/data/pynfs/nfs4.0/servertests/st_readlink.py\", line 29, in testFile\n check(res, NFS4ERR_INVAL, \"READLINK on non-symlink objects\")\n File \"/data/pynfs/nfs4.0/servertests/environment.py\", line 270, in check\n raise testmod.FailureException(msg)\ntestmod.FailureException: READLINK on non-symlink objects should return NFS4ERR_INVAL, instead got NFS4ERR_WRONG_TYPE\n",
> "message": "READLINK on non-symlink objects should return NFS4ERR_INVAL, instead got NFS4ERR_WRONG_TYPE"
> },
> "name": "testFile",
> "time": "0.003440380096435547"
> },
>
>
> Looking at RFC7530, the READLINK section (16.25.5) says:
>
> The READLINK operation is only allowed on objects of type NF4LNK.
> The server should return the error NFS4ERR_INVAL if the object is not
> of type NF4LNK.
RFC7530 is for NFSv4.0 It doesn't have NFS4ERR_WRONG_TYPE. So if this
is a 4.0 test then that is indeed a bug. If the "nfs4.0" in the File
path the only hit that this was 4.0 rather than 4.1?
(RFC8881 declares this should be NFS4ERR_WRONG_TYPE for 4.1)
>
> Several OPEN cases have errors similar to this one:
>
> {
> "classname": "st_open",
> "code": "OPEN7s",
> "failure": {
> "err": "Traceback (most recent call last):\n File \"/data/pynfs/nfs4.0/lib/testmod.py\", line 234, in run\n self.runtest(self, environment)\n File \"/data/pynfs/nfs4.0/servertests/st_open.py\", line 183, in testSocket\n check(res, NFS4ERR_SYMLINK, \"Trying to OPEN socket\")\n File \"/data/pynfs/nfs4.0/servertests/environment.py\", line 270, in check\n raise testmod.FailureException(msg)\ntestmod.FailureException: Trying to OPEN socket should return NFS4ERR_SYMLINK, instead got NFS4ERR_INVAL\n",
> "message": "Trying to OPEN socket should return NFS4ERR_SYMLINK, instead got NFS4ERR_INVAL"
> },
> "name": "testSocket",
> "time": "0.006421566009521484"
> },
>
> RFC7530, 16.16.6:
>
> If the component provided to OPEN resolves to something other than a
> regular file (or a named attribute), an error will be returned to the
> client. If it is a directory, NFS4ERR_ISDIR is returned; otherwise,
> NFS4ERR_SYMLINK is returned. Note that NFS4ERR_SYMLINK is returned
> for both symlinks and for special files of other types; NFS4ERR_INVAL
> would be inappropriate, since the arguments provided by the client
> were correct, and the client cannot necessarily know at the time it
> sent the OPEN that the component would resolve to a non-regular file.
Ahhh - thanks. The comments in the code seemed to suggest that the RFC
wasn't specific. I guess I misread them.
Thanks for the more careful analysis. I'll aim to have an update on
Monday.
NeilBrown
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
>
