Date: Tue, 7 Oct 2014 02:52:37 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: svn commit: r272596 - head/sys/fs/devfs Message-ID: <20141007005237.GA32651@dft-labs.eu> In-Reply-To: <13670379.opPJl0kA6Z@ralph.baldwin.cx> References: <201410060620.s966Kaqt078736@svn.freebsd.org> <13670379.opPJl0kA6Z@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Oct 06, 2014 at 11:37:32AM -0400, John Baldwin wrote: > On Monday, October 06, 2014 06:20:36 AM Mateusz Guzik wrote: > > Author: mjg > > Date: Mon Oct 6 06:20:35 2014 > > New Revision: 272596 > > URL: https://svnweb.freebsd.org/changeset/base/272596 > > > > Log: > > devfs: don't take proctree_lock unconditionally in devfs_close > > > > MFC after: 1 week > > Just for my sanity: > > What keeps td->td_proc->p_session static in this case so that it is safe to > dereference it? Specifically, if you are preempted after reading p_session > but before you then read s_ttyvp, what prevents a race with another thread > changing the session of td->td_proc? > Right, it's buggy. Turns out devfs was quite liberal in relation to that even prior to my change. How about: diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c index d7009a4..a480e4f 100644 --- a/sys/fs/devfs/devfs_vnops.c +++ b/sys/fs/devfs/devfs_vnops.c @@ -499,6 +499,7 @@ devfs_access(struct vop_access_args *ap) { struct vnode *vp = ap->a_vp; struct devfs_dirent *de; + struct proc *p; int error; de = vp->v_data; @@ -511,11 +512,14 @@ devfs_access(struct vop_access_args *ap) return (0); if (error != EACCES) return (error); + p = ap->a_td->td_proc; /* We do, however, allow access to the controlling terminal */ - if (!(ap->a_td->td_proc->p_flag & P_CONTROLT)) + if (!(p->p_flag & P_CONTROLT)) return (error); - if (ap->a_td->td_proc->p_session->s_ttydp == de->de_cdp) - return (0); + PROC_LOCK(p); + if (p->p_session->s_ttydp == de->de_cdp) + error = 0; + PROC_UNLOCK(p); return (error); } @@ -525,6 +529,7 @@ devfs_close(struct vop_close_args *ap) { struct vnode *vp = ap->a_vp, *oldvp; struct thread *td = ap->a_td; + struct proc *p; struct cdev *dev = vp->v_rdev; struct cdevsw *dsw; int vp_locked, error, ref; @@ -545,24 +550,30 @@ devfs_close(struct vop_close_args *ap) * if the reference count is 2 (this last descriptor * plus the session), release the reference from the session. */ - if (td && vp == td->td_proc->p_session->s_ttyvp) { - oldvp = NULL; - sx_xlock(&proctree_lock); - if (vp == td->td_proc->p_session->s_ttyvp) { - SESS_LOCK(td->td_proc->p_session); - VI_LOCK(vp); - if (count_dev(dev) == 2 && - (vp->v_iflag & VI_DOOMED) == 0) { - td->td_proc->p_session->s_ttyvp = NULL; - td->td_proc->p_session->s_ttydp = NULL; - oldvp = vp; + if (td != NULL) { + p = td->td_proc; + PROC_LOCK(p); + if (vp == p->p_session->s_ttyvp) { + PROC_UNLOCK(p); + oldvp = NULL; + sx_xlock(&proctree_lock); + if (vp == p->p_session->s_ttyvp) { + SESS_LOCK(p->p_session); + VI_LOCK(vp); + if (count_dev(dev) == 2 && + (vp->v_iflag & VI_DOOMED) == 0) { + p->p_session->s_ttyvp = NULL; + p->p_session->s_ttydp = NULL; + oldvp = vp; + } + VI_UNLOCK(vp); + SESS_UNLOCK(p->p_session); } - VI_UNLOCK(vp); - SESS_UNLOCK(td->td_proc->p_session); - } - sx_xunlock(&proctree_lock); - if (oldvp != NULL) - vrele(oldvp); + sx_xunlock(&proctree_lock); + if (oldvp != NULL) + vrele(oldvp); + } else + PROC_UNLOCK(p); } /* * We do not want to really close the device if it @@ -814,6 +825,7 @@ devfs_prison_check(struct devfs_dirent *de, struct thread *td) { struct cdev_priv *cdp; struct ucred *dcr; + struct proc *p; int error; cdp = de->de_cdp; @@ -827,10 +839,13 @@ devfs_prison_check(struct devfs_dirent *de, struct thread *td) if (error == 0) return (0); /* We do, however, allow access to the controlling terminal */ - if (!(td->td_proc->p_flag & P_CONTROLT)) + p = td->td_proc; + if (!(p->p_flag & P_CONTROLT)) return (error); - if (td->td_proc->p_session->s_ttydp == cdp) - return (0); + PROC_LOCK(p); + if (p->p_session->s_ttydp == cdp) + error = 0; + PROC_UNLOCK(p); return (error); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141007005237.GA32651>