git: 06148d225170 - main - Revert "nfscl: Fix a use after free in nfscl_cleanupkext()"

The branch main has been updated by rmacklem:

URL: https://cgit.FreeBSD.org/src/commit/?id=06148d22517067dce12f90951c1cffa6ecad7a7b

commit 06148d22517067dce12f90951c1cffa6ecad7a7b
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2022-02-24 15:01:03 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2022-02-24 15:01:03 +0000

    Revert "nfscl: Fix a use after free in nfscl_cleanupkext()"
    
    This reverts commit dd08b84e35b6fdee0df5fd0e0533cd361dec71cb.
    
    cy@ reported a problem caused by this patch.  He will be
    testing an alternate patch, but I'm reverting this one.
---
 sys/fs/nfsclient/nfs_clstate.c | 70 +++++++-----------------------------------
 1 file changed, 11 insertions(+), 59 deletions(-)

diff --git a/sys/fs/nfsclient/nfs_clstate.c b/sys/fs/nfsclient/nfs_clstate.c
index c37ff8a8ab11..5262986645cd 100644
--- a/sys/fs/nfsclient/nfs_clstate.c
+++ b/sys/fs/nfsclient/nfs_clstate.c
@@ -158,7 +158,7 @@ static int nfsrpc_reopen(struct nfsmount *, u_int8_t *, int, u_int32_t,
 static void nfscl_freedeleg(struct nfscldeleghead *, struct nfscldeleg *,
     bool);
 static int nfscl_errmap(struct nfsrv_descript *, u_int32_t);
-static u_int nfscl_cleanup_common(struct nfsclclient *, u_int8_t *);
+static void nfscl_cleanup_common(struct nfsclclient *, u_int8_t *);
 static int nfscl_recalldeleg(struct nfsclclient *, struct nfsmount *,
     struct nfscldeleg *, vnode_t, struct ucred *, NFSPROC_T *, int,
     vnode_t *);
@@ -1642,21 +1642,8 @@ nfscl_expireopen(struct nfsclclient *clp, struct nfsclopen *op,
 static void
 nfscl_freeopenowner(struct nfsclowner *owp, int local)
 {
-	int owned;
 
-	/*
-	 * Make sure the NFSCLSTATE mutex is held, to avoid races with
-	 * calls in nfscl_renewthread() that do not hold a reference
-	 * count on the nfsclclient and just the mutex.
-	 * The mutex will not be held for calls done with the exclusive
-	 * nfsclclient lock held.
-	 */
-	owned = mtx_owned(NFSCLSTATEMUTEXPTR);
-	if (owned == 0)
-		NFSLOCKCLSTATE();
 	LIST_REMOVE(owp, nfsow_list);
-	if (owned == 0)
-		NFSUNLOCKCLSTATE();
 	free(owp, M_NFSCLOWNER);
 	if (local)
 		nfsstatsv1.cllocalopenowners--;
@@ -1671,21 +1658,8 @@ void
 nfscl_freelockowner(struct nfscllockowner *lp, int local)
 {
 	struct nfscllock *lop, *nlop;
-	int owned;
 
-	/*
-	 * Make sure the NFSCLSTATE mutex is held, to avoid races with
-	 * calls in nfscl_renewthread() that do not hold a reference
-	 * count on the nfsclclient and just the mutex.
-	 * The mutex will not be held for calls done with the exclusive
-	 * nfsclclient lock held.
-	 */
-	owned = mtx_owned(NFSCLSTATEMUTEXPTR);
-	if (owned == 0)
-		NFSLOCKCLSTATE();
 	LIST_REMOVE(lp, nfsl_list);
-	if (owned == 0)
-		NFSUNLOCKCLSTATE();
 	LIST_FOREACH_SAFE(lop, &lp->nfsl_lock, nfslo_list, nlop) {
 		nfscl_freelock(lop, local);
 	}
@@ -1867,24 +1841,17 @@ nfscl_expireclient(struct nfsclclient *clp, struct nfsmount *nmp,
 	}
 }
 
-#define	FREED_OPENOWNER	0x1
-#define	FREED_LOCKOWNER	0x2
-
 /*
  * This function must be called after the process represented by "own" has
  * exited. Must be called with CLSTATE lock held.
- * Return the FREED_xxx flag bits, since the caller needs to know if either
- * the open owner or lock owner lists have changed.
  */
-static u_int
+static void
 nfscl_cleanup_common(struct nfsclclient *clp, u_int8_t *own)
 {
 	struct nfsclowner *owp, *nowp;
 	struct nfscllockowner *lp, *nlp;
 	struct nfscldeleg *dp;
-	u_int didfree;
 
-	didfree = 0;
 	/* First, get rid of local locks on delegations. */
 	TAILQ_FOREACH(dp, &clp->nfsc_deleg, nfsdl_list) {
 		LIST_FOREACH_SAFE(lp, &dp->nfsdl_lock, nfsl_list, nlp) {
@@ -1892,7 +1859,6 @@ nfscl_cleanup_common(struct nfsclclient *clp, u_int8_t *own)
 			if ((lp->nfsl_rwlock.nfslock_lock & NFSV4LOCK_WANTED))
 			    panic("nfscllckw");
 			nfscl_freelockowner(lp, 1);
-			didfree |= FREED_LOCKOWNER;
 		    }
 		}
 	}
@@ -1907,15 +1873,13 @@ nfscl_cleanup_common(struct nfsclclient *clp, u_int8_t *own)
 			 * here. For that case, let the renew thread clear
 			 * out the OpenOwner later.
 			 */
-			if (LIST_EMPTY(&owp->nfsow_open)) {
+			if (LIST_EMPTY(&owp->nfsow_open))
 				nfscl_freeopenowner(owp, 0);
-				didfree |= FREED_OPENOWNER;
-			} else
+			else
 				owp->nfsow_defunct = 1;
 		}
 		owp = nowp;
 	}
-	return (didfree);
 }
 
 /*
@@ -1924,11 +1888,10 @@ nfscl_cleanup_common(struct nfsclclient *clp, u_int8_t *own)
 static void
 nfscl_cleanupkext(struct nfsclclient *clp, struct nfscllockownerfhhead *lhp)
 {
-	struct nfsclowner *owp;
+	struct nfsclowner *owp, *nowp;
 	struct nfsclopen *op;
 	struct nfscllockowner *lp, *nlp;
 	struct nfscldeleg *dp;
-	uint8_t own[NFSV4CL_LOCKNAMELEN];
 
 	/*
 	 * All the pidhash locks must be acquired, since they are sx locks
@@ -1938,20 +1901,15 @@ nfscl_cleanupkext(struct nfsclclient *clp, struct nfscllockownerfhhead *lhp)
 	 */
 	pidhash_slockall();
 	NFSLOCKCLSTATE();
