refactor stateid checking and file allocation

refactor stateid checking and file allocation

[Christoph Hellwig]

This series refactor various bits of code so that we always get
a proper file structure returned from nfs4_preprocess_stateid_op
if we ask for it.  Note that the first patch is a bugfixe that
should go into 4.1 and -stable.

[PATCH 1/6] nfsd: fix the check for confirmed openowner in nfs4_preprocess_stateid_op

[Christoph Hellwig]

If we find a non-confirmed openowner we jump to exit the function, but do
not set an error value.  Fix this by factoring out a helper to do the
check and properly set the error from nfsd4_validate_stateid.

Cc: stable@vger.kernel.org
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 9072964..66067a2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4498,10 +4498,17 @@ static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_s
 	return nfserr_old_stateid;
 }
 
+static __be32 nfsd4_check_openowner_confirmed(struct nfs4_ol_stateid *ols)
+{
+	if (ols->st_stateowner->so_is_open_owner &&
+	    !(openowner(ols->st_stateowner)->oo_flags & NFS4_OO_CONFIRMED))
+		return nfserr_bad_stateid;
+	return nfs_ok;
+}
+
 static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 {
 	struct nfs4_stid *s;
-	struct nfs4_ol_stateid *ols;
 	__be32 status = nfserr_bad_stateid;
 
 	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
@@ -4531,13 +4538,7 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 		break;
 	case NFS4_OPEN_STID:
 	case NFS4_LOCK_STID:
-		ols = openlockstateid(s);
-		if (ols->st_stateowner->so_is_open_owner
-	    			&& !(openowner(ols->st_stateowner)->oo_flags
-						& NFS4_OO_CONFIRMED))
-			status = nfserr_bad_stateid;
-		else
-			status = nfs_ok;
+		status = nfsd4_check_openowner_confirmed(openlockstateid(s));
 		break;
 	default:
 		printk("unknown stateid type %x\n", s->sc_type);
@@ -4629,8 +4630,8 @@ nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
 		status = nfs4_check_fh(current_fh, stp);
 		if (status)
 			goto out;
-		if (stp->st_stateowner->so_is_open_owner
-		    && !(openowner(stp->st_stateowner)->oo_flags & NFS4_OO_CONFIRMED))
+		status = nfsd4_check_openowner_confirmed(stp);
+		if (status)
 			goto out;
 		status = nfs4_check_openmode(stp, flags);
 		if (status)
-- 
1.9.1

Re: [PATCH 1/6] nfsd: fix the check for confirmed openowner in nfs4_preprocess_stateid_op

[J. Bruce Fields]

On Tue, Apr 28, 2015 at 03:41:15PM +0200, Christoph Hellwig wrote:
> If we find a non-confirmed openowner we jump to exit the function, but do
> not set an error value.  Fix this by factoring out a helper to do the
> check and properly set the error from nfsd4_validate_stateid.

Thanks.  I can't tell if the bug has any practical consequences--a
correct client should never hit this case as far as I can tell.

--b.

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfsd/nfs4state.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9072964..66067a2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4498,10 +4498,17 @@ static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_s
>  	return nfserr_old_stateid;
>  }
>  
> +static __be32 nfsd4_check_openowner_confirmed(struct nfs4_ol_stateid *ols)
> +{
> +	if (ols->st_stateowner->so_is_open_owner &&
> +	    !(openowner(ols->st_stateowner)->oo_flags & NFS4_OO_CONFIRMED))
> +		return nfserr_bad_stateid;
> +	return nfs_ok;
> +}
> +
>  static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  {
>  	struct nfs4_stid *s;
> -	struct nfs4_ol_stateid *ols;
>  	__be32 status = nfserr_bad_stateid;
>  
>  	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
> @@ -4531,13 +4538,7 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  		break;
>  	case NFS4_OPEN_STID:
>  	case NFS4_LOCK_STID:
> -		ols = openlockstateid(s);
> -		if (ols->st_stateowner->so_is_open_owner
> -	    			&& !(openowner(ols->st_stateowner)->oo_flags
> -						& NFS4_OO_CONFIRMED))
> -			status = nfserr_bad_stateid;
> -		else
> -			status = nfs_ok;
> +		status = nfsd4_check_openowner_confirmed(openlockstateid(s));
>  		break;
>  	default:
>  		printk("unknown stateid type %x\n", s->sc_type);
> @@ -4629,8 +4630,8 @@ nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
>  		status = nfs4_check_fh(current_fh, stp);
>  		if (status)
>  			goto out;
> -		if (stp->st_stateowner->so_is_open_owner
> -		    && !(openowner(stp->st_stateowner)->oo_flags & NFS4_OO_CONFIRMED))
> +		status = nfsd4_check_openowner_confirmed(stp);
> +		if (status)
>  			goto out;
>  		status = nfs4_check_openmode(stp, flags);
>  		if (status)
> -- 
> 1.9.1

[PATCH 2/6] nfsd: remove nfsd_close

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c |  2 +-
 fs/nfsd/vfs.c       | 19 +++++--------------
 fs/nfsd/vfs.h       |  1 -
 3 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 66067a2..39475c9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5506,7 +5506,7 @@ static __be32 nfsd_test_lock(struct svc_rqst *rqstp, struct svc_fh *fhp, struct
 	__be32 err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_READ, &file);
 	if (!err) {
 		err = nfserrno(vfs_test_lock(file, lock));
-		nfsd_close(file);
+		fput(file);
 	}
 	return err;
 }
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 84d770b..a30e799 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -744,7 +744,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
 
 	host_err = ima_file_check(file, may_flags, 0);
 	if (host_err) {
-		nfsd_close(file);
+		fput(file);
 		goto out_nfserr;
 	}
 
@@ -762,15 +762,6 @@ out:
 }
 
 /*
- * Close a file.
- */
-void
-nfsd_close(struct file *filp)
-{
-	fput(filp);
-}
-
-/*
  * Obtain the readahead parameters for the file
  * specified by (dev, ino).
  */
@@ -1040,7 +1031,7 @@ void nfsd_put_tmp_read_open(struct file *file, struct raparms *ra)
 		ra->p_count--;
 		spin_unlock(&rab->pb_lock);
 	}
-	nfsd_close(file);
+	fput(file);
 }
 
 /*
@@ -1093,7 +1084,7 @@ nfsd_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
 		if (cnt)
 			err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen,
 					     cnt, stablep);
-		nfsd_close(file);
+		fput(file);
 	}
 out:
 	return err;
@@ -1138,7 +1129,7 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			err = nfserr_notsupp;
 	}
 
-	nfsd_close(file);
+	fput(file);
 out:
 	return err;
 }
@@ -1977,7 +1968,7 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,
 	if (err == nfserr_eof || err == nfserr_toosmall)
 		err = nfs_ok; /* can still be found in ->err */
 out_close:
