Re: [PATCH] nfsd4: Prevent the reuse of a closed stateid

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Yes, I believe it has, though I haven't tested it yet, unfortunately.

On 10 Nov 2017, at 17:03, J. Bruce Fields wrote:

I'm assuming this has been addressed by Trond's series (in linux-next
now); but I haven't checked carefully....

--b.

On Tue, Oct 17, 2017 at 09:55:05AM -0400, Benjamin Coddington wrote:
While running generic/089 on NFSv4.1, the following on-the-wire exchange
occurs:

Client                  Server
----------              ----------
OPEN (owner A)  ->
                    <-  OPEN response: state A1
CLOSE (state A1)->
OPEN (owner A)  ->
                    <-  CLOSE response: state A2
                    <-  OPEN response: state A3
LOCK (state A3) ->
                    <-  LOCK response: NFS4_BAD_STATEID

The server should not be returning state A3 in response to the second OPEN after processing the CLOSE and sending back state A2. The problem is that nfsd4_process_open2() is able to find an existing open state just after
nfsd4_close() has incremented the state's sequence number but before
calling unhash_ol_stateid().

Fix this by using the state's sc_type field to identify closed state, and protect the update of that field with st_mutex. nfsd4_find_existing_open() will return with the st_mutex held, so that the state will not transition
between being looked up and then updated in nfsd4_process_open2().

Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
---
 fs/nfsd/nfs4state.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0c04f81aa63b..17473a092d01 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3503,11 +3503,12 @@ static const struct nfs4_stateowner_operations openowner_ops = {
 static struct nfs4_ol_stateid *
nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
 {
-	struct nfs4_ol_stateid *local, *ret = NULL;
+	struct nfs4_ol_stateid *local, *ret;
 	struct nfs4_openowner *oo = open->op_openowner;

-	lockdep_assert_held(&fp->fi_lock);
-
+retry:
+	ret = NULL;
+	spin_lock(&fp->fi_lock);
 	list_for_each_entry(local, &fp->fi_stateids, st_perfile) {
 		/* ignore lock owners */
 		if (local->st_stateowner->so_is_open_owner == 0)
@@ -3518,6 +3519,18 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
 			break;
 		}
 	}
+	spin_unlock(&fp->fi_lock);
+
+	/* Did we race with CLOSE? */
+	if (ret) {
+		mutex_lock(&ret->st_mutex);
+		if (ret->st_stid.sc_type != NFS4_OPEN_STID) {
+			mutex_unlock(&ret->st_mutex);
+			nfs4_put_stid(&ret->st_stid);
+			goto retry;
+		}
+	}
+
 	return ret;
 }

@@ -3566,7 +3579,6 @@ init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
 	mutex_lock(&stp->st_mutex);

 	spin_lock(&oo->oo_owner.so_client->cl_lock);
-	spin_lock(&fp->fi_lock);

 	retstp = nfsd4_find_existing_open(fp, open);
 	if (retstp)
@@ -3583,13 +3595,13 @@ init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
 	stp->st_deny_bmap = 0;
 	stp->st_openstp = NULL;
 	list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
+	spin_lock(&fp->fi_lock);
 	list_add(&stp->st_perfile, &fp->fi_stateids);
+	spin_unlock(&fp->fi_lock);

 out_unlock:
-	spin_unlock(&fp->fi_lock);
 	spin_unlock(&oo->oo_owner.so_client->cl_lock);
 	if (retstp) {
-		mutex_lock(&retstp->st_mutex);
 		/* To keep mutex tracking happy */
 		mutex_unlock(&stp->st_mutex);
 		stp = retstp;
@@ -4403,9 +4415,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);
 	} else {
 		open->op_file = NULL;
 		status = nfserr_bad_stateid;
@@ -4419,7 +4429,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 +5303,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);

@@ -5335,6 +5343,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		goto out;
 	nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
+	stp->st_stid.sc_type = NFS4_CLOSED_STID;
 	mutex_unlock(&stp->st_mutex);

 	nfsd4_close_open_stateid(stp);
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux