From owner-svn-src-head@FreeBSD.ORG Tue Oct 7 00:52:43 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 951BE4E7; Tue, 7 Oct 2014 00:52:43 +0000 (UTC) Received: from mail-wg0-x22a.google.com (mail-wg0-x22a.google.com [IPv6:2a00:1450:400c:c00::22a]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 8E23F612; Tue, 7 Oct 2014 00:52:42 +0000 (UTC) Received: by mail-wg0-f42.google.com with SMTP id z12so7904239wgg.1 for ; Mon, 06 Oct 2014 17:52:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=XV940Ik7J3WJdItDomZRn3vq2OSJj1pYDjueTfbRCvk=; b=EUkO1bOXI1rXRog2DRit2zPi7F+YT+Y3oWwgNmte96/gry2C1HVvdhiGFl/hN9dtPz JSUUcGrwNpFr/K+IEl+ANM3+J7V/FHGXtGGyVOoLk1RuZy/63IhBAAkxmYJmWZsrVtQc UTRehzIhsQwaeGWOm//E/uJA68fE2vtbARGDrdxSU174kavHXS91UAOL5WzT1jkolXdD 2x5OLOKckH8Ivve/h9sHjB0lRw3Lmc1HRaEKEpOzCNXpqLupnuDLs42S7jxxWPMusVSf sAa5MJJKbANyC+W0i+9f2YQLpot4MYROA81Y7amlTJYq6AyK0iT9lRTv9Y5umsCA+iGO Dswg== X-Received: by 10.180.182.166 with SMTP id ef6mr7024702wic.10.1412643160838; Mon, 06 Oct 2014 17:52:40 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id o1sm18861574wja.25.2014.10.06.17.52.39 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 06 Oct 2014 17:52:40 -0700 (PDT) Date: Tue, 7 Oct 2014 02:52:37 +0200 From: Mateusz Guzik To: John Baldwin Subject: Re: svn commit: r272596 - head/sys/fs/devfs Message-ID: <20141007005237.GA32651@dft-labs.eu> References: <201410060620.s966Kaqt078736@svn.freebsd.org> <13670379.opPJl0kA6Z@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <13670379.opPJl0kA6Z@ralph.baldwin.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Oct 2014 00:52:43 -0000 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); }