-	nfsd_close(file);
+	fput(file);
 out:
 	return err;
 }
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 2050cb0..17a5e0d 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -71,7 +71,6 @@ __be32		nfsd_commit(struct svc_rqst *, struct svc_fh *,
 #endif /* CONFIG_NFSD_V3 */
 __be32		nfsd_open(struct svc_rqst *, struct svc_fh *, umode_t,
 				int, struct file **);
-void		nfsd_close(struct file *);
 struct raparms;
 __be32		nfsd_get_tmp_read_open(struct svc_rqst *, struct svc_fh *,
 				struct file **, struct raparms **);
-- 
1.9.1

[PATCH 3/6] nfsd: refactor nfs4_preprocess_stateid_op

[Christoph Hellwig]

Split out two self contained helpers to make the function more readable.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 97 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 52 insertions(+), 45 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 39475c9..5070688 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4575,20 +4575,51 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 	return nfs_ok;
 }
 
+static struct file *
+nfs4_find_file(struct nfs4_stid *s, int flags)
+{
+	switch (s->sc_type) {
+	case NFS4_DELEG_STID:
+		if (WARN_ON_ONCE(!s->sc_file->fi_deleg_file))
+			return NULL;
+		return get_file(s->sc_file->fi_deleg_file);
+	case NFS4_OPEN_STID:
+	case NFS4_LOCK_STID:
+		if (flags & RD_STATE)
+			return find_readable_file(s->sc_file);
+		else
+			return find_writeable_file(s->sc_file);
+		break;
+	}
+
+	return NULL;
+}
+
+static __be32
+nfs4_check_olstateid(struct svc_fh *fhp, struct nfs4_ol_stateid *ols, int flags)
+{
+	__be32 status;
+
+	status = nfs4_check_fh(fhp, ols);
+	if (status)
+		return status;
+	status = nfsd4_check_openowner_confirmed(ols);
+	if (status)
+		return status;
+	return nfs4_check_openmode(ols, flags);
+}
+
 /*
-* Checks for stateid operations
-*/
+ * Checks for stateid operations
+ */
 __be32
 nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
 			   stateid_t *stateid, int flags, struct file **filpp)
 {
-	struct nfs4_stid *s;
-	struct nfs4_ol_stateid *stp = NULL;
-	struct nfs4_delegation *dp = NULL;
-	struct svc_fh *current_fh = &cstate->current_fh;
-	struct inode *ino = d_inode(current_fh->fh_dentry);
+	struct svc_fh *fhp = &cstate->current_fh;
+	struct inode *ino = d_inode(fhp->fh_dentry);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
-	struct file *file = NULL;
+	struct nfs4_stid *s;
 	__be32 status;
 
 	if (filpp)
@@ -4598,60 +4629,36 @@ nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
 		return nfserr_grace;
 
 	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
-		return check_special_stateids(net, current_fh, stateid, flags);
+		return check_special_stateids(net, fhp, stateid, flags);
 
 	status = nfsd4_lookup_stateid(cstate, stateid,
 				NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
 				&s, nn);
 	if (status)
 		return status;
-	status = check_stateid_generation(stateid, &s->sc_stateid, nfsd4_has_session(cstate));
+	status = check_stateid_generation(stateid, &s->sc_stateid,
+			nfsd4_has_session(cstate));
 	if (status)
 		goto out;
+
 	switch (s->sc_type) {
 	case NFS4_DELEG_STID:
-		dp = delegstateid(s);
-		status = nfs4_check_delegmode(dp, flags);
-		if (status)
-			goto out;
-		if (filpp) {
-			file = dp->dl_stid.sc_file->fi_deleg_file;
-			if (!file) {
-				WARN_ON_ONCE(1);
-				status = nfserr_serverfault;
-				goto out;
-			}
-			get_file(file);
-		}
+		status = nfs4_check_delegmode(delegstateid(s), flags);
 		break;
 	case NFS4_OPEN_STID:
 	case NFS4_LOCK_STID:
-		stp = openlockstateid(s);
-		status = nfs4_check_fh(current_fh, stp);
-		if (status)
-			goto out;
-		status = nfsd4_check_openowner_confirmed(stp);
-		if (status)
-			goto out;
-		status = nfs4_check_openmode(stp, flags);
-		if (status)
-			goto out;
-		if (filpp) {
-			struct nfs4_file *fp = stp->st_stid.sc_file;
-
-			if (flags & RD_STATE)
-				file = find_readable_file(fp);
-			else
-				file = find_writeable_file(fp);
-		}
+		status = nfs4_check_olstateid(fhp, openlockstateid(s), flags);
 		break;
 	default:
 		status = nfserr_bad_stateid;
-		goto out;
+		break;
+	}
+
+	if (!status && filpp) {
+		*filpp = nfs4_find_file(s, flags);
+		if (!*filpp)
+			status = nfserr_serverfault;
 	}
-	status = nfs_ok;
-	if (file)
-		*filpp = file;
 out:
 	nfs4_put_stid(s);
 	return status;
-- 
1.9.1

[PATCH 4/6] nfsd: clean up raparams handling

[Christoph Hellwig]

Refactor the raparam hash helpers to just deal with the raparms,
and keep opening/closing files separate from that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4xdr.c | 24 +++++++++++++-------
 fs/nfsd/vfs.c     | 68 ++++++++++++++++++++-----------------------------------
 fs/nfsd/vfs.h     |  6 ++---
 3 files changed, 43 insertions(+), 55 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 158badf..54aced2 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -33,6 +33,7 @@
  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <linux/file.h>
 #include <linux/slab.h>
 #include <linux/namei.h>
 #include <linux/statfs.h>
@@ -3424,7 +3425,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
 	struct file *file = read->rd_filp;
 	struct svc_fh *fhp = read->rd_fhp;
 	int starting_len = xdr->buf->len;
-	struct raparms *ra;
+	struct raparms *ra = NULL;
 	__be32 *p;
 	__be32 err;
 
@@ -3446,23 +3447,30 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
 	maxcount = min_t(unsigned long, maxcount, (xdr->buf->buflen - xdr->buf->len));
 	maxcount = min_t(unsigned long, maxcount, read->rd_length);
 
-	if (read->rd_filp)
+	if (read->rd_filp) {
 		err = nfsd_permission(resp->rqstp, fhp->fh_export,
 				fhp->fh_dentry,
 				NFSD_MAY_READ|NFSD_MAY_OWNER_OVERRIDE);
-	else
-		err = nfsd_get_tmp_read_open(resp->rqstp, read->rd_fhp,
-						&file, &ra);
-	if (err)
-		goto err_truncate;
+		if (err)
+			goto err_truncate;
+	} else {
+		err = nfsd_open(resp->rqstp, fhp, S_IFREG, NFSD_MAY_READ,
+				&file);
+		if (err)
+			goto err_truncate;
+
+		ra = nfsd_init_raparms(file);
+	}
 
 	if (file->f_op->splice_read && test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags))
 		err = nfsd4_encode_splice_read(resp, read, file, maxcount);
 	else
 		err = nfsd4_encode_readv(resp, read, file, maxcount);
 
