svn commit: r272596 - head/sys/fs/devfs

[Mateusz Guzik]

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);
 }