Re: Kernel panic / list_add corruption when in nfsd4_run_cb_work

["bfields@fieldses.org" <bfields@fieldses.org>]

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:
> > On Wed, Nov 24, 2021 at 05:14:53PM +0000, Trond Myklebust wrote:
> > > 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 <bfields@redhat.com>
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 <bfields@redhat.com>

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);
 	}