+	if (ra)
+		nfsd_put_raparams(file, ra);
 	if (!read->rd_filp)
-		nfsd_put_tmp_read_open(file, ra);
+		fput(file);
 
 err_truncate:
 	if (err)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index a30e799..9798c2cd 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -761,14 +761,12 @@ out:
 	return err;
 }
 
-/*
- * Obtain the readahead parameters for the file
- * specified by (dev, ino).
- */
-
-static inline struct raparms *
-nfsd_get_raparms(dev_t dev, ino_t ino)
+struct raparms *
+nfsd_init_raparms(struct file *file)
 {
+	struct inode *inode = file_inode(file);
+	dev_t dev = inode->i_sb->s_dev;
+	ino_t ino = inode->i_ino;
 	struct raparms	*ra, **rap, **frap = NULL;
 	int depth = 0;
 	unsigned int hash;
@@ -805,9 +803,23 @@ found:
 	ra->p_count++;
 	nfsdstats.ra_depth[depth*10/nfsdstats.ra_size]++;
 	spin_unlock(&rab->pb_lock);
+
+	if (ra->p_set)
+		file->f_ra = ra->p_ra;
 	return ra;
 }
 
+void nfsd_put_raparams(struct file *file, struct raparms *ra)
+{
+	struct raparm_hbucket *rab = &raparm_hash[ra->p_hindex];
+
+	spin_lock(&rab->pb_lock);
+	ra->p_ra = file->f_ra;
+	ra->p_set = 1;
+	ra->p_count--;
+	spin_unlock(&rab->pb_lock);
+}
+
 /*
  * Grab and keep cached pages associated with a file in the svc_rqst
  * so that they can be passed to the network sendmsg/sendpage routines
@@ -1000,40 +1012,6 @@ out_nfserr:
 	return err;
 }
 
-__be32 nfsd_get_tmp_read_open(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		struct file **file, struct raparms **ra)
-{
-	struct inode *inode;
-	__be32 err;
-
-	err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_READ, file);
-	if (err)
-		return err;
-
-	inode = file_inode(*file);
-
-	/* Get readahead parameters */
-	*ra = nfsd_get_raparms(inode->i_sb->s_dev, inode->i_ino);
-
-	if (*ra && (*ra)->p_set)
-		(*file)->f_ra = (*ra)->p_ra;
-	return nfs_ok;
-}
-
-void nfsd_put_tmp_read_open(struct file *file, struct raparms *ra)
-{
-	/* Write back readahead params */
-	if (ra) {
-		struct raparm_hbucket *rab = &raparm_hash[ra->p_hindex];
-		spin_lock(&rab->pb_lock);
-		ra->p_ra = file->f_ra;
-		ra->p_set = 1;
-		ra->p_count--;
-		spin_unlock(&rab->pb_lock);
-	}
-	fput(file);
-}
-
 /*
  * Read data from a file. count must contain the requested read count
  * on entry. On return, *count contains the number of bytes actually read.
@@ -1046,13 +1024,15 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	struct raparms	*ra;
 	__be32 err;
 
-	err = nfsd_get_tmp_read_open(rqstp, fhp, &file, &ra);
+	err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_READ, &file);
 	if (err)
 		return err;
 
+	ra = nfsd_init_raparms(file);
 	err = nfsd_vfs_read(rqstp, file, offset, vec, vlen, count);
-
-	nfsd_put_tmp_read_open(file, ra);
+	if (ra)
+		nfsd_put_raparams(file, ra);
+	fput(file);
 
 	return err;
 }
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 17a5e0d..053c4ad 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -72,9 +72,6 @@ __be32		nfsd_commit(struct svc_rqst *, struct svc_fh *,
 __be32		nfsd_open(struct svc_rqst *, struct svc_fh *, umode_t,
 				int, struct file **);
 struct raparms;
-__be32		nfsd_get_tmp_read_open(struct svc_rqst *, struct svc_fh *,
-				struct file **, struct raparms **);
-void		nfsd_put_tmp_read_open(struct file *, struct raparms *);
 __be32		nfsd_splice_read(struct svc_rqst *,
 				struct file *, loff_t, unsigned long *);
 __be32		nfsd_readv(struct file *, loff_t, struct kvec *, int,
@@ -103,6 +100,9 @@ __be32		nfsd_statfs(struct svc_rqst *, struct svc_fh *,
 __be32		nfsd_permission(struct svc_rqst *, struct svc_export *,
 				struct dentry *, int);
 
+struct raparms *nfsd_init_raparms(struct file *file);
+void		nfsd_put_raparams(struct file *file, struct raparms *ra);
+
 static inline int fh_want_write(struct svc_fh *fh)
 {
 	int ret = mnt_want_write(fh->fh_export->ex_path.mnt);
-- 
1.9.1

[PATCH 5/6] nfsd: take struct file setup fully into nfs4_preprocess_stateid_op

[Christoph Hellwig]

This patch changes nfs4_preprocess_stateid_op so it always returns
a valid struct file if it has been asked for that.  For that we
now allocate a temporary struct file for special stateids, and check
permissions if we got the file structure from the stateid.  This
ensures that all callers will get their handling of special stateids
right, and avoids code duplication.

There is a little wart in here because the read code needs to know
if we allocated a file structure so that it can copy around the
read-ahead parameters.  In the long run we should probably aim to
cache full file structures used with special stateids instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4proc.c  | 39 +++++++++++++++------------------------
 fs/nfsd/nfs4state.c | 35 ++++++++++++++++++++++++++++++-----
 fs/nfsd/nfs4xdr.c   | 19 +------------------
 fs/nfsd/state.h     |  6 +++---
 fs/nfsd/vfs.c       |  7 +------
 fs/nfsd/vfs.h       |  4 ++++
 fs/nfsd/xdr4.h      |  1 +
 7 files changed, 55 insertions(+), 56 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 864e200..5aa7c4e 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -760,8 +760,6 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 {
 	__be32 status;
 
-	/* no need to check permission - this will be done in nfsd_read() */
-
 	read->rd_filp = NULL;
 	if (read->rd_offset >= OFFSET_MAX)
 		return nfserr_inval;
