[PATCH 00/14] Delegation bugfixes

[PATCH 00/14] Delegation bugfixes

[Trond Myklebust]

The following patchset fixes up a number of issues with delegations,
but mainly attempts to fix a race condition between open and
delegreturn, where the open and the delegreturn get re-ordered so
that the delegreturn ends up revoking the delegation that was returned
by the open.
The root cause is that in certain circumstances, we may currently end
up freeing the delegation from delegreturn, so when we later receive
the reply to the open, we've lost track of the fact that the seqid
predates the one that was returned.

This patchset fixes that case by ensuring that we always keep track
of the last delegation stateid that was returned for any given inode.
That way, if we later see a delegation stateid with the same opaque
field, but an older seqid, we know we cannot trust it, and so we
ask to replay the OPEN compound.

Trond Myklebust (14):
  NFSv4: Don't allow a cached open with a revoked delegation
  NFSv4: Fix delegation handling in update_open_stateid()
  NFSv4: nfs4_callback_getattr() should ignore revoked delegations
  NFSv4: Delegation recalls should not find revoked delegations
  NFSv4: fail nfs4_refresh_delegation_stateid() when the delegation was
    revoked
  NFS: Rename nfs_inode_return_delegation_noreclaim()
  NFSv4: Don't remove the delegation from the super_list more than once
  NFSv4: Hold the delegation spinlock when updating the seqid
  NFSv4: Clear the NFS_DELEGATION_REVOKED flag in
    nfs_update_inplace_delegation()
  NFSv4: Update the stateid seqid in nfs_revoke_delegation()
  NFSv4: Revoke the delegation on success in nfs4_delegreturn_done()
  NFSv4: Ignore requests to return the delegation if it was revoked
  NFSv4: Don't reclaim delegations that have been returned or revoked
  NFSv4: Fix races between open and delegreturn

 fs/nfs/callback_proc.c |   2 +-
 fs/nfs/delegation.c    | 109 +++++++++++++++++++++++++++++------------
 fs/nfs/delegation.h    |   4 +-
 fs/nfs/nfs4proc.c      |  13 ++---
 fs/nfs/nfs4super.c     |   4 +-
 5 files changed, 88 insertions(+), 44 deletions(-)

-- 
2.21.0

[PATCH 01/14] NFSv4: Don't allow a cached open with a revoked delegation

[Trond Myklebust]

If the delegation is marked as being revoked, we must not use it
for cached opens.

Fixes: 869f9dfa4d6d ("NFSv4: Fix races between nfs_remove_bad_delegation() and delegation return")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 3 +--
 fs/nfs/delegation.h | 1 +
 fs/nfs/nfs4proc.c   | 6 +-----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 071b90a45933..5f3eea926af5 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -42,8 +42,7 @@ void nfs_mark_delegation_referenced(struct nfs_delegation *delegation)
 	set_bit(NFS_DELEGATION_REFERENCED, &delegation->flags);
 }
 
