[PATCH v2 0/7] Tighten up locking around stateids in knfsd

[PATCH v2 0/7] Tighten up locking around stateids in knfsd

[Trond Myklebust]

This patchset aims to tighten up the locking rules around stateids to
ensure that knfsd does not reuse stateids that have already been closed
or invalidated. The aim is to ensure we enforce the RFC5661 and RFC7530
rules concerning stateid initialisation and updates.

v2:
- Add a fix for when nfs4_get_vfs_file() fails in nfsd4_process_open2()
- Add a fix for byte range lock creation

Note that byte range locks are not completely fixed. The remaining task
of ensuring that locks don't conflict with CLOSE has been left as an
exercise for the reviewer.

Trond Myklebust (7):
  nfsd: Fix stateid races between OPEN and CLOSE
  nfsd: Fix another OPEN stateid race
  nfsd: CLOSE SHOULD return the invalid special stateid for NFSv4.x
    (x>0)
  nfsd: Ensure we don't recognise lock stateids after freeing them
  nfsd: Fix race in lock stateid creation
  nfsd: Ensure we check stateid validity in the seqid operation checks
  nfsd: Fix races with check_stateid_generation()

 fs/nfsd/nfs4state.c | 254 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 166 insertions(+), 88 deletions(-)

-- 
2.13.6

[PATCH v2 1/7] nfsd: Fix stateid races between OPEN and CLOSE

[Trond Myklebust]

Open file stateids can linger on the nfs4_file list of stateids even
after they have been closed. In order to avoid reusing such a
stateid, and confusing the client, we need to recheck the
nfs4_stid's type after taking the mutex.
Otherwise, we risk reusing an old stateid that was already closed,
which will confuse clients that expect new stateids to conform to
RFC7530 Sections 9.1.4.2 and 16.2.5 or RFC5661 Sections 8.2.2 and 18.2.4.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: stable@vger.kernel.org
---
 fs/nfsd/nfs4state.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0c04f81aa63b..b246aaa20863 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3512,7 +3512,9 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
 		/* ignore lock owners */
 		if (local->st_stateowner->so_is_open_owner == 0)
 			continue;
-		if (local->st_stateowner == &oo->oo_owner) {
+		if (local->st_stateowner != &oo->oo_owner)
+			continue;
+		if (local->st_stid.sc_type == NFS4_OPEN_STID) {
 			ret = local;
 			atomic_inc(&ret->st_stid.sc_count);
 			break;
@@ -3521,6 +3523,52 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
 	return ret;
 }
 
+static __be32
+nfsd4_verify_open_stid(struct nfs4_stid *s)
+{
+	__be32 ret = nfs_ok;
+
+	switch (s->sc_type) {
+	default:
+		break;
+	case NFS4_CLOSED_STID:
+	case NFS4_CLOSED_DELEG_STID:
+		ret = nfserr_bad_stateid;
+		break;
+	case NFS4_REVOKED_DELEG_STID:
+		ret = nfserr_deleg_revoked;
+	}
+	return ret;
+}
+
+/* Lock the stateid st_mutex, and deal with races with CLOSE */
+static __be32
+nfsd4_lock_ol_stateid(struct nfs4_ol_stateid *stp)
+{
+	__be32 ret;
+
+	mutex_lock(&stp->st_mutex);
+	ret = nfsd4_verify_open_stid(&stp->st_stid);
+	if (ret != nfs_ok)
+		mutex_unlock(&stp->st_mutex);
+	return ret;
+}
+
+static struct nfs4_ol_stateid *
+nfsd4_find_and_lock_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
+{
+	struct nfs4_ol_stateid *stp;
+	for (;;) {
+		spin_lock(&fp->fi_lock);
+		stp = nfsd4_find_existing_open(fp, open);
+		spin_unlock(&fp->fi_lock);
+		if (!stp || nfsd4_lock_ol_stateid(stp) == nfs_ok)
+			break;
+		nfs4_put_stid(&stp->st_stid);
+	}
+	return stp;
+}
+
 static struct nfs4_openowner *
 alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
 			   struct nfsd4_compound_state *cstate)