@@ -778,9 +776,9 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		clear_bit(RQ_SPLICE_OK, &rqstp->rq_flags);
 
 	/* check stateid */
-	if ((status = nfs4_preprocess_stateid_op(SVC_NET(rqstp),
-						 cstate, &read->rd_stateid,
-						 RD_STATE, &read->rd_filp))) {
+	status = nfs4_preprocess_stateid_op(rqstp, cstate, &read->rd_stateid,
+			RD_STATE, &read->rd_filp, &read->rd_tmp_file);
+	if (status) {
 		dprintk("NFSD: nfsd4_read: couldn't process stateid!\n");
 		goto out;
 	}
@@ -924,8 +922,8 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	int err;
 
 	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
-		status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
-			&setattr->sa_stateid, WR_STATE, NULL);
+		status = nfs4_preprocess_stateid_op(rqstp, cstate,
+			&setattr->sa_stateid, WR_STATE, NULL, NULL);
 		if (status) {
 			dprintk("NFSD: nfsd4_setattr: couldn't process stateid!\n");
 			return status;
@@ -986,13 +984,11 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unsigned long cnt;
 	int nvecs;
 
-	/* no need to check permission - this will be done in nfsd_write() */
-
 	if (write->wr_offset >= OFFSET_MAX)
 		return nfserr_inval;
 
-	status = nfs4_preprocess_stateid_op(SVC_NET(rqstp),
-					cstate, stateid, WR_STATE, &filp);
+	status = nfs4_preprocess_stateid_op(rqstp, cstate, stateid, WR_STATE,
+			&filp, NULL);
 	if (status) {
 		dprintk("NFSD: nfsd4_write: couldn't process stateid!\n");
 		return status;
@@ -1005,11 +1001,10 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nvecs = fill_in_write_vector(rqstp->rq_vec, write);
 	WARN_ON_ONCE(nvecs > ARRAY_SIZE(rqstp->rq_vec));
 
-	status =  nfsd_write(rqstp, &cstate->current_fh, filp,
-			     write->wr_offset, rqstp->rq_vec, nvecs,
-			     &cnt, &write->wr_how_written);
-	if (filp)
-		fput(filp);
+	status = nfsd_vfs_write(rqstp, &cstate->current_fh, filp,
+				write->wr_offset, rqstp->rq_vec, nvecs, &cnt,
+				&write->wr_how_written);
+	fput(filp);
 
 	write->wr_bytes_written = cnt;
 
@@ -1023,15 +1018,13 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	__be32 status = nfserr_notsupp;
 	struct file *file;
 
-	status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
+	status = nfs4_preprocess_stateid_op(rqstp, cstate,
 					    &fallocate->falloc_stateid,
-					    WR_STATE, &file);
+					    WR_STATE, &file, NULL);
 	if (status != nfs_ok) {
 		dprintk("NFSD: nfsd4_fallocate: couldn't process stateid!\n");
 		return status;
 	}
-	if (!file)
-		return nfserr_bad_stateid;
 
 	status = nfsd4_vfs_fallocate(rqstp, &cstate->current_fh, file,
 				     fallocate->falloc_offset,
@@ -1064,15 +1057,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	__be32 status;
 	struct file *file;
 
-	status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
+	status = nfs4_preprocess_stateid_op(rqstp, cstate,
 					    &seek->seek_stateid,
-					    RD_STATE, &file);
+					    RD_STATE, &file, NULL);
 	if (status) {
 		dprintk("NFSD: nfsd4_seek: couldn't process stateid!\n");
 		return status;
 	}
-	if (!file)
-		return nfserr_bad_stateid;
 
 	switch (seek->seek_whence) {
 	case NFS4_CONTENT_DATA:
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5070688..e8d3f1b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4613,23 +4613,40 @@ nfs4_check_olstateid(struct svc_fh *fhp, struct nfs4_ol_stateid *ols, int flags)
  * Checks for stateid operations
  */
 __be32
-nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
-			   stateid_t *stateid, int flags, struct file **filpp)
+nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
+		struct nfsd4_compound_state *cstate, stateid_t *stateid,
+		int flags, struct file **filpp, bool *tmp_file)
 {
 	struct svc_fh *fhp = &cstate->current_fh;
 	struct inode *ino = d_inode(fhp->fh_dentry);
+	struct net *net = SVC_NET(rqstp);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+	int acc = (flags & RD_STATE) ? NFSD_MAY_READ : NFSD_MAY_WRITE;
 	struct nfs4_stid *s;
 	__be32 status;
 
 	if (filpp)
 		*filpp = NULL;
+	if (tmp_file)
+		*tmp_file = false;
 
 	if (grace_disallows_io(net, ino))
 		return nfserr_grace;
 
-	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
-		return check_special_stateids(net, fhp, stateid, flags);
+	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) {
+		status = check_special_stateids(net, fhp, stateid, flags);
+		if (status)
+			return status;
+
+		if (filpp) {
+			status = nfsd_open(rqstp, fhp, S_IFREG, acc, filpp);
+			if (status)
+				return status;
+			*tmp_file = true;
+		}
+
+		return nfs_ok;
+	}
 
 	status = nfsd4_lookup_stateid(cstate, stateid,
 				NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
@@ -4656,9 +4673,17 @@ nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
 
 	if (!status && filpp) {
 		*filpp = nfs4_find_file(s, flags);
-		if (!*filpp)
+		if (!*filpp) {
 			status = nfserr_serverfault;
+			goto out;
+		}
+
+		status = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
+				acc | NFSD_MAY_OWNER_OVERRIDE);
+		if (status)
+			fput(*filpp);
 	}
+
 out:
 	nfs4_put_stid(s);
 	return status;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 54aced2..692f539 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -33,7 +33,6 @@
  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <linux/file.h>
 #include <linux/slab.h>
 #include <linux/namei.h>
 #include <linux/statfs.h>
@@ -3423,7 +3422,6 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
 	unsigned long maxcount;
 	struct xdr_stream *xdr = &resp->xdr;
 	struct file *file = read->rd_filp;
-	struct svc_fh *fhp = read->rd_fhp;
 	int starting_len = xdr->buf->len;
 	struct raparms *ra = NULL;
 	__be32 *p;
@@ -3447,20 +3445,8 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
 	maxcount = min_t(unsigned long, maxcount, (xdr->buf->buflen - xdr->buf->len));
 	maxcount = min_t(unsigned long, maxcount, read->rd_length);
 
