From owner-p4-projects@FreeBSD.ORG Thu Jul 8 18:46:10 2004 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id D434A16A4D0; Thu, 8 Jul 2004 18:46:09 +0000 (GMT) Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id AD67216A4CE for ; Thu, 8 Jul 2004 18:46:09 +0000 (GMT) Received: from repoman.freebsd.org (repoman.freebsd.org [216.136.204.115]) by mx1.FreeBSD.org (Postfix) with ESMTP id 929CD43D39 for ; Thu, 8 Jul 2004 18:46:09 +0000 (GMT) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.12.11/8.12.11) with ESMTP id i68Ik9X9023626 for ; Thu, 8 Jul 2004 18:46:09 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.12.11/8.12.11/Submit) id i68Ik9u0023623 for perforce@freebsd.org; Thu, 8 Jul 2004 18:46:09 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Date: Thu, 8 Jul 2004 18:46:09 GMT Message-Id: <200407081846.i68Ik9u0023623@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to bb+lists.freebsd.perforce@cyrus.watson.org using -f From: Robert Watson To: Perforce Change Reviews Subject: PERFORCE change 56794 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Jul 2004 18:46:10 -0000 http://perforce.freebsd.org/chv.cgi?CH=56794 Change 56794 by rwatson@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); }