-tryagain:
-	LIST_FOREACH(owp, &clp->nfsc_owner, nfsow_list) {
+	LIST_FOREACH_SAFE(owp, &clp->nfsc_owner, nfsow_list, nowp) {
 		LIST_FOREACH(op, &owp->nfsow_open, nfso_list) {
 			LIST_FOREACH_SAFE(lp, &op->nfso_lock, nfsl_list, nlp) {
 				if (LIST_EMPTY(&lp->nfsl_lock))
 					nfscl_emptylockowner(lp, lhp);
 			}
 		}
-		if (nfscl_procdoesntexist(owp->nfsow_owner)) {
-			memcpy(own, owp->nfsow_owner, NFSV4CL_LOCKNAMELEN);
-			if ((nfscl_cleanup_common(clp, own) &
-			    FREED_OPENOWNER) != 0)
-				goto tryagain;
-		}
+		if (nfscl_procdoesntexist(owp->nfsow_owner))
+			nfscl_cleanup_common(clp, owp->nfsow_owner);
 	}
 
 	/*
@@ -1962,15 +1920,9 @@ tryagain:
 	 * nfscl_cleanup_common().
 	 */
 	TAILQ_FOREACH(dp, &clp->nfsc_deleg, nfsdl_list) {
-tryagain2:
-		LIST_FOREACH(lp, &dp->nfsdl_lock, nfsl_list) {
-			if (nfscl_procdoesntexist(lp->nfsl_owner)) {
-				memcpy(own, lp->nfsl_owner,
-				    NFSV4CL_LOCKNAMELEN);
-				if ((nfscl_cleanup_common(clp, own) &
-				    FREED_LOCKOWNER) != 0)
-					goto tryagain2;
-			}
+		LIST_FOREACH_SAFE(lp, &dp->nfsdl_lock, nfsl_list, nlp) {
+			if (nfscl_procdoesntexist(lp->nfsl_owner))
+				nfscl_cleanup_common(clp, lp->nfsl_owner);
 		}
 	}
 	NFSUNLOCKCLSTATE();