-	if (read->rd_filp) {
-		err = nfsd_permission(resp->rqstp, fhp->fh_export,
-				fhp->fh_dentry,
-				NFSD_MAY_READ|NFSD_MAY_OWNER_OVERRIDE);
-		if (err)
-			goto err_truncate;
-	} else {
-		err = nfsd_open(resp->rqstp, fhp, S_IFREG, NFSD_MAY_READ,
-				&file);
-		if (err)
-			goto err_truncate;
-
+	if (read->rd_tmp_file)
 		ra = nfsd_init_raparms(file);
-	}
 
 	if (file->f_op->splice_read && test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags))
 		err = nfsd4_encode_splice_read(resp, read, file, maxcount);
@@ -3469,10 +3455,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
 
 	if (ra)
 		nfsd_put_raparams(file, ra);
-	if (!read->rd_filp)
-		fput(file);
 
-err_truncate:
 	if (err)
 		xdr_truncate_encode(xdr, starting_len);
 	return err;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index bde45d9..06755c0 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -583,9 +583,9 @@ enum nfsd4_cb_op {
 struct nfsd4_compound_state;
 struct nfsd_net;
 
-extern __be32 nfs4_preprocess_stateid_op(struct net *net,
-		struct nfsd4_compound_state *cstate,
-		stateid_t *stateid, int flags, struct file **filp);
+extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
+		struct nfsd4_compound_state *cstate, stateid_t *stateid,
+		int flags, struct file **filp, bool *tmp_file);
 __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn);
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 9798c2cd..2418419 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -538,16 +538,11 @@ __be32 nfsd4_vfs_fallocate(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			   struct file *file, loff_t offset, loff_t len,
 			   int flags)
 {
-	__be32 err;
 	int error;
 
 	if (!S_ISREG(file_inode(file)->i_mode))
 		return nfserr_inval;
 
-	err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry, NFSD_MAY_WRITE);
-	if (err)
-		return err;
-
 	error = vfs_fallocate(file, flags, offset, len);
 	if (!error)
 		error = commit_metadata(fhp);
@@ -948,7 +943,7 @@ static int wait_for_concurrent_writes(struct file *file)
 	return err;
 }
 
-static __be32
+__be32
 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
 				loff_t offset, struct kvec *vec, int vlen,
 				unsigned long *cnt, int *stablep)
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 053c4ad..5be875e 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -80,6 +80,10 @@ __be32 		nfsd_read(struct svc_rqst *, struct svc_fh *,
 				loff_t, struct kvec *, int, unsigned long *);
 __be32 		nfsd_write(struct svc_rqst *, struct svc_fh *,struct file *,
 				loff_t, struct kvec *,int, unsigned long *, int *);
