On Wed, Nov 24, 2021 at 10:17:51PM +0000, Trond Myklebust wrote:

On Wed, 2021-11-24 at 17:06 -0500, bfields@fieldses.org wrote:

quoted

On Wed, Nov 24, 2021 at 05:14:53PM +0000, Trond Myklebust wrote:

quoted

It is a little nasty that we hide the list_del() calls in several
levels of function call, so they probably do deserve a comment.

That said, if, as in the case here, the delegation was unhashed, we
still end up not calling list_del_init() in
unhash_delegation_locked(),
and since the list_add() is not conditional on it being successful,
the
global list is again corrupted.

Yes, it is an unlikely race, but it is possible despite your
change.

Thanks, good point.

Probably not something anyone's actually hitting, but another sign
this
logic need rethinking.

I think it should be sufficient to let the laundromat skip that entry
and leave it on the list if the unhash_delegation_locked() fails, since
your fix should then be able to pick the delegation up and destroy it
safely.

We can keep the code in __destroy_client() and
nfs4_state_shutdown_net() unchanged, since those are presumably not
affected by this race.

I think simplest is just not to put the thing on the lru at all if it's
not hashed:

--b.

commit 5011f84ef05e
Author: J. Bruce Fields [off-list ref]
Date:   Mon Nov 29 15:08:00 2021 -0500

    nfsd: fix use-after-free due to delegation race
    
    A delegation break could arrive as soon as we've called vfs_setlease.  A
    delegation break runs a callback which immediately (in
    nfsd4_cb_recall_prepare) adds the delegation to del_recall_lru.  If we
    then exit nfs4_set_delegation without hashing the delegation, it will be
    freed as soon as the callback is done with it, without ever being
    removed from del_recall_lru.
    
    Symptoms show up later as use-after-free or list corruption warnings,
    usually in the laundromat thread.
    
    Signed-off-by: J. Bruce Fields [off-list ref]

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index bfad94c70b84..1956d377d1a6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c

@@ -1207,6 +1207,11 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
 	return 0;
 }
 
+static bool delegation_hashed(struct nfs4_delegation *dp)
+{
+	return !(list_empty(&dp->dl_perfile));
+}
+
 static bool
 unhash_delegation_locked(struct nfs4_delegation *dp)
 {

@@ -1214,7 +1219,7 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
 
 	lockdep_assert_held(&state_lock);
 
-	if (list_empty(&dp->dl_perfile))
+	if (!delegation_hashed(dp))
 		return false;
 
 	dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;

@@ -4598,7 +4603,7 @@ static void nfsd4_cb_recall_prepare(struct nfsd4_callback *cb)
 	 * queued for a lease break. Don't queue it again.
 	 */
 	spin_lock(&state_lock);
-	if (dp->dl_time == 0) {
+	if (delegation_hashed(dp) && dp->dl_time == 0) {
 		dp->dl_time = ktime_get_boottime_seconds();
 		list_add_tail(&dp->dl_recall_lru, &nn->del_recall_lru);
 	}