-static bool
-nfs4_is_valid_delegation(const struct nfs_delegation *delegation,
+bool nfs4_is_valid_delegation(const struct nfs_delegation *delegation,
 		fmode_t flags)
 {
 	if (delegation != NULL && (delegation->type & flags) == flags &&
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index 9eb87ae4c982..2b35a99929a0 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -69,6 +69,7 @@ bool nfs4_copy_delegation_stateid(struct inode *inode, fmode_t flags, nfs4_state
 bool nfs4_refresh_delegation_stateid(nfs4_stateid *dst, struct inode *inode);
 
 void nfs_mark_delegation_referenced(struct nfs_delegation *delegation);
+bool nfs4_is_valid_delegation(const struct nfs_delegation *delegation, fmode_t flags);
 int nfs4_have_delegation(struct inode *inode, fmode_t flags);
 int nfs4_check_delegation(struct inode *inode, fmode_t flags);
 bool nfs4_delegation_flush_on_close(const struct inode *inode);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ab8ca20fd579..294ea8c1a163 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1436,11 +1436,7 @@ static int can_open_cached(struct nfs4_state *state, fmode_t mode,
 static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode,
 		enum open_claim_type4 claim)
 {
-	if (delegation == NULL)
-		return 0;
-	if ((delegation->type & fmode) != fmode)
-		return 0;
-	if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
+	if (!nfs4_is_valid_delegation(delegation, fmode))
 		return 0;
 	switch (claim) {
 	case NFS4_OPEN_CLAIM_NULL:
-- 
2.21.0

[PATCH 02/14] NFSv4: Fix delegation handling in update_open_stateid()

[Trond Myklebust]

If the delegation is marked as being revoked, then don't use it in
the open state structure.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs4proc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 294ea8c1a163..c407e2eed3d5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1740,14 +1740,12 @@ static int update_open_stateid(struct nfs4_state *state,
 		goto no_delegation;
 
 	spin_lock(&deleg_cur->lock);
-	if (rcu_dereference(nfsi->delegation) != deleg_cur ||
-	   test_bit(NFS_DELEGATION_RETURNING, &deleg_cur->flags) ||
-	    (deleg_cur->type & fmode) != fmode)
+	if (!nfs4_is_valid_delegation(deleg_cur, fmode))
 		goto no_delegation_unlock;
 
 	if (delegation == NULL)
 		delegation = &deleg_cur->stateid;
-	else if (!nfs4_stateid_match(&deleg_cur->stateid, delegation))
+	else if (!nfs4_stateid_match_other(&deleg_cur->stateid, delegation))
 		goto no_delegation_unlock;
 
 	nfs_mark_delegation_referenced(deleg_cur);
-- 
2.21.0

[PATCH 03/14] NFSv4: nfs4_callback_getattr() should ignore revoked delegations

[Trond Myklebust]

If the delegation has been revoked, ignore it.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/callback_proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index db3e7771e597..58a77c41ff36 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -50,7 +50,7 @@ __be32 nfs4_callback_getattr(void *argp, void *resp,
 	nfsi = NFS_I(inode);
 	rcu_read_lock();
 	delegation = rcu_dereference(nfsi->delegation);
-	if (delegation == NULL || (delegation->type & FMODE_WRITE) == 0)
+	if (!nfs4_is_valid_delegation(delegation, FMODE_WRITE))
 		goto out_iput;
 	res->size = i_size_read(inode);
 	res->change_attr = delegation->change_attr;
-- 
2.21.0

[PATCH 04/14] NFSv4: Delegation recalls should not find revoked delegations

[Trond Myklebust]

If we're processsing a delegation recall, ignore the delegations that
have already been revoked or returned.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5f3eea926af5..4bc40c27141b 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -830,7 +830,7 @@ int nfs_async_inode_return_delegation(struct inode *inode,
 
 	rcu_read_lock();
 	delegation = rcu_dereference(NFS_I(inode)->delegation);
-	if (delegation == NULL)
+	if (!nfs4_is_valid_delegation(delegation, FMODE_READ))
 		goto out_enoent;
 	if (stateid != NULL &&
 	    !clp->cl_mvops->match_stateid(&delegation->stateid, stateid))
@@ -855,6 +855,7 @@ nfs_delegation_find_inode_server(struct nfs_server *server,
 	list_for_each_entry_rcu(delegation, &server->delegations, super_list) {
 		spin_lock(&delegation->lock);
 		if (delegation->inode != NULL &&
+		    !test_bit(NFS_DELEGATION_REVOKED, &delegation->flags) &&
 		    nfs_compare_fh(fhandle, &NFS_I(delegation->inode)->fh) == 0) {
 			freeme = igrab(delegation->inode);
 			if (freeme && nfs_sb_active(freeme->i_sb))
-- 
2.21.0

[PATCH 05/14] NFSv4: fail nfs4_refresh_delegation_stateid() when the delegation was revoked

[Trond Myklebust]

If the delegation was revoked, we don't want to retry the delegreturn.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 4bc40c27141b..f90c3cf82f8f 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -1179,6 +1179,7 @@ bool nfs4_refresh_delegation_stateid(nfs4_stateid *dst, struct inode *inode)
 	rcu_read_lock();
 	delegation = rcu_dereference(NFS_I(inode)->delegation);
 	if (delegation != NULL &&
+	    !test_bit(NFS_DELEGATION_REVOKED, &delegation->flags) &&
 	    nfs4_stateid_match_other(dst, &delegation->stateid)) {
 		dst->seqid = delegation->stateid.seqid;
 		return ret;
-- 
2.21.0

[PATCH 06/14] NFS: Rename nfs_inode_return_delegation_noreclaim()

[Trond Myklebust]

Rename nfs_inode_return_delegation_noreclaim() to
nfs_inode_evict_delegation(), which better describes what it
does.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 11 +++++++----
 fs/nfs/delegation.h |  2 +-
 fs/nfs/nfs4super.c  |  4 ++--
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index f90c3cf82f8f..e60737be6f26 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -574,19 +574,22 @@ int nfs_client_return_marked_delegations(struct nfs_client *clp)
 }
 
 /**
- * nfs_inode_return_delegation_noreclaim - return delegation, don't reclaim opens
+ * nfs_inode_evict_delegation - return delegation, don't reclaim opens
  * @inode: inode to process
  *
  * Does not protect against delegation reclaims, therefore really only safe
- * to be called from nfs4_clear_inode().
+ * to be called from nfs4_clear_inode(). Guaranteed to always free
+ * the delegation structure.
  */
-void nfs_inode_return_delegation_noreclaim(struct inode *inode)
+void nfs_inode_evict_delegation(struct inode *inode)
 {
 	struct nfs_delegation *delegation;
 
 	delegation = nfs_inode_detach_delegation(inode);
-	if (delegation != NULL)
+	if (delegation != NULL) {
+		set_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags);
 		nfs_do_return_delegation(inode, delegation, 1);
+	}
 }
 
 /**
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index 2b35a99929a0..9a14a7ca1df9 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -43,7 +43,7 @@ void nfs_inode_reclaim_delegation(struct inode *inode, const struct cred *cred,
 		fmode_t type, const nfs4_stateid *stateid, unsigned long pagemod_limit);
 int nfs4_inode_return_delegation(struct inode *inode);
 int nfs_async_inode_return_delegation(struct inode *inode, const nfs4_stateid *stateid);
-void nfs_inode_return_delegation_noreclaim(struct inode *inode);
+void nfs_inode_evict_delegation(struct inode *inode);
 
 struct inode *nfs_delegation_find_inode(struct nfs_client *clp, const struct nfs_fh *fhandle);
 void nfs_server_return_all_delegations(struct nfs_server *);
diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
index 04c57066a11a..2c9cbade561a 100644
--- a/fs/nfs/nfs4super.c
+++ b/fs/nfs/nfs4super.c
@@ -92,8 +92,8 @@ static void nfs4_evict_inode(struct inode *inode)
 {
 	truncate_inode_pages_final(&inode->i_data);
 	clear_inode(inode);
-	/* If we are holding a delegation, return it! */
-	nfs_inode_return_delegation_noreclaim(inode);
+	/* If we are holding a delegation, return and free it */
+	nfs_inode_evict_delegation(inode);
 	/* Note that above delegreturn would trigger pnfs return-on-close */
 	pnfs_return_layout(inode);
 	pnfs_destroy_layout(NFS_I(inode));
-- 
2.21.0

[PATCH 07/14] NFSv4: Don't remove the delegation from the super_list more than once

[Trond Myklebust]

Add a check to ensure that we haven't already removed the delegation
from the inode after we take all the relevant locks.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index e60737be6f26..b49faf1b3d91 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -287,6 +287,10 @@ nfs_detach_delegation_locked(struct nfs_inode *nfsi,
 		return NULL;
 
 	spin_lock(&delegation->lock);
+	if (!delegation->inode) {
+		spin_unlock(&delegation->lock);
+		return NULL;
+	}
 	set_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
 	list_del_rcu(&delegation->super_list);
 	delegation->inode = NULL;
-- 
2.21.0

[PATCH 08/14] NFSv4: Hold the delegation spinlock when updating the seqid

[Trond Myklebust]

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index b49faf1b3d91..d9c80871cecc 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -376,8 +376,10 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 		/* Is this an update of the existing delegation? */
 		if (nfs4_stateid_match_other(&old_delegation->stateid,
 					&delegation->stateid)) {
+			spin_lock(&old_delegation->lock);
 			nfs_update_inplace_delegation(old_delegation,
 					delegation);
+			spin_unlock(&old_delegation->lock);
 			goto out;
 		}
 		/*
-- 
2.21.0

[PATCH 09/14] NFSv4: Clear the NFS_DELEGATION_REVOKED flag in nfs_update_inplace_delegation()

[Trond Myklebust]

If the server sent us a new delegation stateid that is more recent than
the one that got revoked, then clear the NFS_DELEGATION_REVOKED flag.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index d9c80871cecc..b6cc2e71fcdb 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -332,6 +332,7 @@ nfs_update_inplace_delegation(struct nfs_delegation *delegation,
 		delegation->stateid.seqid = update->stateid.seqid;
 		smp_wmb();
 		delegation->type = update->type;
+		clear_bit(NFS_DELEGATION_REVOKED, &delegation->flags);
 	}
 }
 
-- 
2.21.0

[PATCH 10/14] NFSv4: Update the stateid seqid in nfs_revoke_delegation()

[Trond Myklebust]

If we revoke a delegation, but the stateid's seqid is newer, then
ensure we update the seqid when marking the delegation as revoked.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index b6cc2e71fcdb..5a87d32a8d7c 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -760,8 +760,19 @@ static bool nfs_revoke_delegation(struct inode *inode,
 	if (stateid == NULL) {
 		nfs4_stateid_copy(&tmp, &delegation->stateid);
 		stateid = &tmp;
-	} else if (!nfs4_stateid_match(stateid, &delegation->stateid))
-		goto out;
+	} else {
+		if (!nfs4_stateid_match_other(stateid, &delegation->stateid))
+			goto out;
+		spin_lock(&delegation->lock);
+		if (stateid->seqid) {
+			if (nfs4_stateid_is_newer(&delegation->stateid, stateid)) {
+				spin_unlock(&delegation->lock);
+				goto out;
+			}
+			delegation->stateid.seqid = stateid->seqid;
+		}
+		spin_unlock(&delegation->lock);
+	}
 	nfs_mark_delegation_revoked(NFS_SERVER(inode), delegation);
 	ret = true;
 out:
-- 
2.21.0

[PATCH 11/14] NFSv4: Revoke the delegation on success in nfs4_delegreturn_done()

[Trond Myklebust]

If the delegation was successfully returned, then mark it as revoked.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 32 ++++++++++++++++++++++++++++++++
 fs/nfs/delegation.h |  1 +
 fs/nfs/nfs4proc.c   |  1 +
 3 files changed, 34 insertions(+)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5a87d32a8d7c..b214b88b35b5 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -795,6 +795,38 @@ void nfs_remove_bad_delegation(struct inode *inode,
 }
 EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
 
+void nfs_delegation_mark_returned(struct inode *inode,
+		const nfs4_stateid *stateid)
+{
+	struct nfs_delegation *delegation;
+
+	if (!inode)
+		return;
+
+	rcu_read_lock();
+	delegation = rcu_dereference(NFS_I(inode)->delegation);
+	if (!delegation)
+		goto out_rcu_unlock;
+
+	spin_lock(&delegation->lock);
+	if (!nfs4_stateid_match_other(stateid, &delegation->stateid))
+		goto out_spin_unlock;
+	if (stateid->seqid &&
+	    nfs4_stateid_is_newer(&delegation->stateid, stateid))
+		goto out_clear_returning;
+
+	set_bit(NFS_DELEGATION_REVOKED, &delegation->flags);
+
+out_clear_returning:
+	clear_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
+out_spin_unlock:
+	spin_unlock(&delegation->lock);
+out_rcu_unlock:
+	rcu_read_unlock();
+
+	nfs_inode_find_state_and_recover(inode, stateid);
+}
+
 /**
  * nfs_expire_unused_delegation_types
  * @clp: client to process
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index 9a14a7ca1df9..7f0151c40d2f 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -53,6 +53,7 @@ void nfs_expire_unreferenced_delegations(struct nfs_client *clp);
 int nfs_client_return_marked_delegations(struct nfs_client *clp);
 int nfs_delegations_present(struct nfs_client *clp);
 void nfs_remove_bad_delegation(struct inode *inode, const nfs4_stateid *stateid);
+void nfs_delegation_mark_returned(struct inode *inode, const nfs4_stateid *stateid);
 
 void nfs_delegation_mark_reclaim(struct nfs_client *clp);
 void nfs_delegation_reap_unclaimed(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index c407e2eed3d5..4375b07ede25 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6211,6 +6211,7 @@ static void nfs4_delegreturn_done(struct rpc_task *task, void *calldata)
 		if (exception.retry)
 			goto out_restart;
 	}
+	nfs_delegation_mark_returned(data->inode, data->args.stateid);
 	data->rpc_status = task->tk_status;
 	return;
 out_restart:
-- 
2.21.0

[PATCH 12/14] NFSv4: Ignore requests to return the delegation if it was revoked

[Trond Myklebust]

If the delegation was revoked, or is already being returned, just
clear the NFS_DELEGATION_RETURN and NFS_DELEGATION_RETURN_IF_CLOSED
flags and keep going.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index b214b88b35b5..150a3bf7b35c 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -465,8 +465,6 @@ static bool nfs_delegation_need_return(struct nfs_delegation *delegation)
 {
 	bool ret = false;
 
-	if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
-		goto out;
 	if (test_and_clear_bit(NFS_DELEGATION_RETURN, &delegation->flags))
 		ret = true;
 	if (test_and_clear_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags) && !ret) {
@@ -478,7 +476,10 @@ static bool nfs_delegation_need_return(struct nfs_delegation *delegation)
 			ret = true;
 		spin_unlock(&delegation->lock);
 	}
-out:
+	if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags) ||
+	    test_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
+		ret = false;
+
 	return ret;
 }
 
-- 
2.21.0

[PATCH 13/14] NFSv4: Don't reclaim delegations that have been returned or revoked

[Trond Myklebust]

If the delegation has already been revoked, we want to avoid reclaiming
it on reboot.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 150a3bf7b35c..00c6c343dced 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -188,7 +188,7 @@ void nfs_inode_reclaim_delegation(struct inode *inode, const struct cred *cred,
 	delegation = rcu_dereference(NFS_I(inode)->delegation);
 	if (delegation != NULL) {
 		spin_lock(&delegation->lock);
-		if (delegation->inode != NULL) {
+		if (nfs4_is_valid_delegation(delegation, FMODE_READ)) {
 			nfs4_stateid_copy(&delegation->stateid, stateid);
 			delegation->type = type;
 			delegation->pagemod_limit = pagemod_limit;
-- 
2.21.0

[PATCH 14/14] NFSv4: Fix races between open and delegreturn

[Trond Myklebust]

If the server returns the same delegation in an open that we just used
in a delegreturn, we need to ensure we don't apply that stateid if
the delegreturn has freed it on the server.
To do so, we ensure that we do not free the storage for the delegation
until either it is replaced by a new one, or we throw the inode out of
cache.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/delegation.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 00c6c343dced..db7cf480c108 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -218,7 +218,6 @@ static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation *
 				delegation->cred,
 				&delegation->stateid,
 				issync);
-	nfs_free_delegation(delegation);
 	return res;
 }
 
@@ -291,7 +290,6 @@ nfs_detach_delegation_locked(struct nfs_inode *nfsi,
 		spin_unlock(&delegation->lock);
 		return NULL;
 	}
-	set_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
 	list_del_rcu(&delegation->super_list);
 	delegation->inode = NULL;
 	rcu_assign_pointer(nfsi->delegation, NULL);
@@ -318,10 +316,12 @@ nfs_inode_detach_delegation(struct inode *inode)
 	struct nfs_server *server = NFS_SERVER(inode);
 	struct nfs_delegation *delegation;
 
-	delegation = nfs_start_delegation_return(nfsi);
-	if (delegation == NULL)
-		return NULL;
-	return nfs_detach_delegation(nfsi, delegation, server);
+	rcu_read_lock();
+	delegation = rcu_dereference(nfsi->delegation);
+	if (delegation != NULL)
+		delegation = nfs_detach_delegation(nfsi, delegation, server);
+	rcu_read_unlock();
+	return delegation;
 }
 
 static void
@@ -420,8 +420,10 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 	spin_unlock(&clp->cl_lock);
 	if (delegation != NULL)
 		nfs_free_delegation(delegation);
-	if (freeme != NULL)
+	if (freeme != NULL) {
 		nfs_do_return_delegation(inode, freeme, 0);
+		nfs_free_delegation(freeme);
+	}
 	return status;
 }
 
@@ -431,7 +433,6 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation *delegation, int issync)
 {
 	struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
-	struct nfs_inode *nfsi = NFS_I(inode);
 	int err = 0;
 
 	if (delegation == NULL)
@@ -453,8 +454,6 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation
 		nfs_abort_delegation_return(delegation, clp);
 		goto out;
 	}
-	if (!nfs_detach_delegation(nfsi, delegation, NFS_SERVER(inode)))
-		goto out;
 
 	err = nfs_do_return_delegation(inode, delegation, issync);
 out:
@@ -597,6 +596,7 @@ void nfs_inode_evict_delegation(struct inode *inode)
 	if (delegation != NULL) {
 		set_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags);
 		nfs_do_return_delegation(inode, delegation, 1);
+		nfs_free_delegation(delegation);
 	}
 }
 
@@ -744,10 +744,9 @@ static void nfs_mark_delegation_revoked(struct nfs_server *server,
 {
 	set_bit(NFS_DELEGATION_REVOKED, &delegation->flags);
 	delegation->stateid.type = NFS4_INVALID_STATEID_TYPE;
-	nfs_mark_return_delegation(server, delegation);
 }
 
-static bool nfs_revoke_delegation(struct inode *inode,
+static void nfs_revoke_delegation(struct inode *inode,
 		const nfs4_stateid *stateid)
 {
 	struct nfs_delegation *delegation;
@@ -780,19 +779,12 @@ static bool nfs_revoke_delegation(struct inode *inode,
 	rcu_read_unlock();
 	if (ret)
 		nfs_inode_find_state_and_recover(inode, stateid);
-	return ret;
 }
 
 void nfs_remove_bad_delegation(struct inode *inode,
 		const nfs4_stateid *stateid)
 {
-	struct nfs_delegation *delegation;
-
-	if (!nfs_revoke_delegation(inode, stateid))
-		return;
-	delegation = nfs_inode_detach_delegation(inode);
-	if (delegation)
-		nfs_free_delegation(delegation);
+	nfs_revoke_delegation(inode, stateid);
 }
 EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
 
@@ -816,7 +808,7 @@ void nfs_delegation_mark_returned(struct inode *inode,
 	    nfs4_stateid_is_newer(&delegation->stateid, stateid))
 		goto out_clear_returning;
 
-	set_bit(NFS_DELEGATION_REVOKED, &delegation->flags);
+	nfs_mark_delegation_revoked(NFS_SERVER(inode), delegation);
 
 out_clear_returning:
 	clear_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
-- 
2.21.0

Re: [PATCH 00/14] Delegation bugfixes

[Olga Kornievskaia]

Hi Trond,

This patch set produces the following in my testing. Basically what I
see the client is prevented from using a delegation at all.

After I induce a race of DELEGRETURN/OPEN
--- the racing OPEN gets a delegation (it returns the same seqid and
other as the delegation being returned) but the client doesn't use it.
--- the following (next) OPEN that also gets a delegation immediately
has the client returning the given delegation.

Disclaimer: in my testing the racing DELEGRETURN doesn't fail with
OLD_STATEID, NetApp returns OK.

On Thu, Oct 24, 2019 at 6:56 AM Trond Myklebust <trondmy@gmail.com> wrote:
>
> The following patchset fixes up a number of issues with delegations,
> but mainly attempts to fix a race condition between open and
> delegreturn, where the open and the delegreturn get re-ordered so
> that the delegreturn ends up revoking the delegation that was returned
> by the open.
> The root cause is that in certain circumstances, we may currently end
> up freeing the delegation from delegreturn, so when we later receive
> the reply to the open, we've lost track of the fact that the seqid
> predates the one that was returned.
>
> This patchset fixes that case by ensuring that we always keep track
> of the last delegation stateid that was returned for any given inode.
> That way, if we later see a delegation stateid with the same opaque
> field, but an older seqid, we know we cannot trust it, and so we
> ask to replay the OPEN compound.
>
> Trond Myklebust (14):
>   NFSv4: Don't allow a cached open with a revoked delegation
>   NFSv4: Fix delegation handling in update_open_stateid()
>   NFSv4: nfs4_callback_getattr() should ignore revoked delegations
>   NFSv4: Delegation recalls should not find revoked delegations
>   NFSv4: fail nfs4_refresh_delegation_stateid() when the delegation was
>     revoked
>   NFS: Rename nfs_inode_return_delegation_noreclaim()
>   NFSv4: Don't remove the delegation from the super_list more than once
>   NFSv4: Hold the delegation spinlock when updating the seqid
>   NFSv4: Clear the NFS_DELEGATION_REVOKED flag in
>     nfs_update_inplace_delegation()
>   NFSv4: Update the stateid seqid in nfs_revoke_delegation()
>   NFSv4: Revoke the delegation on success in nfs4_delegreturn_done()
>   NFSv4: Ignore requests to return the delegation if it was revoked
>   NFSv4: Don't reclaim delegations that have been returned or revoked
>   NFSv4: Fix races between open and delegreturn
>
>  fs/nfs/callback_proc.c |   2 +-
>  fs/nfs/delegation.c    | 109 +++++++++++++++++++++++++++++------------
>  fs/nfs/delegation.h    |   4 +-
>  fs/nfs/nfs4proc.c      |  13 ++---
>  fs/nfs/nfs4super.c     |   4 +-
>  5 files changed, 88 insertions(+), 44 deletions(-)
>
> --
> 2.21.0
>

Re: [PATCH 00/14] Delegation bugfixes

[Olga Kornievskaia]

On Thu, Oct 31, 2019 at 11:27 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> Hi Trond,
>
> This patch set produces the following in my testing. Basically what I
> see the client is prevented from using a delegation at all.
>
> After I induce a race of DELEGRETURN/OPEN
> --- the racing OPEN gets a delegation (it returns the same seqid and
> other as the delegation being returned) but the client doesn't use it.
> --- the following (next) OPEN that also gets a delegation immediately
> has the client returning the given delegation.
>
> Disclaimer: in my testing the racing DELEGRETURN doesn't fail with
> OLD_STATEID, NetApp returns OK.

Testing the same against Linux. It prevents the client from using
future delegation stateid. On the induced DELEGRETURN/OPEN race, the
linux server doesn't give a new read delegation. The following open
gets a read delegation and returns it right away.


> On Thu, Oct 24, 2019 at 6:56 AM Trond Myklebust <trondmy@gmail.com> wrote:
> >
> > The following patchset fixes up a number of issues with delegations,
> > but mainly attempts to fix a race condition between open and
> > delegreturn, where the open and the delegreturn get re-ordered so
> > that the delegreturn ends up revoking the delegation that was returned
> > by the open.
> > The root cause is that in certain circumstances, we may currently end
> > up freeing the delegation from delegreturn, so when we later receive
> > the reply to the open, we've lost track of the fact that the seqid
> > predates the one that was returned.
> >
> > This patchset fixes that case by ensuring that we always keep track
> > of the last delegation stateid that was returned for any given inode.
> > That way, if we later see a delegation stateid with the same opaque
> > field, but an older seqid, we know we cannot trust it, and so we
> > ask to replay the OPEN compound.
> >
> > Trond Myklebust (14):
> >   NFSv4: Don't allow a cached open with a revoked delegation
> >   NFSv4: Fix delegation handling in update_open_stateid()
> >   NFSv4: nfs4_callback_getattr() should ignore revoked delegations
> >   NFSv4: Delegation recalls should not find revoked delegations
> >   NFSv4: fail nfs4_refresh_delegation_stateid() when the delegation was
> >     revoked
> >   NFS: Rename nfs_inode_return_delegation_noreclaim()
> >   NFSv4: Don't remove the delegation from the super_list more than once
> >   NFSv4: Hold the delegation spinlock when updating the seqid
> >   NFSv4: Clear the NFS_DELEGATION_REVOKED flag in
> >     nfs_update_inplace_delegation()
> >   NFSv4: Update the stateid seqid in nfs_revoke_delegation()
> >   NFSv4: Revoke the delegation on success in nfs4_delegreturn_done()
> >   NFSv4: Ignore requests to return the delegation if it was revoked
> >   NFSv4: Don't reclaim delegations that have been returned or revoked
> >   NFSv4: Fix races between open and delegreturn
> >
> >  fs/nfs/callback_proc.c |   2 +-
> >  fs/nfs/delegation.c    | 109 +++++++++++++++++++++++++++++------------
> >  fs/nfs/delegation.h    |   4 +-
> >  fs/nfs/nfs4proc.c      |  13 ++---
> >  fs/nfs/nfs4super.c     |   4 +-
> >  5 files changed, 88 insertions(+), 44 deletions(-)
> >
> > --
> > 2.21.0
> >

Re: [PATCH 00/14] Delegation bugfixes

[Olga Kornievskaia]

Hi Trond,

Now that I recall the reason the Ontap server gives out the same
delegation was to deal with the linux client behaviour who sends an
open even though it holds a delegation (according to the server's
view). So what server does is it gives out the same delegation. This
patch series changes the semantics. Can you describe what you expect
the server is supposed to do in this case?

On Thu, Oct 31, 2019 at 11:49 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Thu, Oct 31, 2019 at 11:27 AM Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > Hi Trond,
> >
> > This patch set produces the following in my testing. Basically what I
> > see the client is prevented from using a delegation at all.
> >
> > After I induce a race of DELEGRETURN/OPEN
> > --- the racing OPEN gets a delegation (it returns the same seqid and
> > other as the delegation being returned) but the client doesn't use it.
> > --- the following (next) OPEN that also gets a delegation immediately
> > has the client returning the given delegation.
> >
> > Disclaimer: in my testing the racing DELEGRETURN doesn't fail with
> > OLD_STATEID, NetApp returns OK.
>
> Testing the same against Linux. It prevents the client from using
> future delegation stateid. On the induced DELEGRETURN/OPEN race, the
> linux server doesn't give a new read delegation. The following open
> gets a read delegation and returns it right away.
>
>
> > On Thu, Oct 24, 2019 at 6:56 AM Trond Myklebust <trondmy@gmail.com> wrote:
> > >
> > > The following patchset fixes up a number of issues with delegations,
> > > but mainly attempts to fix a race condition between open and
> > > delegreturn, where the open and the delegreturn get re-ordered so
> > > that the delegreturn ends up revoking the delegation that was returned
> > > by the open.
> > > The root cause is that in certain circumstances, we may currently end
> > > up freeing the delegation from delegreturn, so when we later receive
> > > the reply to the open, we've lost track of the fact that the seqid
> > > predates the one that was returned.
> > >
> > > This patchset fixes that case by ensuring that we always keep track
> > > of the last delegation stateid that was returned for any given inode.
> > > That way, if we later see a delegation stateid with the same opaque
> > > field, but an older seqid, we know we cannot trust it, and so we
> > > ask to replay the OPEN compound.
> > >
> > > Trond Myklebust (14):
> > >   NFSv4: Don't allow a cached open with a revoked delegation
> > >   NFSv4: Fix delegation handling in update_open_stateid()
> > >   NFSv4: nfs4_callback_getattr() should ignore revoked delegations
> > >   NFSv4: Delegation recalls should not find revoked delegations
> > >   NFSv4: fail nfs4_refresh_delegation_stateid() when the delegation was
> > >     revoked
> > >   NFS: Rename nfs_inode_return_delegation_noreclaim()
> > >   NFSv4: Don't remove the delegation from the super_list more than once
> > >   NFSv4: Hold the delegation spinlock when updating the seqid
> > >   NFSv4: Clear the NFS_DELEGATION_REVOKED flag in
> > >     nfs_update_inplace_delegation()
> > >   NFSv4: Update the stateid seqid in nfs_revoke_delegation()
> > >   NFSv4: Revoke the delegation on success in nfs4_delegreturn_done()
> > >   NFSv4: Ignore requests to return the delegation if it was revoked
> > >   NFSv4: Don't reclaim delegations that have been returned or revoked
> > >   NFSv4: Fix races between open and delegreturn
> > >
> > >  fs/nfs/callback_proc.c |   2 +-
> > >  fs/nfs/delegation.c    | 109 +++++++++++++++++++++++++++++------------
> > >  fs/nfs/delegation.h    |   4 +-
> > >  fs/nfs/nfs4proc.c      |  13 ++---
> > >  fs/nfs/nfs4super.c     |   4 +-
> > >  5 files changed, 88 insertions(+), 44 deletions(-)
> > >
> > > --
> > > 2.21.0
> > >

Re: [PATCH 00/14] Delegation bugfixes

[Trond Myklebust]

Hi Olga

On Thu, 2019-10-31 at 12:15 -0400, Olga Kornievskaia wrote:
> Hi Trond,
> 
> Now that I recall the reason the Ontap server gives out the same
> delegation was to deal with the linux client behaviour who sends an
> open even though it holds a delegation (according to the server's
> view). So what server does is it gives out the same delegation. This
> patch series changes the semantics. Can you describe what you expect
> the server is supposed to do in this case?

If the client sends out a second OPEN for the same file (presumably
because an application request caused the file to be opened again
before the client could process the reply to the first open), then I
think it should be OK for the server to send the delegation stateid
again. If the new open caused a state change (for instance the
delegation was upgraded from READ to WRITE) then the seqid for the
delegation stateid should be bumped. If there was not state change, it
really should be up to the server whether or not it bumps the seqid.

However on the client side, we want to be able to consider that if the
2 open replies return the exact same delegation stateid, then we
consider the second reply to be a no-op as far as our update of the
internal state is concerned. If the seqid was bumped, the client needs
to update its internal state.

So all this patch series is doing, is setting up rules to allow us to
enforce that case, and in particular to ensure that _if_ we send a
DELEGRETURN before we process the reply to the second open, then we
recognise that a successful DELEGRETURN means we no longer hold
delegation state, no matter what the contents of the reply to the
second open tells us.

> On Thu, Oct 31, 2019 at 11:49 AM Olga Kornievskaia <aglo@umich.edu>
> wrote:
> > On Thu, Oct 31, 2019 at 11:27 AM Olga Kornievskaia <aglo@umich.edu>
> > wrote:
> > > Hi Trond,
> > > 
> > > This patch set produces the following in my testing. Basically
> > > what I
> > > see the client is prevented from using a delegation at all.
> > > 
> > > After I induce a race of DELEGRETURN/OPEN
> > > --- the racing OPEN gets a delegation (it returns the same seqid
> > > and
> > > other as the delegation being returned) but the client doesn't
> > > use it.
> > > --- the following (next) OPEN that also gets a delegation
> > > immediately
> > > has the client returning the given delegation.
> > > 
> > > Disclaimer: in my testing the racing DELEGRETURN doesn't fail
> > > with
> > > OLD_STATEID, NetApp returns OK.
> > 
> > Testing the same against Linux. It prevents the client from using
> > future delegation stateid. On the induced DELEGRETURN/OPEN race,
> > the
> > linux server doesn't give a new read delegation. The following open
> > gets a read delegation and returns it right away.
> > 
> > 
> > > On Thu, Oct 24, 2019 at 6:56 AM Trond Myklebust <
> > > trondmy@gmail.com> wrote:
> > > > The following patchset fixes up a number of issues with
> > > > delegations,
> > > > but mainly attempts to fix a race condition between open and
> > > > delegreturn, where the open and the delegreturn get re-ordered
> > > > so
> > > > that the delegreturn ends up revoking the delegation that was
> > > > returned
> > > > by the open.
> > > > The root cause is that in certain circumstances, we may
> > > > currently end
> > > > up freeing the delegation from delegreturn, so when we later
> > > > receive
> > > > the reply to the open, we've lost track of the fact that the
> > > > seqid
> > > > predates the one that was returned.
> > > > 
> > > > This patchset fixes that case by ensuring that we always keep
> > > > track
> > > > of the last delegation stateid that was returned for any given
> > > > inode.
> > > > That way, if we later see a delegation stateid with the same
> > > > opaque
> > > > field, but an older seqid, we know we cannot trust it, and so
> > > > we
> > > > ask to replay the OPEN compound.
> > > > 
> > > > Trond Myklebust (14):
> > > >   NFSv4: Don't allow a cached open with a revoked delegation
> > > >   NFSv4: Fix delegation handling in update_open_stateid()
> > > >   NFSv4: nfs4_callback_getattr() should ignore revoked
> > > > delegations
> > > >   NFSv4: Delegation recalls should not find revoked delegations
> > > >   NFSv4: fail nfs4_refresh_delegation_stateid() when the
> > > > delegation was
> > > >     revoked
> > > >   NFS: Rename nfs_inode_return_delegation_noreclaim()
> > > >   NFSv4: Don't remove the delegation from the super_list more
> > > > than once
> > > >   NFSv4: Hold the delegation spinlock when updating the seqid
> > > >   NFSv4: Clear the NFS_DELEGATION_REVOKED flag in
> > > >     nfs_update_inplace_delegation()
> > > >   NFSv4: Update the stateid seqid in nfs_revoke_delegation()
> > > >   NFSv4: Revoke the delegation on success in
> > > > nfs4_delegreturn_done()
> > > >   NFSv4: Ignore requests to return the delegation if it was
> > > > revoked
> > > >   NFSv4: Don't reclaim delegations that have been returned or
> > > > revoked
> > > >   NFSv4: Fix races between open and delegreturn
> > > > 
> > > >  fs/nfs/callback_proc.c |   2 +-
> > > >  fs/nfs/delegation.c    | 109 +++++++++++++++++++++++++++++--
> > > > ----------
> > > >  fs/nfs/delegation.h    |   4 +-
> > > >  fs/nfs/nfs4proc.c      |  13 ++---
> > > >  fs/nfs/nfs4super.c     |   4 +-
> > > >  5 files changed, 88 insertions(+), 44 deletions(-)
> > > > 
> > > > --
> > > > 2.21.0
> > > > 
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com