Re: [PATCH 3/3] nfsd: move error choice for incorrect object types to version-specific code.

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2024-08-09 at 08:00 +1000, NeilBrown wrote:
> 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@suse.de>
> > > > > ---
> > > > >  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)
> 

This was a v4.0 pynfs run, so yes it appears this was leaking out
somewhere.

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

Sounds great!

> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>