+__be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
+				struct file *file, loff_t offset,
+				struct kvec *vec, int vlen, unsigned long *cnt,
+				int *stablep);
 __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
 				char *, int *);
 __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 2f8c092..9f99100 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -273,6 +273,7 @@ struct nfsd4_read {
 	u32		rd_length;          /* request */
 	int		rd_vlen;
 	struct file     *rd_filp;
+	bool		rd_tmp_file;
 	
 	struct svc_rqst *rd_rqstp;          /* response */
 	struct svc_fh * rd_fhp;             /* response */
-- 
1.9.1

Re: [PATCH 5/6] nfsd: take struct file setup fully into nfs4_preprocess_stateid_op

[J. Bruce Fields]

On Tue, Apr 28, 2015 at 03:41:19PM +0200, Christoph Hellwig wrote:
> This patch changes nfs4_preprocess_stateid_op so it always returns
> a valid struct file if it has been asked for that.  For that we
> now allocate a temporary struct file for special stateids, and check
> permissions if we got the file structure from the stateid.  This
> ensures that all callers will get their handling of special stateids
> right, and avoids code duplication.
> 
> There is a little wart in here because the read code needs to know
> if we allocated a file structure so that it can copy around the
> read-ahead parameters.  In the long run we should probably aim to
> cache full file structures used with special stateids instead.

This causes a failure on pynfs OPEN23b.

It's doing a READ using a stateid from a write open.  We previously
returned NFS_OK, taking the "may" option from:

	http://tools.ietf.org/html/rfc7530#page-111

	In the case of READ, the server may perform the corresponding
	check on the access mode, or it may choose to allow READ on
	opens for WRITE only, to accommodate clients whose write
	implementation may unavoidably do reads (e.g., due to buffer
	cache constraints).

OPENMODE might also have been OK, but we're returning SERVERFAULT.  I
guess the old code was passing preprocess_stateid_op without returning a
file, then relying on a temporary open for the read?  Ugh.

--b.


> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfsd/nfs4proc.c  | 39 +++++++++++++++------------------------
>  fs/nfsd/nfs4state.c | 35 ++++++++++++++++++++++++++++++-----
>  fs/nfsd/nfs4xdr.c   | 19 +------------------
>  fs/nfsd/state.h     |  6 +++---
>  fs/nfsd/vfs.c       |  7 +------
>  fs/nfsd/vfs.h       |  4 ++++
>  fs/nfsd/xdr4.h      |  1 +
>  7 files changed, 55 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 864e200..5aa7c4e 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -760,8 +760,6 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  {
>  	__be32 status;
>  
> -	/* no need to check permission - this will be done in nfsd_read() */
> -
>  	read->rd_filp = NULL;
>  	if (read->rd_offset >= OFFSET_MAX)
>  		return nfserr_inval;
> @@ -778,9 +776,9 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		clear_bit(RQ_SPLICE_OK, &rqstp->rq_flags);
>  
>  	/* check stateid */
> -	if ((status = nfs4_preprocess_stateid_op(SVC_NET(rqstp),
> -						 cstate, &read->rd_stateid,
> -						 RD_STATE, &read->rd_filp))) {
> +	status = nfs4_preprocess_stateid_op(rqstp, cstate, &read->rd_stateid,
> +			RD_STATE, &read->rd_filp, &read->rd_tmp_file);
> +	if (status) {
>  		dprintk("NFSD: nfsd4_read: couldn't process stateid!\n");
>  		goto out;
>  	}
> @@ -924,8 +922,8 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	int err;
>  
>  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> -		status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
> -			&setattr->sa_stateid, WR_STATE, NULL);
> +		status = nfs4_preprocess_stateid_op(rqstp, cstate,
> +			&setattr->sa_stateid, WR_STATE, NULL, NULL);
>  		if (status) {
>  			dprintk("NFSD: nfsd4_setattr: couldn't process stateid!\n");
>  			return status;
> @@ -986,13 +984,11 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	unsigned long cnt;
>  	int nvecs;
>  
> -	/* no need to check permission - this will be done in nfsd_write() */
> -
>  	if (write->wr_offset >= OFFSET_MAX)
>  		return nfserr_inval;
>  
> -	status = nfs4_preprocess_stateid_op(SVC_NET(rqstp),
> -					cstate, stateid, WR_STATE, &filp);
> +	status = nfs4_preprocess_stateid_op(rqstp, cstate, stateid, WR_STATE,
> +			&filp, NULL);
>  	if (status) {
>  		dprintk("NFSD: nfsd4_write: couldn't process stateid!\n");
>  		return status;
> @@ -1005,11 +1001,10 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	nvecs = fill_in_write_vector(rqstp->rq_vec, write);
>  	WARN_ON_ONCE(nvecs > ARRAY_SIZE(rqstp->rq_vec));
>  
> -	status =  nfsd_write(rqstp, &cstate->current_fh, filp,
> -			     write->wr_offset, rqstp->rq_vec, nvecs,
> -			     &cnt, &write->wr_how_written);
> -	if (filp)
> -		fput(filp);
> +	status = nfsd_vfs_write(rqstp, &cstate->current_fh, filp,
> +				write->wr_offset, rqstp->rq_vec, nvecs, &cnt,
> +				&write->wr_how_written);
> +	fput(filp);
>  
>  	write->wr_bytes_written = cnt;
>  
> @@ -1023,15 +1018,13 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	__be32 status = nfserr_notsupp;
>  	struct file *file;
>  
> -	status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
> +	status = nfs4_preprocess_stateid_op(rqstp, cstate,
>  					    &fallocate->falloc_stateid,
> -					    WR_STATE, &file);
> +					    WR_STATE, &file, NULL);
>  	if (status != nfs_ok) {
>  		dprintk("NFSD: nfsd4_fallocate: couldn't process stateid!\n");
>  		return status;
>  	}
> -	if (!file)
> -		return nfserr_bad_stateid;
>  
>  	status = nfsd4_vfs_fallocate(rqstp, &cstate->current_fh, file,
>  				     fallocate->falloc_offset,
> @@ -1064,15 +1057,13 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	__be32 status;
>  	struct file *file;
>  
> -	status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
> +	status = nfs4_preprocess_stateid_op(rqstp, cstate,
>  					    &seek->seek_stateid,
> -					    RD_STATE, &file);
> +					    RD_STATE, &file, NULL);
>  	if (status) {
>  		dprintk("NFSD: nfsd4_seek: couldn't process stateid!\n");
>  		return status;
>  	}
> -	if (!file)
> -		return nfserr_bad_stateid;
>  
>  	switch (seek->seek_whence) {
>  	case NFS4_CONTENT_DATA:
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 5070688..e8d3f1b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4613,23 +4613,40 @@ nfs4_check_olstateid(struct svc_fh *fhp, struct nfs4_ol_stateid *ols, int flags)
>   * Checks for stateid operations
>   */
>  __be32
> -nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
> -			   stateid_t *stateid, int flags, struct file **filpp)
> +nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> +		struct nfsd4_compound_state *cstate, stateid_t *stateid,
> +		int flags, struct file **filpp, bool *tmp_file)
>  {
>  	struct svc_fh *fhp = &cstate->current_fh;
>  	struct inode *ino = d_inode(fhp->fh_dentry);
> +	struct net *net = SVC_NET(rqstp);
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +	int acc = (flags & RD_STATE) ? NFSD_MAY_READ : NFSD_MAY_WRITE;
>  	struct nfs4_stid *s;
>  	__be32 status;
>  
>  	if (filpp)
>  		*filpp = NULL;
> +	if (tmp_file)
> +		*tmp_file = false;
>  
>  	if (grace_disallows_io(net, ino))
>  		return nfserr_grace;
>  
> -	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
> -		return check_special_stateids(net, fhp, stateid, flags);
> +	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) {
> +		status = check_special_stateids(net, fhp, stateid, flags);
> +		if (status)
> +			return status;
> +
> +		if (filpp) {
> +			status = nfsd_open(rqstp, fhp, S_IFREG, acc, filpp);
> +			if (status)
> +				return status;
> +			*tmp_file = true;
> +		}
> +
> +		return nfs_ok;
> +	}
>  
>  	status = nfsd4_lookup_stateid(cstate, stateid,
>  				NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
> @@ -4656,9 +4673,17 @@ nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
>  
>  	if (!status && filpp) {
>  		*filpp = nfs4_find_file(s, flags);
> -		if (!*filpp)
> +		if (!*filpp) {
>  			status = nfserr_serverfault;
> +			goto out;
> +		}
> +
> +		status = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> +				acc | NFSD_MAY_OWNER_OVERRIDE);
> +		if (status)
> +			fput(*filpp);
>  	}
> +
>  out:
>  	nfs4_put_stid(s);
>  	return status;
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 54aced2..692f539 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -33,7 +33,6 @@
>   *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> -#include <linux/file.h>
>  #include <linux/slab.h>
>  #include <linux/namei.h>
>  #include <linux/statfs.h>
> @@ -3423,7 +3422,6 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	unsigned long maxcount;
>  	struct xdr_stream *xdr = &resp->xdr;
>  	struct file *file = read->rd_filp;
> -	struct svc_fh *fhp = read->rd_fhp;
>  	int starting_len = xdr->buf->len;
>  	struct raparms *ra = NULL;
>  	__be32 *p;
> @@ -3447,20 +3445,8 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	maxcount = min_t(unsigned long, maxcount, (xdr->buf->buflen - xdr->buf->len));
>  	maxcount = min_t(unsigned long, maxcount, read->rd_length);
>  
> -	if (read->rd_filp) {
> -		err = nfsd_permission(resp->rqstp, fhp->fh_export,
> -				fhp->fh_dentry,
> -				NFSD_MAY_READ|NFSD_MAY_OWNER_OVERRIDE);
> -		if (err)
> -			goto err_truncate;
> -	} else {
> -		err = nfsd_open(resp->rqstp, fhp, S_IFREG, NFSD_MAY_READ,
> -				&file);
> -		if (err)
> -			goto err_truncate;
> -
> +	if (read->rd_tmp_file)
>  		ra = nfsd_init_raparms(file);
> -	}
>  
>  	if (file->f_op->splice_read && test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags))
>  		err = nfsd4_encode_splice_read(resp, read, file, maxcount);
> @@ -3469,10 +3455,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>  
>  	if (ra)
>  		nfsd_put_raparams(file, ra);
> -	if (!read->rd_filp)
> -		fput(file);
>  
> -err_truncate:
>  	if (err)
>  		xdr_truncate_encode(xdr, starting_len);
>  	return err;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index bde45d9..06755c0 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -583,9 +583,9 @@ enum nfsd4_cb_op {
>  struct nfsd4_compound_state;
>  struct nfsd_net;
>  
> -extern __be32 nfs4_preprocess_stateid_op(struct net *net,
> -		struct nfsd4_compound_state *cstate,
> -		stateid_t *stateid, int flags, struct file **filp);
> +extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> +		struct nfsd4_compound_state *cstate, stateid_t *stateid,
> +		int flags, struct file **filp, bool *tmp_file);
>  __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		     stateid_t *stateid, unsigned char typemask,
>  		     struct nfs4_stid **s, struct nfsd_net *nn);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 9798c2cd..2418419 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -538,16 +538,11 @@ __be32 nfsd4_vfs_fallocate(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			   struct file *file, loff_t offset, loff_t len,
>  			   int flags)
>  {
> -	__be32 err;
>  	int error;
>  
>  	if (!S_ISREG(file_inode(file)->i_mode))
>  		return nfserr_inval;
>  
> -	err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry, NFSD_MAY_WRITE);
> -	if (err)
> -		return err;
> -
>  	error = vfs_fallocate(file, flags, offset, len);
>  	if (!error)
>  		error = commit_metadata(fhp);
> @@ -948,7 +943,7 @@ static int wait_for_concurrent_writes(struct file *file)
>  	return err;
>  }
>  
> -static __be32
> +__be32
>  nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
>  				loff_t offset, struct kvec *vec, int vlen,
>  				unsigned long *cnt, int *stablep)
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 053c4ad..5be875e 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -80,6 +80,10 @@ __be32 		nfsd_read(struct svc_rqst *, struct svc_fh *,
>  				loff_t, struct kvec *, int, unsigned long *);
>  __be32 		nfsd_write(struct svc_rqst *, struct svc_fh *,struct file *,
>  				loff_t, struct kvec *,int, unsigned long *, int *);
> +__be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> +				struct file *file, loff_t offset,
> +				struct kvec *vec, int vlen, unsigned long *cnt,
> +				int *stablep);
>  __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
>  				char *, int *);
>  __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 2f8c092..9f99100 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -273,6 +273,7 @@ struct nfsd4_read {
>  	u32		rd_length;          /* request */
>  	int		rd_vlen;
>  	struct file     *rd_filp;
> +	bool		rd_tmp_file;
>  	
>  	struct svc_rqst *rd_rqstp;          /* response */
>  	struct svc_fh * rd_fhp;             /* response */
> -- 
> 1.9.1

Re: [PATCH 5/6] nfsd: take struct file setup fully into nfs4_preprocess_stateid_op

[J. Bruce Fields]

On Tue, Apr 28, 2015 at 04:44:55PM -0400, J. Bruce Fields wrote:
> On Tue, Apr 28, 2015 at 03:41:19PM +0200, Christoph Hellwig wrote:
> > This patch changes nfs4_preprocess_stateid_op so it always returns
> > a valid struct file if it has been asked for that.  For that we
> > now allocate a temporary struct file for special stateids, and check
> > permissions if we got the file structure from the stateid.  This
> > ensures that all callers will get their handling of special stateids
> > right, and avoids code duplication.
> > 
> > There is a little wart in here because the read code needs to know
> > if we allocated a file structure so that it can copy around the
> > read-ahead parameters.  In the long run we should probably aim to
> > cache full file structures used with special stateids instead.
> 
> This causes a failure on pynfs OPEN23b.
> 
> It's doing a READ using a stateid from a write open.  We previously
> returned NFS_OK, taking the "may" option from:
> 
> 	http://tools.ietf.org/html/rfc7530#page-111
> 
> 	In the case of READ, the server may perform the corresponding
> 	check on the access mode, or it may choose to allow READ on
> 	opens for WRITE only, to accommodate clients whose write
> 	implementation may unavoidably do reads (e.g., due to buffer
> 	cache constraints).
> 
> OPENMODE might also have been OK, but we're returning SERVERFAULT.  I
> guess the old code was passing preprocess_stateid_op without returning a
> file, then relying on a temporary open for the read?  Ugh.

Hm, and writes with special stateids (e.g. WRT1) are timing out, I
haven't figured that out.

--b.

Re: [PATCH 5/6] nfsd: take struct file setup fully into nfs4_preprocess_stateid_op

[Christoph Hellwig]

On Tue, Apr 28, 2015 at 04:44:55PM -0400, J. Bruce Fields wrote:
> This causes a failure on pynfs OPEN23b.

I've once again tried to run pynfs, but I'm still errors out with weird
python backtraces for any of the examples from the readme I copy
and pasted, e.g.:

root@vm:~/pynfs/nfs4.0# ./testserver.py 127.0.0.1:/mnt/test --maketree all
Initialization failed, no tests run.
Traceback (most recent call last):
  File "./testserver.py", line 379, in <module>
    main()
  File "./testserver.py", line 342, in main
    env.init()
  File "/root/pynfs/nfs4.0/servertests/environment.py", line 140, in init
    self._maketree()
  File "/root/pynfs/nfs4.0/servertests/environment.py", line 159, in _maketree
    "Could not LOOKUP /%s," % '/'.join(path))
  File "/root/pynfs/nfs4.0/servertests/environment.py", line 274, in checklist
    raise testmod.FailureException(msg)
