[PATCH v4 0/2] Eliminate race between LOCK and FREE_STATEID

[PATCH v4 0/2] Eliminate race between LOCK and FREE_STATEID

[Chuck Lever]

Hi Bruce-

IMO this series is both safe and effective and thus is ready to merge.

Alexey reports:

@ All 500 nfslock test runs passed, also run other related NFS tests with 
@ different configurations, as a result they passed as well. 


Changes since v3:
- Add Tested-by tags

Changes since v2:
- Move NFS4_LOCK_STID arm into a helper, for clarity
- Add more detail to patch description
- Add Jeff's patch to fix similar race in nfsd4_lock

Changes since v1:
- Use s->sc_count to preserve stateid while cl_lock is dropped

---

Chuck Lever (1):
      nfsd: Fix race between FREE_STATEID and LOCK

Jeff Layton (1):
      nfsd: don't return an unhashed lock stateid after taking mutex


 fs/nfsd/nfs4state.c |   65 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 17 deletions(-)

--
Chuck Lever

[PATCH v4 1/2] nfsd: Fix race between FREE_STATEID and LOCK

[Chuck Lever]

When running LTP's nfslock01 test, the Linux client can send a LOCK
and a FREE_STATEID request at the same time. The outcome is:

Frame 324    R OPEN stateid [2,O]

Frame 115004 C LOCK lockowner_is_new stateid [2,O] offset 672000 len 64
Frame 115008 R LOCK stateid [1,L]
Frame 115012 C WRITE stateid [0,L] offset 672000 len 64
Frame 115016 R WRITE NFS4_OK
Frame 115019 C LOCKU stateid [1,L] offset 672000 len 64
Frame 115022 R LOCKU NFS4_OK
Frame 115025 C FREE_STATEID stateid [2,L]
Frame 115026 C LOCK lockowner_is_new stateid [2,O] offset 672128 len 64
Frame 115029 R FREE_STATEID NFS4_OK
Frame 115030 R LOCK stateid [3,L]
Frame 115034 C WRITE stateid [0,L] offset 672128 len 64
Frame 115038 R WRITE NFS4ERR_BAD_STATEID

In other words, the server returns stateid L in a successful LOCK
reply, but it has already released it. Subsequent uses of stateid L
fail.

To address this, protect the generation check in nfsd4_free_stateid
with the st_mutex. This should guarantee that only one of two
outcomes occurs: either LOCK returns a fresh valid stateid, or
FREE_STATEID returns NFS4ERR_LOCKS_HELD.

Reported-by: Alexey Kodanev <alexey.kodanev@oracle.com>
Fix-suggested-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Tested-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 fs/nfsd/nfs4state.c |   40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b921123..de868fe 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4885,6 +4885,32 @@ nfsd4_test_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	return nfs_ok;
 }
 
+static __be32
+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 = check_stateid_generation(stateid, &s->sc_stateid, 1);
+	if (ret)
+		goto out;
+
+	ret = nfserr_locks_held;
+	if (check_for_locks(stp->st_stid.sc_file,
+			    lockowner(stp->st_stateowner)))
+		goto out;
+
+	release_lock_stateid(stp);
+	ret = nfs_ok;
+
+out:
+	mutex_unlock(&stp->st_mutex);
+	nfs4_put_stid(s);
+	return ret;
+}
+
 __be32
 nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		   struct nfsd4_free_stateid *free_stateid)
@@ -4892,7 +4918,6 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	stateid_t *stateid = &free_stateid->fr_stateid;
 	struct nfs4_stid *s;
 	struct nfs4_delegation *dp;
-	struct nfs4_ol_stateid *stp;
 	struct nfs4_client *cl = cstate->session->se_client;
 	__be32 ret = nfserr_bad_stateid;
 
@@ -4911,18 +4936,9 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		ret = nfserr_locks_held;
 		break;
 	case NFS4_LOCK_STID:
-		ret = check_stateid_generation(stateid, &s->sc_stateid, 1);
-		if (ret)
-			break;
-		stp = openlockstateid(s);
-		ret = nfserr_locks_held;
-		if (check_for_locks(stp->st_stid.sc_file,
-				    lockowner(stp->st_stateowner)))
-			break;
-		WARN_ON(!unhash_lock_stateid(stp));
+		atomic_inc(&s->sc_count);
 		spin_unlock(&cl->cl_lock);
-		nfs4_put_stid(s);
-		ret = nfs_ok;
+		ret = nfsd4_free_lock_stateid(stateid, s);
 		goto out;
 	case NFS4_REVOKED_DELEG_STID:
 		dp = delegstateid(s);

[PATCH v4 2/2] nfsd: don't return an unhashed lock stateid after taking mutex

[Chuck Lever]

From: Jeff Layton <jlayton@redhat.com>