@@ -3565,6 +3613,7 @@ init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
 	mutex_init(&stp->st_mutex);
 	mutex_lock(&stp->st_mutex);
 
+retry:
 	spin_lock(&oo->oo_owner.so_client->cl_lock);
 	spin_lock(&fp->fi_lock);
 
@@ -3589,7 +3638,11 @@ init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
 	spin_unlock(&fp->fi_lock);
 	spin_unlock(&oo->oo_owner.so_client->cl_lock);
 	if (retstp) {
-		mutex_lock(&retstp->st_mutex);
+		/* Handle races with CLOSE */
+		if (nfsd4_lock_ol_stateid(retstp) != nfs_ok) {
+			nfs4_put_stid(&retstp->st_stid);
+			goto retry;
+		}
 		/* To keep mutex tracking happy */
 		mutex_unlock(&stp->st_mutex);
 		stp = retstp;
@@ -4403,9 +4456,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)
 			goto out;
-		spin_lock(&fp->fi_lock);
-		stp = nfsd4_find_existing_open(fp, open);
-		spin_unlock(&fp->fi_lock);
+		stp = nfsd4_find_and_lock_existing_open(fp, open);
 	} else {
 		open->op_file = NULL;
 		status = nfserr_bad_stateid;
@@ -4419,7 +4470,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 */
 	if (stp) {
 		/* Stateid was found, this is an OPEN upgrade */
-		mutex_lock(&stp->st_mutex);
 		status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
 		if (status) {
 			mutex_unlock(&stp->st_mutex);
@@ -5294,7 +5344,6 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
 	bool unhashed;
 	LIST_HEAD(reaplist);
 
-	s->st_stid.sc_type = NFS4_CLOSED_STID;
 	spin_lock(&clp->cl_lock);
 	unhashed = unhash_open_stateid(s, &reaplist);
 
@@ -5334,10 +5383,12 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfsd4_bump_seqid(cstate, status);
 	if (status)
 		goto out; 
+
+	stp->st_stid.sc_type = NFS4_CLOSED_STID;
 	nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
-	mutex_unlock(&stp->st_mutex);
 
 	nfsd4_close_open_stateid(stp);
+	mutex_unlock(&stp->st_mutex);
 
 	/* put reference from nfs4_preprocess_seqid_op */
 	nfs4_put_stid(&stp->st_stid);
-- 
2.13.6

[PATCH v2 2/7] nfsd: Fix another OPEN stateid race

[Trond Myklebust]

If nfsd4_process_open2() is initialising a new stateid, and yet the
call to nfs4_get_vfs_file() fails for some reason, then we must
declare the stateid closed, and unhash it before dropping the mutex.

Right now, we unhash the stateid after dropping the mutex, and without
changing the stateid type, meaning that another OPEN could theoretically
look it up and attempt to use it.

Reported-by: Andrew W Elble <aweits@rit.edu>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: stable@vger.kernel.org
---
 fs/nfsd/nfs4state.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b246aaa20863..c7dcedbd9c41 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4445,6 +4445,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	struct nfs4_ol_stateid *stp = NULL;
 	struct nfs4_delegation *dp = NULL;
 	__be32 status;
+	bool new_stp = false;
 
 	/*
 	 * Lookup file; if found, lookup stateid and check open request,
@@ -4464,11 +4465,19 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 			goto out;
 	}
 
+	if (!stp) {
+		stp = init_open_stateid(fp, open);
+		if (!open->op_stp)
+			new_stp = true;
+	}
+
 	/*
 	 * OPEN the file, or upgrade an existing OPEN.
 	 * If truncate fails, the OPEN fails.
+	 *
+	 * stp is already locked.
 	 */
-	if (stp) {
+	if (!new_stp) {
 		/* Stateid was found, this is an OPEN upgrade */
 		status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
 		if (status) {
@@ -4476,22 +4485,11 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 			goto out;
 		}
 	} else {
-		/* stp is returned locked. */
-		stp = init_open_stateid(fp, open);
-		/* See if we lost the race to some other thread */
-		if (stp->st_access_bmap != 0) {
-			status = nfs4_upgrade_open(rqstp, fp, current_fh,
-						stp, open);
-			if (status) {
-				mutex_unlock(&stp->st_mutex);
-				goto out;
-			}
-			goto upgrade_out;
-		}
 		status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
 		if (status) {
-			mutex_unlock(&stp->st_mutex);
+			stp->st_stid.sc_type = NFS4_CLOSED_STID;
 			release_open_stateid(stp);
+			mutex_unlock(&stp->st_mutex);
 			goto out;
 		}
 
@@ -4500,7 +4498,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 		if (stp->st_clnt_odstate == open->op_odstate)
 			open->op_odstate = NULL;
 	}
-upgrade_out:
+
 	nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
 	mutex_unlock(&stp->st_mutex);
 
-- 
2.13.6

[PATCH v2 3/7] nfsd: CLOSE SHOULD return the invalid special stateid for NFSv4.x (x>0)

[Trond Myklebust]

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfs4state.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c7dcedbd9c41..5ae6baff09e0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -63,6 +63,9 @@ static const stateid_t zero_stateid = {
 static const stateid_t currentstateid = {
 	.si_generation = 1,
 };
+static const stateid_t close_stateid = {
+	.si_generation = 0xffffffffU,
+};
 
 static u64 current_sessionid = 1;
 
@@ -5388,6 +5391,11 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfsd4_close_open_stateid(stp);
 	mutex_unlock(&stp->st_mutex);
 
+	/* See RFC5661 sectionm 18.2.4 */
+	if (stp->st_stid.sc_client->cl_minorversion)
+		memcpy(&close->cl_stateid, &close_stateid,
+				sizeof(close->cl_stateid));
+
 	/* put reference from nfs4_preprocess_seqid_op */
 	nfs4_put_stid(&stp->st_stid);
 out:
-- 
2.13.6

[PATCH v2 4/7] nfsd: Ensure we don't recognise lock stateids after freeing them

[Trond Myklebust]

In order to deal with lookup races, nfsd4_free_lock_stateid() needs
to be able to signal to other stateful functions that the lock stateid
is no longer valid. Right now, nfsd_lock() will check whether or not an
existing stateid is still hashed, but only in the "new lock" path.

To ensure the stateid invalidation is also recognised by the "existing lock"
path, and also by a second call to nfsd4_free_lock_stateid() itself, we can
change the type to NFS4_CLOSED_STID under the stp->st_mutex.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfs4state.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5ae6baff09e0..f93ca0682aaa 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5076,7 +5076,9 @@ nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
 	struct nfs4_ol_stateid *stp = openlockstateid(s);
 	__be32 ret;
 
-	mutex_lock(&stp->st_mutex);
+	ret = nfsd4_lock_ol_stateid(stp);
+	if (ret)
+		goto out_put_stid;
 
 	ret = check_stateid_generation(stateid, &s->sc_stateid, 1);
 	if (ret)
@@ -5087,11 +5089,13 @@ nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
 			    lockowner(stp->st_stateowner)))
 		goto out;
 
+	stp->st_stid.sc_type = NFS4_CLOSED_STID;
 	release_lock_stateid(stp);
 	ret = nfs_ok;
 
 out:
 	mutex_unlock(&stp->st_mutex);
+out_put_stid:
 	nfs4_put_stid(s);
 	return ret;
 }
@@ -5660,6 +5664,8 @@ find_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp)
 	lockdep_assert_held(&clp->cl_lock);
 
 	list_for_each_entry(lst, &lo->lo_owner.so_stateids, st_perstateowner) {
+		if (lst->st_stid.sc_type != NFS4_LOCK_STID)
+			continue;
 		if (lst->st_stid.sc_file == fp) {
 			atomic_inc(&lst->st_stid.sc_count);
 			return lst;
@@ -5734,7 +5740,6 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 	struct nfs4_lockowner *lo;
 	struct nfs4_ol_stateid *lst;
 	unsigned int strhashval;
-	bool hashed;
 
 	lo = find_lockowner_str(cl, &lock->lk_new_owner);
 	if (!lo) {
@@ -5757,15 +5762,7 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	mutex_lock(&lst->st_mutex);
-
-	/* See if it's still hashed to avoid race with FREE_STATEID */
-	spin_lock(&cl->cl_lock);
-	hashed = !list_empty(&lst->st_perfile);
-	spin_unlock(&cl->cl_lock);
-
-	if (!hashed) {
-		mutex_unlock(&lst->st_mutex);
+	if (nfsd4_lock_ol_stateid(lst) != nfs_ok) {
 		nfs4_put_stid(&lst->st_stid);
 		goto retry;
 	}
-- 
2.13.6

[PATCH v2 5/7] nfsd: Fix race in lock stateid creation

[Trond Myklebust]

If we're looking up a new lock state, and the creation fails, then
we want to unhash it, just like we do for OPEN. However in order
to do so, we need to that no other LOCK requests can grab the
mutex until we have unhashed it (and marked it as closed).

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfs4state.c | 104 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 59 insertions(+), 45 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f93ca0682aaa..f13fba4f51ec 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5630,14 +5630,41 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 	return ret;
 }
 
-static void
+static struct nfs4_ol_stateid *
+find_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp)
+{
+	struct nfs4_ol_stateid *lst;
+	struct nfs4_client *clp = lo->lo_owner.so_client;
+
+	lockdep_assert_held(&clp->cl_lock);
+
+	list_for_each_entry(lst, &lo->lo_owner.so_stateids, st_perstateowner) {
+		if (lst->st_stid.sc_type != NFS4_LOCK_STID)
+			continue;
+		if (lst->st_stid.sc_file == fp) {
+			atomic_inc(&lst->st_stid.sc_count);
+			return lst;
+		}
+	}
+	return NULL;
+}
+
+static struct nfs4_ol_stateid *
 init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
 		  struct nfs4_file *fp, struct inode *inode,
 		  struct nfs4_ol_stateid *open_stp)
 {
 	struct nfs4_client *clp = lo->lo_owner.so_client;
+	struct nfs4_ol_stateid *retstp;
 
-	lockdep_assert_held(&clp->cl_lock);
+	mutex_init(&stp->st_mutex);
+	mutex_lock(&stp->st_mutex);
+retry:
+	spin_lock(&clp->cl_lock);
+	spin_lock(&fp->fi_lock);
+	retstp = find_lock_stateid(lo, fp);
+	if (retstp)
+		goto out_unlock;
 
 	atomic_inc(&stp->st_stid.sc_count);
 	stp->st_stid.sc_type = NFS4_LOCK_STID;
@@ -5647,31 +5674,22 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
 	stp->st_access_bmap = 0;
 	stp->st_deny_bmap = open_stp->st_deny_bmap;
 	stp->st_openstp = open_stp;
-	mutex_init(&stp->st_mutex);
 	list_add(&stp->st_locks, &open_stp->st_locks);
 	list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
-	spin_lock(&fp->fi_lock);
 	list_add(&stp->st_perfile, &fp->fi_stateids);
+out_unlock:
 	spin_unlock(&fp->fi_lock);
-}
-
-static struct nfs4_ol_stateid *
-find_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp)
-{
-	struct nfs4_ol_stateid *lst;
-	struct nfs4_client *clp = lo->lo_owner.so_client;
-
-	lockdep_assert_held(&clp->cl_lock);
-
-	list_for_each_entry(lst, &lo->lo_owner.so_stateids, st_perstateowner) {
-		if (lst->st_stid.sc_type != NFS4_LOCK_STID)
-			continue;
-		if (lst->st_stid.sc_file == fp) {
-			atomic_inc(&lst->st_stid.sc_count);
-			return lst;
+	spin_unlock(&clp->cl_lock);
+	if (retstp) {
+		if (nfsd4_lock_ol_stateid(retstp) != nfs_ok) {
+			nfs4_put_stid(&retstp->st_stid);
+			goto retry;
 		}
+		/* To keep mutex tracking happy */
+		mutex_unlock(&stp->st_mutex);
+		stp = retstp;
 	}
-	return NULL;
+	return stp;
 }
 
 static struct nfs4_ol_stateid *
@@ -5684,26 +5702,25 @@ find_or_create_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fi,
 	struct nfs4_openowner *oo = openowner(ost->st_stateowner);
 	struct nfs4_client *clp = oo->oo_owner.so_client;
 
+	*new = false;
 	spin_lock(&clp->cl_lock);
 	lst = find_lock_stateid(lo, fi);
-	if (lst == NULL) {
-		spin_unlock(&clp->cl_lock);
-		ns = nfs4_alloc_stid(clp, stateid_slab, nfs4_free_lock_stateid);
-		if (ns == NULL)
-			return NULL;
-
-		spin_lock(&clp->cl_lock);
-		lst = find_lock_stateid(lo, fi);
-		if (likely(!lst)) {
-			lst = openlockstateid(ns);
-			init_lock_stateid(lst, lo, fi, inode, ost);
-			ns = NULL;
-			*new = true;
-		}
-	}
 	spin_unlock(&clp->cl_lock);
-	if (ns)
+	if (lst != NULL) {
+		if (nfsd4_lock_ol_stateid(lst) == nfs_ok)
+			goto out;
+		nfs4_put_stid(&lst->st_stid);
+	}
+	ns = nfs4_alloc_stid(clp, stateid_slab, nfs4_free_lock_stateid);
+	if (ns == NULL)
+		return NULL;
+
+	lst = init_lock_stateid(openlockstateid(ns), lo, fi, inode, ost);
+	if (lst == openlockstateid(ns))
+		*new = true;
+	else
 		nfs4_put_stid(ns);
+out:
 	return lst;
 }
 
@@ -5755,17 +5772,12 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 			goto out;
 	}
 
-retry:
 	lst = find_or_create_lock_stateid(lo, fi, inode, ost, new);
 	if (lst == NULL) {
 		status = nfserr_jukebox;
 		goto out;
 	}
 
-	if (nfsd4_lock_ol_stateid(lst) != nfs_ok) {
-		nfs4_put_stid(&lst->st_stid);
-		goto retry;
-	}
 	status = nfs_ok;
 	*plst = lst;
 out:
@@ -5971,14 +5983,16 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		    seqid_mutating_err(ntohl(status)))
 			lock_sop->lo_owner.so_seqid++;
 
