From owner-freebsd-smp Fri Feb 14 2:38:13 2003 Delivered-To: freebsd-smp@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 68BC837B401; Fri, 14 Feb 2003 02:38:09 -0800 (PST) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.FreeBSD.org (Postfix) with ESMTP id E17E843FBD; Fri, 14 Feb 2003 02:38:08 -0800 (PST) (envelope-from bright@elvis.mu.org) Received: by elvis.mu.org (Postfix, from userid 1192) id AE437AE147; Fri, 14 Feb 2003 02:38:08 -0800 (PST) Date: Fri, 14 Feb 2003 02:38:08 -0800 From: Alfred Perlstein To: current@freebsd.org Cc: smp@freebsd.org Subject: fix: lock order reversal proc/filedesc. Message-ID: <20030214103808.GE93252@elvis.mu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4i Sender: owner-freebsd-smp@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org Thanks to Paul Saab's work on fixing twe(4) I was able to get a crash dump from my box and figure out why my filedesc locking patch was panic'ing. kevent_close was dereferencing the proc's filedesc pointer during close, that doesn't work so well when you need to do what I had to do. :) The gist of the fix is that the mutex 'fdesc_mutex' is used as a global barrier against losing hold of the proc->p_fd reference. So, basically, if you are not the process that owns a filedesc you must grab the mutex over _all_ usage of it, and you probably want the allproc_lock as well to make sure you don't lose your process as well. :) (see sysctl_kern_file() for an example.) I've also fixed kqueue_close to use the stashed filedesc pointer inside the f_data pointer rather than trying to get at it via p->p_fd. please review/test: Index: kern/kern_descrip.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_descrip.c,v retrieving revision 1.185 diff -u -r1.185 kern_descrip.c --- kern/kern_descrip.c 11 Feb 2003 07:20:52 -0000 1.185 +++ kern/kern_descrip.c 14 Feb 2003 10:11:43 -0000 @@ -1366,6 +1366,10 @@ return (newfdp); } +/* A mutex to protect the association between a proc and filedesc. */ +struct mtx fdesc_mtx; +MTX_SYSINIT(fdesc, &fdesc_mtx, "fdesc", MTX_DEF); + /* * Release a filedesc structure. */ @@ -1382,6 +1386,10 @@ if (fdp == NULL) return; + mtx_lock(&fdesc_mtx); + td->td_proc->p_fd = NULL; + mtx_unlock(&fdesc_mtx); + FILEDESC_LOCK(fdp); if (--fdp->fd_refcnt > 0) { FILEDESC_UNLOCK(fdp); @@ -1398,7 +1406,6 @@ if (*fpp) (void) closef(*fpp, td); } - td->td_proc->p_fd = NULL; if (fdp->fd_nfiles > NDFILE) FREE(fdp->fd_ofiles, M_FILEDESC); if (fdp->fd_cdir) @@ -2105,7 +2112,9 @@ xf.xf_pid = p->p_pid; xf.xf_uid = p->p_ucred->cr_uid; PROC_UNLOCK(p); + mtx_lock(&fdesc_mtx); if ((fdp = p->p_fd) == NULL) { + mtx_unlock(&fdesc_mtx); continue; } FILEDESC_LOCK(fdp); @@ -2125,6 +2134,7 @@ break; } FILEDESC_UNLOCK(fdp); + mtx_unlock(&fdesc_mtx); if (error) break; } Index: kern/kern_event.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_event.c,v retrieving revision 1.54 diff -u -r1.54 kern_event.c --- kern/kern_event.c 21 Jan 2003 08:55:53 -0000 1.54 +++ kern/kern_event.c 14 Feb 2003 09:59:11 -0000 @@ -839,7 +840,7 @@ kqueue_close(struct file *fp, struct thread *td) { struct kqueue *kq = fp->f_data; - struct filedesc *fdp = td->td_proc->p_fd; + struct filedesc *fdp = kq->kq_fdp; struct knote **knp, *kn, *kn0; int i; Index: kern/kern_exit.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_exit.c,v retrieving revision 1.193 diff -u -r1.193 kern_exit.c --- kern/kern_exit.c 8 Feb 2003 02:58:16 -0000 1.193 +++ kern/kern_exit.c 13 Feb 2003 09:15:30 -0000 @@ -263,7 +263,7 @@ * Close open files and release open-file table. * This may block! */ - fdfree(td); /* XXXKSE *//* may not be the one in proc */ + fdfree(td); /* * Remove ourself from our leader's peer list and wake our leader. Index: kern/vfs_mount.c =================================================================== RCS file: /home/ncvs/src/sys/kern/vfs_mount.c,v retrieving revision 1.97 diff -u -r1.97 vfs_mount.c --- kern/vfs_mount.c 21 Jan 2003 08:55:55 -0000 1.97 +++ kern/vfs_mount.c 14 Feb 2003 10:09:16 -0000 @@ -1141,10 +1141,10 @@ return; sx_slock(&allproc_lock); LIST_FOREACH(p, &allproc, p_list) { - PROC_LOCK(p); + mtx_lock(&fdesc_mtx); fdp = p->p_fd; if (fdp == NULL) { - PROC_UNLOCK(p); + mtx_unlock(&fdesc_mtx); continue; } nrele = 0; @@ -1160,7 +1160,7 @@ nrele++; } FILEDESC_UNLOCK(fdp); - PROC_UNLOCK(p); + mtx_unlock(&fdesc_mtx); while (nrele--) vrele(olddp); } Index: sys/filedesc.h =================================================================== RCS file: /home/ncvs/src/sys/sys/filedesc.h,v retrieving revision 1.49 diff -u -r1.49 filedesc.h --- sys/filedesc.h 1 Jan 2003 01:56:19 -0000 1.49 +++ sys/filedesc.h 14 Feb 2003 10:12:59 -0000 @@ -139,6 +139,8 @@ return ((u_int)fd >= (u_int)fdp->fd_nfiles ? NULL : fdp->fd_ofiles[fd]); } +extern struct mtx fdesc_mtx; + #endif /* _KERNEL */ #endif /* !_SYS_FILEDESC_H_ */ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-smp" in the body of the message