testmod.FailureException: Could not LOOKUP /mnt, should return NFS4_OK or NFS4ERR_NOENT, instead got NFS4ERR_PERM


root@vm:~/pynfs/nfs4.1# ./testserver.py 127.0.0.1:/mnt/test --maketree all
Could not find gssapi module, proceeding without
INFO   :rpc.poll:got connection from ('127.0.0.1', 37715), assigned to fd=5
INFO   :rpc.thread:Called connect(('127.0.0.1', 2049))
INFO   :rpc.poll:Adding 6 generated by another thread
INFO   :test.env:Created client to 127.0.0.1, 2049
INFO   :nfs.client.cb:********************
INFO   :nfs.client.cb:Handling CB_NULL
Initialization failed, no tests run.
Traceback (most recent call last):
  File "./testserver.py", line 359, in <module>
    main()
  File "./testserver.py", line 322, in main
    env.init()
  File "/root/pynfs/nfs4.1/server41tests/environment.py", line 150, in init
    self._maketree(sess)
  File "/root/pynfs/nfs4.1/server41tests/environment.py", line 166, in _maketree
    "LOOKUP /%s," % '/'.join(path))
  File "/root/pynfs/nfs4.1/server41tests/environment.py", line 304, in checklist
    raise testmod.FailureException(msg)