-		mutex_unlock(&lock_stp->st_mutex);
-
 		/*
 		 * If this is a new, never-before-used stateid, and we are
 		 * returning an error, then just go ahead and release it.
 		 */
-		if (status && new)
+		if (status && new) {
+			lock_stp->st_stid.sc_type = NFS4_CLOSED_STID;
 			release_lock_stateid(lock_stp);
+		}
+
+		mutex_unlock(&lock_stp->st_mutex);
 
 		nfs4_put_stid(&lock_stp->st_stid);
 	}
-- 
2.13.6

[PATCH v2 6/7] nfsd: Ensure we check stateid validity in the seqid operation checks

[Trond Myklebust]

After taking the stateid st_mutex, we want to know that the stateid
still represents valid state before performing any non-idempotent
actions.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfs4state.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f13fba4f51ec..cbd6a10ddda2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5161,15 +5161,9 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
 	status = nfsd4_check_seqid(cstate, sop, seqid);
 	if (status)
 		return status;
-	if (stp->st_stid.sc_type == NFS4_CLOSED_STID
-		|| stp->st_stid.sc_type == NFS4_REVOKED_DELEG_STID)
-		/*
-		 * "Closed" stateid's exist *only* to return
-		 * nfserr_replay_me from the previous step, and
-		 * revoked delegations are kept only for free_stateid.
-		 */
-		return nfserr_bad_stateid;
-	mutex_lock(&stp->st_mutex);
+	status = nfsd4_lock_ol_stateid(stp);
+	if (status != nfs_ok)
+		return status;
 	status = check_stateid_generation(stateid, &stp->st_stid.sc_stateid, nfsd4_has_session(cstate));
 	if (status == nfs_ok)
 		status = nfs4_check_fh(current_fh, &stp->st_stid);
