PERFORCE change 56794 for review

[Robert Watson]

http://perforce.freebsd.org/chv.cgi?CH=56794

Change 56794 by rwatson at rwatson_tislabs on 2004/07/08 18:45:19

	While reference counting the sysv_msg module to prevent unload
	while in use is a good idea, the current implementation suffers
	from races that can't be fixed without work at the system call
	layer.  Remove the current implementation from now to simplify
	the set of changes between the FreeBSD CVS version of System V
	Message Queues and the TrustedBSD MAC version.

Affected files ...

.. //depot/projects/trustedbsd/mac/sys/kern/sysv_msg.c#20 edit

Differences ...

==== //depot/projects/trustedbsd/mac/sys/kern/sysv_msg.c#20 (text+ko) ====

@@ -128,8 +128,6 @@
 static struct msg *msghdrs;	/* MSGTQL msg headers */
 static struct msqid_kernel *msqids;	/* MSGMNI msqid_kernel struct's */
 static struct mtx msq_mtx;	/* global mutex for message queues. */
-static int refcount; /* to ensure consistency during and after msgunload */
-static struct mtx refcnt_mtx;	/* global mutex for refcount. */
 
 static void
 msginit()
@@ -212,16 +210,6 @@
 #endif
 	}
 	mtx_init(&msq_mtx, "msq", NULL, MTX_DEF);
-	refcount = 0;
-	/*
-	 * It is not permissible to pass the same mutex to mtx_init()
-	 * multiple times without intervening calls to mtx_destroy().  Since
-	 * we cannot destroy the refcnt_mtx during msgunload, we check if the
-	 * mtx_init has ever been called. If so, we dont need to do mtx_init
-	 * as the mutex is already initialized.
-	 */
-	if (mtx_initialized(&refcnt_mtx) == 0)
-		mtx_init(&refcnt_mtx, "msgrefcnt", NULL, MTX_DEF);
 }
 
 static int
@@ -237,11 +225,6 @@
 	 * (tinkering with the data structures), and also that no thread
 	 * can enter the code-paths once the module is unloaded.
 	 */
-	mtx_lock(&refcnt_mtx);
-	if (refcount > 0) {
-		mtx_unlock(&refcnt_mtx);
-		return (EBUSY);
-	}
 	for (msqid = 0; msqid < msginfo.msgmni; msqid++) {
 		/*
 		 * Look for an unallocated and unlocked msqid_ds.
@@ -254,13 +237,9 @@
 		    (msqkptr->u.msg_perm.mode & MSG_LOCKED) != 0)
 			break;
 	}
-	if (msqid != msginfo.msgmni) {
-		mtx_unlock(&refcnt_mtx);
+	if (msqid != msginfo.msgmni)
 		return (EBUSY);
-	}
 
-	refcount= -1; /* Mark the module as being unloaded */
-	mtx_unlock(&refcnt_mtx);
 
 #ifdef MAC
 	int i;
@@ -276,11 +255,6 @@
 	free(msghdrs, M_MSG);
 	free(msqids, M_MSG);
 	mtx_destroy(&msq_mtx);
-	/*
-	 * NOTE: We cannot destroy the refcnt_mtx as it is possible that
-	 * some thread might (attempt to) hold the mutex.
-	 */
-/* 	mtx_destroy(&refcnt_mtx); */
 	return (0);
 }
 
@@ -406,29 +380,16 @@
 	if (!jail_sysvipc_allowed && jailed(td->td_ucred))
 		return (ENOSYS);
 
-	/*
-	 * Prevent thread from going any further if module is (being)
-	 * unloaded.
-	 */
-	mtx_lock(&refcnt_mtx);
-	if (refcount < 0 ) {
-		mtx_unlock(&refcnt_mtx);
-		return (ENOSYS);
-	}
-	refcount++; /* Indicate that thread is active in the code-path */
-	mtx_unlock(&refcnt_mtx);
-
 	msqid = IPCID_TO_IX(msqid);
 
 	if (msqid < 0 || msqid >= msginfo.msgmni) {
 		DPRINTF(("msqid (%d) out of range (0<=msqid<%d)\n", msqid,
 		    msginfo.msgmni));
-		error = EINVAL;
-		goto done3;
+		return (EINVAL);
 	}
 	if (cmd == IPC_SET &&
 	    (error = copyin(user_msqptr, &msqbuf, sizeof(msqbuf))) != 0)
