DragonFlyBSD Kernel Audit
DF-0843 / fix.diff
← back to finding ↓ download raw
diff --git a/sys/vfs/ufs/ufs_dirhash.c b/sys/vfs/ufs/ufs_dirhash.c
--- a/sys/vfs/ufs/ufs_dirhash.c
+++ b/sys/vfs/ufs/ufs_dirhash.c
@@ -91,6 +91,17 @@
 static TAILQ_HEAD(, dirhash) ufsdirhash_list;
 
 /*
+ * Global lock guarding ufsdirhash_list membership and the dh_hash lifetime
+ * of every dirhash on the list.  Held shared by ufsdirhash_lookup (and the
+ * helper readers) so a concurrent ufsdirhash_recycle / ufsdirhash_free --
+ * which need it exclusive -- cannot free a dh_hash array out from under a
+ * reader.  Mirrors FreeBSD's ufsdirhash_lock (sx); DragonFly previously had
+ * no lock here at all.
+ */
+static struct lock ufsdirhash_lock;
+
+
+/*
  * Attempt to build up a hash table for the directory contents in
  * inode 'ip'. Returns 0 on success, or -1 of the operation failed.
  */
@@ -218,8 +229,10 @@
 
 	if (bp != NULL)
 		brelse(bp);
+	lockmgr(&ufsdirhash_lock, LK_EXCLUSIVE);
 	TAILQ_INSERT_TAIL(&ufsdirhash_list, dh, dh_list);
 	dh->dh_onlist = 1;
+	lockmgr(&ufsdirhash_lock, LK_RELEASE);
 	return (0);
 
 fail:
@@ -248,8 +261,11 @@
 
 	if ((dh = ip->i_dirhash) == NULL)
 		return;
-	if (dh->dh_onlist)
+	if (dh->dh_onlist) {
+		lockmgr(&ufsdirhash_lock, LK_EXCLUSIVE);
 		TAILQ_REMOVE(&ufsdirhash_list, dh, dh_list);
+		lockmgr(&ufsdirhash_lock, LK_RELEASE);
+	}
 
 	/* The dirhash pointed to by 'dh' is exclusively ours now. */
 
@@ -294,18 +310,14 @@
 	if ((dh = ip->i_dirhash) == NULL)
 		return (EJUSTRETURN);
 	/*
-	 * Move this dirhash towards the end of the list if it has a
-	 * score higher than the next entry.
-	 * Optimise the case where it's already the last by performing
-	 * an unlocked read of the TAILQ_NEXT pointer.
+	 * Take the global dirhash lock exclusively for the (rare) score
+	 * reorder of the global list, then downgrade to shared for the
+	 * actual hash lookup.  recycle/free need the lock exclusive, so
+	 * while we hold it (either way) dh_hash cannot be freed out from
+	 * under us.  Re-validate dh_hash != NULL at every transition.
 	 */
+	lockmgr(&ufsdirhash_lock, LK_EXCLUSIVE);
 	if (TAILQ_NEXT(dh, dh_list) != NULL) {
-		/*
-		 * If the new score will be greater than that of the next
-		 * entry, then move this entry past it. With both mutexes
-		 * held, dh_next won't go away, but its dh_score could
-		 * change; that's not important since it is just a hint.
-		 */
 		if (dh->dh_hash != NULL &&
 		    (dh_next = TAILQ_NEXT(dh, dh_list)) != NULL &&
 		    dh->dh_score >= dh_next->dh_score) {
@@ -316,9 +328,12 @@
 		}
 	}
 	if (dh->dh_hash == NULL) {
+		lockmgr(&ufsdirhash_lock, LK_RELEASE);
 		ufsdirhash_free(ip);
 		return (EJUSTRETURN);
 	}
+	/* Hold shared across the hash-array derefs: recycle is now excluded. */
+	lockmgr(&ufsdirhash_lock, LK_DOWNGRADE);
 
 	/* Update the score. */
 	if (dh->dh_score < DH_SCOREMAX)
@@ -364,8 +379,10 @@
 			if (bp != NULL)
 				brelse(bp);
 			blkoff = offset & ~bmask;
-			if (ffs_blkatoff(vp, (off_t)blkoff, NULL, &bp) != 0)
+			if (ffs_blkatoff(vp, (off_t)blkoff, NULL, &bp) != 0) {
+				lockmgr(&ufsdirhash_lock, LK_RELEASE);
 				return (EJUSTRETURN);
+			}
 		}
 		dp = (struct direct *)(bp->b_data + (offset & bmask));
 		if (dp->d_reclen == 0 || dp->d_reclen >
@@ -397,12 +414,14 @@
 
 			*bpp = bp;
 			*offp = offset;
+			lockmgr(&ufsdirhash_lock, LK_RELEASE);
 			return (0);
 		}
 
 		if (dh->dh_hash == NULL) {
 			if (bp != NULL)
 				brelse(bp);
+			lockmgr(&ufsdirhash_lock, LK_RELEASE);
 			ufsdirhash_free(ip);
 			return (EJUSTRETURN);
 		}
@@ -417,6 +436,7 @@
 	}
 	if (bp != NULL)
 		brelse(bp);
+	lockmgr(&ufsdirhash_lock, LK_RELEASE);
 	return (ENOENT);
 }
 
@@ -932,15 +952,18 @@
 	uint8_t *blkfree;
 	int i, mem, narrays;
 
+	lockmgr(&ufsdirhash_lock, LK_EXCLUSIVE);
 	while (wanted + ufs_dirhashmem > ufs_dirhashmaxmem) {
 		/* Find a dirhash, and lock it. */
 		if ((dh = TAILQ_FIRST(&ufsdirhash_list)) == NULL) {
+			lockmgr(&ufsdirhash_lock, LK_RELEASE);
 			return (-1);
 		}
 		KASSERT(dh->dh_hash != NULL, ("dirhash: NULL hash on list"));
 
 		/* Decrement the score; only recycle if it becomes zero. */
 		if (--dh->dh_score > 0) {
+			lockmgr(&ufsdirhash_lock, LK_RELEASE);
 			return (-1);
 		}
 
@@ -965,6 +988,7 @@
 		/* Account for the returned memory, and repeat if necessary. */
 		ufs_dirhashmem -= mem;
 	}
+	lockmgr(&ufsdirhash_lock, LK_RELEASE);
 	/* Success. */
 	return (0);
 }
@@ -975,6 +999,7 @@
 {
 	ufsdirhash_oc = objcache_create_simple(M_DIRHASH,
 	    DH_NBLKOFF * sizeof(daddr_t));
+	lockinit(&ufsdirhash_lock, "ufsdirhash", 0, 0);
 	TAILQ_INIT(&ufsdirhash_list);
 }
 SYSINIT(ufsdirhash, SI_SUB_PSEUDO, SI_ORDER_ANY, ufsdirhash_init, NULL);