-- 
2.13.6

[PATCH v2 7/7] nfsd: Fix races with check_stateid_generation()

[Trond Myklebust]

The various functions that call check_stateid_generation() in order
to compare a client-supplied stateid with the nfs4_stid state, usually
need to atomically check for closed state. Those that perform the
check after locking the st_mutex using nfsd4_lock_ol_stateid()
should now be OK, but we do want to fix up the others.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfs4state.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cbd6a10ddda2..6f552ca62415 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4849,6 +4849,18 @@ static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_s
 	return nfserr_old_stateid;
 }
 
+static __be32 nfsd4_stid_check_stateid_generation(stateid_t *in, struct nfs4_stid *s, bool has_session)
+{
+	__be32 ret;
+
+	spin_lock(&s->sc_lock);
+	ret = nfsd4_verify_open_stid(s);
+	if (ret == nfs_ok)
+		ret = check_stateid_generation(in, &s->sc_stateid, has_session);
+	spin_unlock(&s->sc_lock);
+	return ret;
+}
+
 static __be32 nfsd4_check_openowner_confirmed(struct nfs4_ol_stateid *ols)
 {
 	if (ols->st_stateowner->so_is_open_owner &&
@@ -4877,7 +4889,7 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 	s = find_stateid_locked(cl, stateid);
 	if (!s)
 		goto out_unlock;
-	status = check_stateid_generation(stateid, &s->sc_stateid, 1);
+	status = nfsd4_stid_check_stateid_generation(stateid, s, 1);
 	if (status)
 		goto out_unlock;
 	switch (s->sc_type) {
@@ -5022,7 +5034,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 				&s, nn);
 	if (status)
 		return status;
-	status = check_stateid_generation(stateid, &s->sc_stateid,
+	status = nfsd4_stid_check_stateid_generation(stateid, s,
 			nfsd4_has_session(cstate));
 	if (status)
 		goto out;
@@ -5115,6 +5127,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	s = find_stateid_locked(cl, stateid);
 	if (!s)
 		goto out_unlock;
+	spin_lock(&s->sc_lock);
 	switch (s->sc_type) {
 	case NFS4_DELEG_STID:
 		ret = nfserr_locks_held;
@@ -5126,11 +5139,13 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		ret = nfserr_locks_held;
 		break;
 	case NFS4_LOCK_STID:
+		spin_unlock(&s->sc_lock);
 		atomic_inc(&s->sc_count);
 		spin_unlock(&cl->cl_lock);
 		ret = nfsd4_free_lock_stateid(stateid, s);
 		goto out;
 	case NFS4_REVOKED_DELEG_STID:
+		spin_unlock(&s->sc_lock);
 		dp = delegstateid(s);
 		list_del_init(&dp->dl_recall_lru);
 		spin_unlock(&cl->cl_lock);
@@ -5139,6 +5154,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	/* Default falls through and returns nfserr_bad_stateid */
 	}
+	spin_unlock(&s->sc_lock);
 out_unlock:
 	spin_unlock(&cl->cl_lock);
 out:
@@ -5418,7 +5434,7 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		goto out;
 	dp = delegstateid(s);
-	status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
+	status = nfsd4_stid_check_stateid_generation(stateid, &dp->dl_stid, nfsd4_has_session(cstate));
 	if (status)
 		goto put_stateid;
 
-- 
2.13.6

Re: [PATCH v2 5/7] nfsd: Fix race in lock stateid creation

[J. Bruce Fields]

On Fri, Nov 03, 2017 at 08:00:14AM -0400, Trond Myklebust wrote:
> If we're looking up a new lock state, and the creation fails, then
> we want to unhash it, just like we do for OPEN. However in order
> to do so, we need to that no other LOCK requests can grab the
> mutex until we have unhashed it (and marked it as closed).

I split the move of find_lock_stateid() into a separate patch just to
make it easier for me to read the result....  Looks fine otherwise.

--b.

> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 104 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 59 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f93ca0682aaa..f13fba4f51ec 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5630,14 +5630,41 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
>  	return ret;
>  }
>  
> -static void
> +static struct nfs4_ol_stateid *
> +find_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp)
> +{
> +	struct nfs4_ol_stateid *lst;
> +	struct nfs4_client *clp = lo->lo_owner.so_client;
> +
> +	lockdep_assert_held(&clp->cl_lock);
> +
> +	list_for_each_entry(lst, &lo->lo_owner.so_stateids, st_perstateowner) {
> +		if (lst->st_stid.sc_type != NFS4_LOCK_STID)
> +			continue;
> +		if (lst->st_stid.sc_file == fp) {
> +			atomic_inc(&lst->st_stid.sc_count);
> +			return lst;
> +		}
> +	}
> +	return NULL;
> +}
> +
> +static struct nfs4_ol_stateid *
>  init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
>  		  struct nfs4_file *fp, struct inode *inode,
>  		  struct nfs4_ol_stateid *open_stp)
>  {
>  	struct nfs4_client *clp = lo->lo_owner.so_client;
> +	struct nfs4_ol_stateid *retstp;
>  
> -	lockdep_assert_held(&clp->cl_lock);
> +	mutex_init(&stp->st_mutex);
> +	mutex_lock(&stp->st_mutex);
> +retry:
> +	spin_lock(&clp->cl_lock);
> +	spin_lock(&fp->fi_lock);
> +	retstp = find_lock_stateid(lo, fp);
> +	if (retstp)
> +		goto out_unlock;
>  
>  	atomic_inc(&stp->st_stid.sc_count);
>  	stp->st_stid.sc_type = NFS4_LOCK_STID;
> @@ -5647,31 +5674,22 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
>  	stp->st_access_bmap = 0;
>  	stp->st_deny_bmap = open_stp->st_deny_bmap;
>  	stp->st_openstp = open_stp;
> -	mutex_init(&stp->st_mutex);
>  	list_add(&stp->st_locks, &open_stp->st_locks);
>  	list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
> -	spin_lock(&fp->fi_lock);
>  	list_add(&stp->st_perfile, &fp->fi_stateids);
> +out_unlock:
>  	spin_unlock(&fp->fi_lock);
> -}
> -
> -static struct nfs4_ol_stateid *
> -find_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp)
> -{
> -	struct nfs4_ol_stateid *lst;
> -	struct nfs4_client *clp = lo->lo_owner.so_client;
> -
> -	lockdep_assert_held(&clp->cl_lock);
> -
> -	list_for_each_entry(lst, &lo->lo_owner.so_stateids, st_perstateowner) {
> -		if (lst->st_stid.sc_type != NFS4_LOCK_STID)
> -			continue;
> -		if (lst->st_stid.sc_file == fp) {
> -			atomic_inc(&lst->st_stid.sc_count);
> -			return lst;
> +	spin_unlock(&clp->cl_lock);
> +	if (retstp) {
> +		if (nfsd4_lock_ol_stateid(retstp) != nfs_ok) {
> +			nfs4_put_stid(&retstp->st_stid);
> +			goto retry;
>  		}
> +		/* To keep mutex tracking happy */
> +		mutex_unlock(&stp->st_mutex);
> +		stp = retstp;
>  	}
> -	return NULL;
> +	return stp;
>  }
>  
>  static struct nfs4_ol_stateid *
> @@ -5684,26 +5702,25 @@ find_or_create_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fi,
>  	struct nfs4_openowner *oo = openowner(ost->st_stateowner);
>  	struct nfs4_client *clp = oo->oo_owner.so_client;
>  
> +	*new = false;
>  	spin_lock(&clp->cl_lock);
>  	lst = find_lock_stateid(lo, fi);
> -	if (lst == NULL) {
> -		spin_unlock(&clp->cl_lock);
> -		ns = nfs4_alloc_stid(clp, stateid_slab, nfs4_free_lock_stateid);
> -		if (ns == NULL)
> -			return NULL;
> -
> -		spin_lock(&clp->cl_lock);
> -		lst = find_lock_stateid(lo, fi);
> -		if (likely(!lst)) {
> -			lst = openlockstateid(ns);
> -			init_lock_stateid(lst, lo, fi, inode, ost);
> -			ns = NULL;
> -			*new = true;
> -		}
> -	}
>  	spin_unlock(&clp->cl_lock);
> -	if (ns)
> +	if (lst != NULL) {
> +		if (nfsd4_lock_ol_stateid(lst) == nfs_ok)
> +			goto out;
> +		nfs4_put_stid(&lst->st_stid);
> +	}
> +	ns = nfs4_alloc_stid(clp, stateid_slab, nfs4_free_lock_stateid);
> +	if (ns == NULL)
> +		return NULL;
> +
> +	lst = init_lock_stateid(openlockstateid(ns), lo, fi, inode, ost);
> +	if (lst == openlockstateid(ns))
> +		*new = true;
> +	else
>  		nfs4_put_stid(ns);
> +out:
>  	return lst;
>  }
>  
> @@ -5755,17 +5772,12 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
>  			goto out;
>  	}
>  
> -retry:
>  	lst = find_or_create_lock_stateid(lo, fi, inode, ost, new);
>  	if (lst == NULL) {
>  		status = nfserr_jukebox;
>  		goto out;
>  	}
>  
> -	if (nfsd4_lock_ol_stateid(lst) != nfs_ok) {
> -		nfs4_put_stid(&lst->st_stid);
> -		goto retry;
> -	}
>  	status = nfs_ok;
>  	*plst = lst;
>  out:
> @@ -5971,14 +5983,16 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		    seqid_mutating_err(ntohl(status)))
>  			lock_sop->lo_owner.so_seqid++;
>  
> -		mutex_unlock(&lock_stp->st_mutex);
> -
>  		/*
>  		 * If this is a new, never-before-used stateid, and we are
>  		 * returning an error, then just go ahead and release it.
>  		 */
> -		if (status && new)
> +		if (status && new) {
> +			lock_stp->st_stid.sc_type = NFS4_CLOSED_STID;
>  			release_lock_stateid(lock_stp);
> +		}
> +
> +		mutex_unlock(&lock_stp->st_mutex);
>  
>  		nfs4_put_stid(&lock_stp->st_stid);
>  	}
> -- 
> 2.13.6