-		goto done3;
+		return (error);
 
 	msqkptr = &msqids[msqid];
 
@@ -561,10 +522,6 @@
 	mtx_unlock(&msq_mtx);
 	if (cmd == IPC_STAT && error == 0)
 		error = copyout(&(msqkptr->u), user_msqptr, sizeof(struct msqid_ds));
-done3:
-	mtx_lock(&refcnt_mtx);
-	refcount--; /* Indicate that thread no longer active in the code-path */
-	mtx_unlock(&refcnt_mtx);
 	return(error);
 }
 
@@ -594,18 +551,6 @@
 	if (!jail_sysvipc_allowed && jailed(td->td_ucred))
 		return (ENOSYS);
 
-	/*
-	 * Prevent thread from going any further if module is (being)
-	 * unloaded.
-	 */
-	mtx_lock(&refcnt_mtx);
-	if (refcount < 0 ) {
-		mtx_unlock(&refcnt_mtx);
-		return (ENOSYS);
-	}
-	refcount++;	/* Indicate that thread is active in the code-path */
-	mtx_unlock(&refcnt_mtx);
-
 	mtx_lock(&msq_mtx);
 	if (key != IPC_PRIVATE) {
 		for (msqid = 0; msqid < msginfo.msgmni; msqid++) {
@@ -690,9 +635,6 @@
 	td->td_retval[0] = IXSEQ_TO_IPCID(msqid, msqkptr->u.msg_perm);
 done2:
 	mtx_unlock(&msq_mtx);
-	mtx_lock(&refcnt_mtx);
-	refcount--; /* Indicate that thread no longer active in the code-path */
-	mtx_unlock(&refcnt_mtx);
 	return (error);
 }
 
@@ -727,18 +669,6 @@
 	if (!jail_sysvipc_allowed && jailed(td->td_ucred))
 		return (ENOSYS);
 
-	/*
-	 * Prevent thread from going any further if module is (being)
-	 * unloaded.
-	 */
-	mtx_lock(&refcnt_mtx);
-	if (refcount < 0 ) {
-		mtx_unlock(&refcnt_mtx);
-		return (ENOSYS);
-	}
-	refcount++;	/* Indicate that thread is active in the code-path */
-	mtx_unlock(&refcnt_mtx);
-
 	mtx_lock(&msq_mtx);
 	msqid = IPCID_TO_IX(msqid);
 
@@ -1046,9 +976,6 @@
 	td->td_retval[0] = 0;
 done2:
 	mtx_unlock(&msq_mtx);
-	mtx_lock(&refcnt_mtx);
-	refcount--; /* Indicate that thread no longer active in the code-path */
-	mtx_unlock(&refcnt_mtx);
 	return (error);
 }
 
@@ -1087,25 +1014,12 @@
 	if (!jail_sysvipc_allowed && jailed(td->td_ucred))
 		return (ENOSYS);
 
-	/*
-	 * Prevent thread from going any further if module is (being)
-	 * unloaded.
-	 */
-	mtx_lock(&refcnt_mtx);
-	if (refcount < 0 ) {
-		mtx_unlock(&refcnt_mtx);
-		return (ENOSYS);
-	}
-	refcount++;	/* Indicate that thread is active in the code-path */
-	mtx_unlock(&refcnt_mtx);
-
 	msqid = IPCID_TO_IX(msqid);
 
 	if (msqid < 0 || msqid >= msginfo.msgmni) {
 		DPRINTF(("msqid (%d) out of range (0<=msqid<%d)\n", msqid,
 		    msginfo.msgmni));
-		error = EINVAL;
-		goto done3;
+		return (EINVAL);
 	}
 
 	msqkptr = &msqids[msqid];
@@ -1366,10 +1280,6 @@
 	td->td_retval[0] = msgsz;
 done2:
 	mtx_unlock(&msq_mtx);
-done3:
-	mtx_lock(&refcnt_mtx);
-	refcount--; /* Indicate that thread no longer active in the code-path */
-	mtx_unlock(&refcnt_mtx);
 	return (error);
 }