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

[John Baldwin]

On Monday, October 06, 2014 8:52:37 pm Mateusz Guzik wrote:
> 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);
>  }

I think this is fine (and I've had one of these fixes in a side branch forever
myself).  In my version I held PROC_LOCK while checking P_CONTROLT.  I think
you might need that in case another thread in the same process is invoking
TIOCSCTTY.  (I am assuming that the thread variables in devfs_access() and
devfs_prison_check() are in fact curthread.  If they weren't, I think you'd
definitely need the lock.  Since TIOCSCTTY acts on curthread, you could get
by without the lock for reading the flag for curproc for a single-threaded
process, but not for a multi-threaded one.)

-- 
John Baldwin