Re: [PATCH v2 0/7] Tighten up locking around stateids in knfsd

[J. Bruce Fields]

What tree are these patches against?

--b.

On Fri, Nov 03, 2017 at 08:00:09AM -0400, Trond Myklebust wrote:
> This patchset aims to tighten up the locking rules around stateids to
> ensure that knfsd does not reuse stateids that have already been closed
> or invalidated. The aim is to ensure we enforce the RFC5661 and RFC7530
> rules concerning stateid initialisation and updates.
> 
> v2:
> - Add a fix for when nfs4_get_vfs_file() fails in nfsd4_process_open2()
> - Add a fix for byte range lock creation
> 
> Note that byte range locks are not completely fixed. The remaining task
> of ensuring that locks don't conflict with CLOSE has been left as an
> exercise for the reviewer.
> 
> Trond Myklebust (7):
>   nfsd: Fix stateid races between OPEN and CLOSE
>   nfsd: Fix another OPEN stateid race
>   nfsd: CLOSE SHOULD return the invalid special stateid for NFSv4.x
>     (x>0)
>   nfsd: Ensure we don't recognise lock stateids after freeing them
>   nfsd: Fix race in lock stateid creation
>   nfsd: Ensure we check stateid validity in the seqid operation checks
>   nfsd: Fix races with check_stateid_generation()
> 
>  fs/nfsd/nfs4state.c | 254 ++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 166 insertions(+), 88 deletions(-)
> 
> -- 
> 2.13.6

Re: [PATCH v2 0/7] Tighten up locking around stateids in knfsd

[J. Bruce Fields]

On Tue, Nov 07, 2017 at 03:06:16PM -0500, J. Bruce Fields wrote:
> What tree are these patches against?

Never mind, there were just some minor conflicts against the refcount
patches.

--b.

Re: [PATCH v2 0/7] Tighten up locking around stateids in knfsd

[J. Bruce Fields]

On Tue, Nov 07, 2017 at 03:47:02PM -0500, J. Bruce Fields wrote:
> On Tue, Nov 07, 2017 at 03:06:16PM -0500, J. Bruce Fields wrote:
> > What tree are these patches against?
> 
> Never mind, there were just some minor conflicts against the refcount
> patches.

Applying the series for 4.15, thanks.--b.