testmod.FailureException: LOOKUP /mnt, should return NFS4_OK or NFS4ERR_NOENT, instead got NFS4ERR_PERM


> It's doing a READ using a stateid from a write open.  We previously
> returned NFS_OK, taking the "may" option from:
> 
> 	http://tools.ietf.org/html/rfc7530#page-111
> 
> 	In the case of READ, the server may perform the corresponding
> 	check on the access mode, or it may choose to allow READ on
> 	opens for WRITE only, to accommodate clients whose write
> 	implementation may unavoidably do reads (e.g., due to buffer
> 	cache constraints).
> 
> OPENMODE might also have been OK, but we're returning SERVERFAULT.  I
> guess the old code was passing preprocess_stateid_op without returning a
> file, then relying on a temporary open for the read?  Ugh.

Looks like it.  I can change it to return an OPENMODE, or we could
make it fall back to a temp read open.

Re: [PATCH 5/6] nfsd: take struct file setup fully into nfs4_preprocess_stateid_op

[J. Bruce Fields]

On Wed, Apr 29, 2015 at 03:11:48PM +0200, Christoph Hellwig wrote:
> On Tue, Apr 28, 2015 at 04:44:55PM -0400, J. Bruce Fields wrote:
> > This causes a failure on pynfs OPEN23b.
> 
> I've once again tried to run pynfs, but I'm still errors out with weird
> python backtraces for any of the examples from the readme I copy
> and pasted, e.g.:
> 
> root@vm:~/pynfs/nfs4.0# ./testserver.py 127.0.0.1:/mnt/test --maketree all
> Initialization failed, no tests run.
> Traceback (most recent call last):
>   File "./testserver.py", line 379, in <module>
>     main()
>   File "./testserver.py", line 342, in main
>     env.init()
>   File "/root/pynfs/nfs4.0/servertests/environment.py", line 140, in init
>     self._maketree()
>   File "/root/pynfs/nfs4.0/servertests/environment.py", line 159, in _maketree
>     "Could not LOOKUP /%s," % '/'.join(path))
>   File "/root/pynfs/nfs4.0/servertests/environment.py", line 274, in checklist
>     raise testmod.FailureException(msg)
> testmod.FailureException: Could not LOOKUP /mnt, should return NFS4_OK or NFS4ERR_NOENT, instead got NFS4ERR_PERM

You probably just need to add "insecure" to the export.

(Every now and then we talk about changing that default.  The security
model there assumed you have untrusted users, but trusted root on every
machine on your network.  So the fact that a connection came from a low
port was enough to trust it.  That assumption seems fragile.  I don't
know if it's still a common setup.)

> > It's doing a READ using a stateid from a write open.  We previously
> > returned NFS_OK, taking the "may" option from:
> > 
> > 	http://tools.ietf.org/html/rfc7530#page-111
> > 
> > 	In the case of READ, the server may perform the corresponding
> > 	check on the access mode, or it may choose to allow READ on
> > 	opens for WRITE only, to accommodate clients whose write
> > 	implementation may unavoidably do reads (e.g., due to buffer
> > 	cache constraints).
> > 
> > OPENMODE might also have been OK, but we're returning SERVERFAULT.  I
> > guess the old code was passing preprocess_stateid_op without returning a
> > file, then relying on a temporary open for the read?  Ugh.
> 
> Looks like it.  I can change it to return an OPENMODE, or we could
> make it fall back to a temp read open.

I don't know if anyone actually behaves as described in the spec, but
for now I'd rather be conservative and keep the old behavior.  (So, yes,
fall back on a temporary read open.)

--b.

[PATCH 6/6] nfsd: drop the file argument to nfsd_write

[Christoph Hellwig]

All remaining callers of nfsd_write will never pass a file argument, so
remove it and the associated code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs3proc.c |  2 +-
 fs/nfsd/nfsproc.c  |  2 +-
 fs/nfsd/vfs.c      | 32 +++++++++++---------------------
 fs/nfsd/vfs.h      |  5 +++--
 4 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 7b755b7..4e46ac5 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -192,7 +192,7 @@ nfsd3_proc_write(struct svc_rqst *rqstp, struct nfsd3_writeargs *argp,
 
 	fh_copy(&resp->fh, &argp->fh);
 	resp->committed = argp->stable;
-	nfserr = nfsd_write(rqstp, &resp->fh, NULL,
+	nfserr = nfsd_write(rqstp, &resp->fh,
 				   argp->offset,
 				   rqstp->rq_vec, argp->vlen,
 				   &cnt,
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index aecbcd3..83100b5 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -165,7 +165,7 @@ nfsd_proc_write(struct svc_rqst *rqstp, struct nfsd_writeargs *argp,
 		SVCFH_fmt(&argp->fh),
 		argp->len, argp->offset);
 
-	nfserr = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh), NULL,
+	nfserr = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh),
 				   argp->offset,
 				   rqstp->rq_vec, argp->vlen,
 			           &cnt,
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 2418419..b8a0a3b 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1038,30 +1038,20 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
  * N.B. After this call fhp needs an fh_put
  */
 __be32
-nfsd_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
-		loff_t offset, struct kvec *vec, int vlen, unsigned long *cnt,
-		int *stablep)
+nfsd_write(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t offset,
+		struct kvec *vec, int vlen, unsigned long *cnt, int *stablep)
 {
-	__be32			err = 0;
+	struct file		*file;
+	__be32			err;
 
-	if (file) {
-		err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
-				NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE);
-		if (err)
-			goto out;
-		err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen, cnt,
-				stablep);
-	} else {
-		err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_WRITE, &file);
-		if (err)
-			goto out;
+	err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_WRITE, &file);
+	if (err)
+		return err;
 
-		if (cnt)
-			err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen,
-					     cnt, stablep);
-		fput(file);
-	}
-out:
+	if (cnt)
+		err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen,
+				     cnt, stablep);
+	fput(file);
 	return err;
 }
 
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 5be875e..f1a7a3b 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -78,8 +78,9 @@ __be32		nfsd_readv(struct file *, loff_t, struct kvec *, int,
 				unsigned long *);
 __be32 		nfsd_read(struct svc_rqst *, struct svc_fh *,
 				loff_t, struct kvec *, int, unsigned long *);
-__be32 		nfsd_write(struct svc_rqst *, struct svc_fh *,struct file *,
-				loff_t, struct kvec *,int, unsigned long *, int *);
+__be32		nfsd_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
+				loff_t offset, struct kvec *vec, int vlen,
+				unsigned long *cnt, int *stablep);
 __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				struct file *file, loff_t offset,
 				struct kvec *vec, int vlen, unsigned long *cnt,
-- 
1.9.1