From nobody Sat Aug 29 09:43:46 2026 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4hX9LW6zDtz6qlDw for ; Sat, 29 Aug 2026 09:43:47 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "YR2" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4hX9LW040Cz3g5y for ; Sat, 29 Aug 2026 09:43:47 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1787996627; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=bUbpWiyGEnRh57gWn8OeOo2esd1C5HvMSLdvosn18T8=; b=vXl5cpo5B/VHN3XjOojFe8JkyBJsI5fxMKvp/SSqt0pcOg+LsoMTrFfei00UhRjvh05lk0 oICTqq0Qd6CK4MoQeGJPntEF5a1vQ9F2ILsf2jHO8gZz2BGjwMBKozXLEPl1phBK+7hlTK fgHTDdRtmDhM0gTKp/E+JpC+U1Wc9ob87YT56C7Xn/z0UXcIAYqjwTlE1cRjpaHqaEo0hR MEnbGpkZTY3Bk6RE5fwAaOjA89d/QelKHuF0huxw6RN3hL59318/X8ZMVTlevLGCfnzQL0 CyK28j/9MHJ7MVcKZh/sxuahL89ANS/sJkce1fQyy4VI5ljf53ECObeY4q1DbQ== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1787996627; a=rsa-sha256; cv=none; b=ZXeNBFDQeTAcfNOmphcL/09Rn3hR62mNVEQsib7uXDnyv6dzLX7lZiwm59Be40CwbWbg8I WJ8oVFTm8k8zpc5+nTObq+gybQgnLKxaxFVVLZeRHpaErPPeRaRF/RxnrIQbHgNk/FnmHu msNl26YE/2796vYl57qMIAxS/RhjiEIDD/YBvYaoQi0ttqmIF4UHaRJdFIryQCqv5ZRjlx NTdRSSuZFLQ91zU7e3uvhshJZCKm/CoS7oH5D9Uc2TkS+0ZF2vGqmZYPdgawj/Sbv4juJq LxhjJy650cNnB5A2rjVyrMHQeFncfE71YOHS8hlND4EMZnX1TZbSlZwQz9XxMA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1787996627; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=bUbpWiyGEnRh57gWn8OeOo2esd1C5HvMSLdvosn18T8=; b=tXNWqIH2OZtfr8JLKqI+LdSsX/JBVj6c3LCan5X9HHPlilqXU0HCEgzEkkYiYPeOFV7yJc R+kJwCfUPQJFXr2tdVVyrxDLh3B18tnH2FKz1/24b9M53w4xCAFLukaKrVf06snQ0xvpTu WqMcMBuU8m3ffV6o70ez0SS2/wHr0lDM104/0Xgoue7+O1fSypIOKtL/GL7JFCeBnh06D+ MlSMaZsllSD9BtYRDwabxhT7UWJifo0W5rxefAA2N0nSSnTDTxKlV0dk4jfJyfcWoLWhnw +5hggOKFpvjlL+MS24WgchbYKp4TI+Shf5gxgrZBUoqImqi+LizKzjy56sr5yw== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) by mxrelay.nyi.freebsd.org (Postfix) with ESMTP id 4hX9LV3NHSzbnJ for ; Sat, 29 Aug 2026 09:43:46 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from git (uid 1279) (envelope-from git@FreeBSD.org) id 47f56 by gitrepo.freebsd.org (DragonFly Mail Agent v0.13+ on gitrepo.freebsd.org); Sat, 29 Aug 2026 09:43:46 +0000 To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Konstantin Belousov Subject: git: 420428718da7 - main - tty: gracefully handle proctree_lock locking List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: kib X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 420428718da769ea72d3f18ed8eba7c3d998b1c8 Auto-Submitted: auto-generated Date: Sat, 29 Aug 2026 09:43:46 +0000 Message-Id: <6a92a9d2.47f56.fd0c5fb@gitrepo.freebsd.org> The branch main has been updated by kib: URL: https://cgit.FreeBSD.org/src/commit/?id=420428718da769ea72d3f18ed8eba7c3d998b1c8 commit 420428718da769ea72d3f18ed8eba7c3d998b1c8 Author: Konstantin Belousov AuthorDate: 2026-08-23 20:53:00 +0000 Commit: Konstantin Belousov CommitDate: 2026-08-29 09:43:07 +0000 tty: gracefully handle proctree_lock locking Instead of relocking tty to get the proctree_lock and experiencing the race due to the relock, take the proctree_lock in advance for ioctl commands that need it. The affected commands, TIOCNOTTY, TIOCSCTTY, and TIOCSPGRP, must not be overridden by the specific tty drivers, so the common handling is cleaner. Reviewed by: kevans (previous version), markj Tested by: pho Sponsored by: The FreeBSD Foundation MFC after: 1 week Differential revision: https://reviews.freebsd.org/D59132 --- sys/kern/tty.c | 381 +++++++++++++++++++++++++++++------------------------ sys/kern/tty_pts.c | 3 + sys/sys/tty.h | 3 + 3 files changed, 215 insertions(+), 172 deletions(-) diff --git a/sys/kern/tty.c b/sys/kern/tty.c index e1e9989cee09..100da93536fe 100644 --- a/sys/kern/tty.c +++ b/sys/kern/tty.c @@ -578,6 +578,207 @@ done: tty_unlock(tp); return (error); } +static int +tty_ioctl_cnotty(struct tty *tp, struct thread *td) +{ + struct session *session; + struct vnode *vp; + struct proc *p; + int error; + + p = td->td_proc; + error = 0; + + sx_xlock(&proctree_lock); + error = ttydev_enter(tp); + if (error != 0) + goto out_unlock2; + + /* + * If the session doesn't have a controlling TTY, or if we weren't + * invoked on the controlling TTY, we'll return ENOTTY as we've + * historically done. + */ + session = p->p_session; + if (session->s_ttyp == NULL || session->s_ttyp != tp) { + error = EXTERROR(ENOTTY, "no controlling tty"); + goto out_unlock1; + } + + if (!SESS_LEADER(p)) { + error = EXTERROR(EPERM, "not a session leader"); + goto out_unlock1; + } + + PROC_LOCK(p); + SESS_LOCK(session); + vp = session->s_ttyvp; + session->s_ttyp = NULL; + session->s_ttyvp = NULL; + session->s_ttydp = NULL; + SESS_UNLOCK(session); + + if (tp->t_session == session) { + tp->t_session = NULL; + tp->t_pgrp = NULL; + } + tp->t_sessioncnt--; + p->p_flag &= ~P_CONTROLT; + PROC_UNLOCK(p); + sx_xunlock(&proctree_lock); + + /* + * If we did have a vnode, release our reference. Ordinarily + * we manage these at the devfs layer, but we can't + * necessarily know that we were invoked on the vnode + * referenced in the session (i.e. the vnode we hold a + * reference to). We explicitly don't check VBAD/VIRF_DOOMED + * here to avoid a vnode leak -- in circumstances elsewhere + * where we'd hit a VIRF_DOOMED vnode, release has been + * deferred until the controlling TTY is either changed or + * released. + */ + if (vp != NULL) + devfs_ctty_unref(vp); + + tty_unlock(tp); + return (error); + +out_unlock1: + tty_unlock(tp); +out_unlock2: + sx_xunlock(&proctree_lock); + return (error); +} + +static int +ttydev_ioctl_sctty(struct tty *tp, caddr_t data, struct thread *td) +{ + struct proc *p; + int error; + + p = td->td_proc; + error = 0; + + error = ttydev_enter(tp); + if (error != 0) + return (error); + + error = tty_wait_background(tp, td, SIGTTOU, LA_XLOCKED); + if (error != 0) + goto out; + + if (!SESS_LEADER(p)) { + /* Only the session leader may do this. */ + error = EXTERROR(EPERM, "not a session leader"); + goto out; + } + + if (tp->t_session != NULL && tp->t_session == p->p_session) { + /* This is already our controlling TTY. */ + goto out; + } + + if (p->p_session->s_ttyp != NULL || + (tp->t_session != NULL && tp->t_session->s_ttyvp != NULL && + tp->t_session->s_ttyvp->v_type != VBAD)) { + /* + * There is already a relation between a TTY and + * a session, or the caller is not the session + * leader. + * + * Allow the TTY to be stolen when the vnode is + * invalid, but the reference to the TTY is + * still active. This allows immediate reuse of + * TTYs of which the session leader has been + * killed or the TTY revoked. + */ + error = EXTERROR(EPERM, "session already has CTTY"); + goto out; + } + + /* Connect the session to the TTY. */ + tp->t_session = p->p_session; + tp->t_session->s_ttyp = tp; + tp->t_sessioncnt++; + + /* Assign foreground process group. */ + tp->t_pgrp = p->p_pgrp; + PROC_LOCK(p); + p->p_flag |= P_CONTROLT; + PROC_UNLOCK(p); +out: + tty_unlock(tp); + return (error); +} + +static int +ttydev_ioctl_spgrp(struct tty *tp, caddr_t data, struct thread *td) +{ + struct pgrp *pg; + int error; + + error = ttydev_enter(tp); + if (error != 0) + return (error); + + error = tty_wait_background(tp, td, SIGTTOU, LA_SLOCKED); + if (error != 0) + goto out; + + pg = pgfind(*(int *)data); + if (pg != NULL) + PGRP_UNLOCK(pg); + if (pg == NULL || pg->pg_session != td->td_proc->p_session) { + error = EXTERROR(EPERM, + "pgrp %jd belongs to other session %jd", + pg != NULL ? pg->pg_id : -1, + td->td_proc->p_session->s_sid); + goto out; + } + + /* + * Determine if this TTY is the controlling TTY. + */ + if (!tty_is_ctty(tp, td->td_proc)) { + error = EXTERROR(ENOTTY, "not a controlling tty"); + goto out; + } + tp->t_pgrp = pg; + + /* Wake up the background process groups. */ + cv_broadcast(&tp->t_bgwait); +out: + tty_unlock(tp); + return (error); +} + +int +ttydev_ioctl_proctree(struct tty *tp, u_long cmd, caddr_t data, + struct thread *td) +{ + int error; + + switch (cmd) { + case TIOCNOTTY: + error = tty_ioctl_cnotty(tp, td); + break; + case TIOCSCTTY: + sx_xlock(&proctree_lock); + error = ttydev_ioctl_sctty(tp, data, td); + sx_xunlock(&proctree_lock); + break; + case TIOCSPGRP: + sx_slock(&proctree_lock); + error = ttydev_ioctl_spgrp(tp, data, td); + sx_sunlock(&proctree_lock); + break; + default: + __unreachable(); + } + return (error); +} + static int ttydev_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int fflag, struct thread *td) @@ -585,6 +786,9 @@ ttydev_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int fflag, struct tty *tp = dev->si_drv1; int error; + if (cmd == TIOCNOTTY || cmd == TIOCSCTTY || cmd == TIOCSPGRP) + return (ttydev_ioctl_proctree(tp, cmd, data, td)); + error = ttydev_enter(tp); if (error) return (error); @@ -597,11 +801,9 @@ ttydev_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int fflag, case TIOCFLUSH: case TIOCNXCL: case TIOCSBRK: - case TIOCSCTTY: case TIOCSETA: case TIOCSETAF: case TIOCSETAW: - case TIOCSPGRP: case TIOCSTART: case TIOCSTAT: case TIOCSTI: @@ -1243,75 +1445,6 @@ tty_rel_gone(struct tty *tp) tty_rel_free(tp, false); } -static int -tty_drop_ctty(struct tty *tp, struct proc *p) -{ - struct session *session; - struct vnode *vp; - - /* - * This looks terrible, but it's generally safe as long as the tty - * hasn't gone away while we had the lock dropped. All of our sanity - * checking that this operation is OK happens after we've picked it back - * up, so other state changes are generally not fatal and the potential - * for this particular operation to happen out-of-order in a - * multithreaded scenario is likely a non-issue. - */ - tty_unlock(tp); - sx_xlock(&proctree_lock); - tty_lock(tp); - if (tty_gone(tp)) { - sx_xunlock(&proctree_lock); - return (EXTERROR(ENODEV, "tty_drop_ctty: device is gone")); - } - - /* - * If the session doesn't have a controlling TTY, or if we weren't - * invoked on the controlling TTY, we'll return ENOTTY as we've - * historically done. - */ - session = p->p_session; - if (session->s_ttyp == NULL || session->s_ttyp != tp) { - sx_xunlock(&proctree_lock); - return (EXTERROR(ENOTTY, "no controlling tty")); - } - - if (!SESS_LEADER(p)) { - sx_xunlock(&proctree_lock); - return (EXTERROR(EPERM, "not a session leader")); - } - - PROC_LOCK(p); - SESS_LOCK(session); - vp = session->s_ttyvp; - session->s_ttyp = NULL; - session->s_ttyvp = NULL; - session->s_ttydp = NULL; - SESS_UNLOCK(session); - - if (tp->t_session == session) { - tp->t_session = NULL; - tp->t_pgrp = NULL; - } - tp->t_sessioncnt--; - p->p_flag &= ~P_CONTROLT; - PROC_UNLOCK(p); - sx_xunlock(&proctree_lock); - - /* - * If we did have a vnode, release our reference. Ordinarily we manage - * these at the devfs layer, but we can't necessarily know that we were - * invoked on the vnode referenced in the session (i.e. the vnode we - * hold a reference to). We explicitly don't check VBAD/VIRF_DOOMED here - * to avoid a vnode leak -- in circumstances elsewhere where we'd hit a - * VIRF_DOOMED vnode, release has been deferred until the controlling TTY - * is either changed or released. - */ - if (vp != NULL) - devfs_ctty_unref(vp); - return (0); -} - /* * Exposing information about current TTY's through sysctl */ @@ -1930,107 +2063,11 @@ tty_generic_ioctl(struct tty *tp, u_long cmd, void *data, int fflag, *(int *)data = tp->t_session->s_sid; return (0); case TIOCNOTTY: - return (tty_drop_ctty(tp, td->td_proc)); - case TIOCSCTTY: { - struct proc *p = td->td_proc; - - /* XXX: This looks awful. */ - tty_unlock(tp); - sx_xlock(&proctree_lock); - error = ttydev_enter(tp); - if (error != 0) { - sx_xunlock(&proctree_lock); - tty_lock(tp); - return (error); - } - - if (!SESS_LEADER(p)) { - /* Only the session leader may do this. */ - sx_xunlock(&proctree_lock); - return (EXTERROR(EPERM, "not a session leader")); - } - - if (tp->t_session != NULL && tp->t_session == p->p_session) { - /* This is already our controlling TTY. */ - sx_xunlock(&proctree_lock); - return (0); - } - - if (p->p_session->s_ttyp != NULL || - (tp->t_session != NULL && tp->t_session->s_ttyvp != NULL && - tp->t_session->s_ttyvp->v_type != VBAD)) { - /* - * There is already a relation between a TTY and - * a session, or the caller is not the session - * leader. - * - * Allow the TTY to be stolen when the vnode is - * invalid, but the reference to the TTY is - * still active. This allows immediate reuse of - * TTYs of which the session leader has been - * killed or the TTY revoked. - */ - sx_xunlock(&proctree_lock); - return (EXTERROR(EPERM, "session already has CTTY")); - } - - /* Connect the session to the TTY. */ - tp->t_session = p->p_session; - tp->t_session->s_ttyp = tp; - tp->t_sessioncnt++; - - /* Assign foreground process group. */ - tp->t_pgrp = p->p_pgrp; - PROC_LOCK(p); - p->p_flag |= P_CONTROLT; - PROC_UNLOCK(p); - - sx_xunlock(&proctree_lock); - return (0); - } - case TIOCSPGRP: { - struct pgrp *pg; - - /* - * XXX: Temporarily unlock the TTY to locate the process - * group. This code would be lot nicer if we would ever - * decompose proctree_lock. - */ - tty_unlock(tp); - sx_slock(&proctree_lock); - pg = pgfind(*(int *)data); - if (pg != NULL) - PGRP_UNLOCK(pg); - if (pg == NULL || pg->pg_session != td->td_proc->p_session) { - sx_sunlock(&proctree_lock); - tty_lock(tp); - return (EXTERROR(EPERM, - "pgrp %jd belongs to other session %jd", - pg != NULL ? pg->pg_id : -1, - td->td_proc->p_session->s_sid)); - } - error = ttydev_enter(tp); - if (error != 0) { - sx_sunlock(&proctree_lock); - tty_lock(tp); - return (error); - } - - /* - * Determine if this TTY is the controlling TTY after - * relocking the TTY. - */ - if (!tty_is_ctty(tp, td->td_proc)) { - sx_sunlock(&proctree_lock); - return (EXTERROR(ENOTTY, "not a controlling tty")); - } - tp->t_pgrp = pg; - sx_sunlock(&proctree_lock); - - /* Wake up the background process groups. */ - cv_broadcast(&tp->t_bgwait); - return (0); - } + panic("TIOCNOTTY"); + case TIOCSCTTY: + panic("TIOCSCTTY"); + case TIOCSPGRP: + panic("TIOCSPGRP"); case TIOCFLUSH: { int flags = *(int *)data; diff --git a/sys/kern/tty_pts.c b/sys/kern/tty_pts.c index 2672935c2d89..02fb1259dbee 100644 --- a/sys/kern/tty_pts.c +++ b/sys/kern/tty_pts.c @@ -261,6 +261,9 @@ ptsdev_ioctl(struct file *fp, u_long cmd, void *data, struct pts_softc *psc = tty_softc(tp); int error = 0, sig; + if (cmd == TIOCNOTTY || cmd == TIOCSCTTY || cmd == TIOCSPGRP) + return (ttydev_ioctl_proctree(tp, cmd, data, td)); + switch (cmd) { case FIODTYPE: *(int *)data = D_TTY; diff --git a/sys/sys/tty.h b/sys/sys/tty.h index 65cc88f861c2..54e11871381f 100644 --- a/sys/sys/tty.h +++ b/sys/sys/tty.h @@ -228,6 +228,9 @@ int pts_alloc(int fflags, struct thread *td, struct file *fp); int pts_alloc_external(int fd, struct thread *td, struct file *fp, struct cdev *dev, const char *name); +int ttydev_ioctl_proctree(struct tty *tp, u_long cmd, caddr_t data, + struct thread *td); + /* Drivers and line disciplines also need to call these. */ #include #include