nfsd4_lock will take the st_mutex before working with the stateid it
gets, but between the time when we drop the cl_lock and take the mutex,
the stateid could become unhashed (a'la FREE_STATEID). If that happens
the lock stateid returned to the client will be forgotten.

Fix this by first moving the st_mutex acquisition into
lookup_or_create_lock_state. Then, have it check to see if the lock
stateid is still hashed after taking the mutex. If it's not, then put
the stateid and try the find/create again.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Tested-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 fs/nfsd/nfs4state.c |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index de868fe..6a23098 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5505,7 +5505,7 @@ static __be32
 lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 			    struct nfs4_ol_stateid *ost,
 			    struct nfsd4_lock *lock,
-			    struct nfs4_ol_stateid **lst, bool *new)
+			    struct nfs4_ol_stateid **plst, bool *new)
 {
 	__be32 status;
 	struct nfs4_file *fi = ost->st_stid.sc_file;
@@ -5513,7 +5513,9 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 	struct nfs4_client *cl = oo->oo_owner.so_client;
 	struct inode *inode = d_inode(cstate->current_fh.fh_dentry);
 	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) {
@@ -5529,12 +5531,27 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 			goto out;
 	}
 
-	*lst = find_or_create_lock_stateid(lo, fi, inode, ost, new);
-	if (*lst == NULL) {
+retry:
+	lst = find_or_create_lock_stateid(lo, fi, inode, ost, new);
+	if (lst == NULL) {
 		status = nfserr_jukebox;
 		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);
+		nfs4_put_stid(&lst->st_stid);
+		goto retry;
+	}
 	status = nfs_ok;
+	*plst = lst;
 out:
 	nfs4_put_stateowner(&lo->lo_owner);
 	return status;
@@ -5601,8 +5618,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			goto out;
 		status = lookup_or_create_lock_state(cstate, open_stp, lock,
 							&lock_stp, &new);
-		if (status == nfs_ok)
-			mutex_lock(&lock_stp->st_mutex);
 	} else {
 		status = nfs4_preprocess_seqid_op(cstate,
 				       lock->lk_old_lock_seqid,

Re: [PATCH v4 0/2] Eliminate race between LOCK and FREE_STATEID

[J. Bruce Fields]

On Thu, Aug 11, 2016 at 10:37:21AM -0400, Chuck Lever wrote:
> Hi Bruce-
> 
> IMO this series is both safe and effective and thus is ready to merge.

Thanks for the care taken with this series....

I'll queue it up for 4.8 and stable, unless there's objections.

--b.

> 
> Alexey reports:
> 
> @ All 500 nfslock test runs passed, also run other related NFS tests with 
> @ different configurations, as a result they passed as well. 
> 
> 
> Changes since v3:
> - Add Tested-by tags
> 
> Changes since v2:
> - Move NFS4_LOCK_STID arm into a helper, for clarity
> - Add more detail to patch description
> - Add Jeff's patch to fix similar race in nfsd4_lock
> 
> Changes since v1:
> - Use s->sc_count to preserve stateid while cl_lock is dropped
> 
> ---
> 
> Chuck Lever (1):
>       nfsd: Fix race between FREE_STATEID and LOCK
> 
> Jeff Layton (1):
>       nfsd: don't return an unhashed lock stateid after taking mutex
> 
> 
>  fs/nfsd/nfs4state.c |   65 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 17 deletions(-)
> 
> --
> Chuck Lever

Re: [PATCH v4 0/2] Eliminate race between LOCK and FREE_STATEID

[Chuck Lever]

> On Aug 11, 2016, at 3:57 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Thu, Aug 11, 2016 at 10:37:21AM -0400, Chuck Lever wrote:
>> Hi Bruce-
>> 
>> IMO this series is both safe and effective and thus is ready to merge.
> 
> Thanks for the care taken with this series....
> 
> I'll queue it up for 4.8 and stable, unless there's objections.

Excellent, thanks.

Re: stable: Alexey tells me that: v4.7-rc4 commit feb9dad5
("nfsd: Always lock state  exclusively.") is required for this
series to apply successfully to v4.1-stable.


> --b.
> 
>> 
>> Alexey reports:
>> 
>> @ All 500 nfslock test runs passed, also run other related NFS tests with 
>> @ different configurations, as a result they passed as well. 
>> 
>> 
>> Changes since v3:
>> - Add Tested-by tags
>> 
>> Changes since v2:
>> - Move NFS4_LOCK_STID arm into a helper, for clarity
>> - Add more detail to patch description
>> - Add Jeff's patch to fix similar race in nfsd4_lock
>> 
>> Changes since v1:
>> - Use s->sc_count to preserve stateid while cl_lock is dropped
>> 
>> ---
>> 
>> Chuck Lever (1):
>>      nfsd: Fix race between FREE_STATEID and LOCK
>> 
>> Jeff Layton (1):
>>      nfsd: don't return an unhashed lock stateid after taking mutex
>> 
>> 
>> fs/nfsd/nfs4state.c |   65 ++++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 48 insertions(+), 17 deletions(-)
>> 
>> --
>> Chuck Lever
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever