Module Name: src
Committed By: riastradh
Date: Thu Oct 6 19:58:41 UTC 2022
Modified Files:
src/sys/dev: cons.c
src/sys/kern: subr_prf.c tty.c
src/sys/sys: tty.h
Log Message:
constty(4): Make MP-safe.
Access to the global constty variable is coordinated as follows:
1. Setting constty to nonnull, with atomic_store_release, is allowed
only under the new adaptive constty_lock in thread context. This
serializes TIOCCONS operations and ensures unlocked readers can
safely use a constty pointer read with atomic_load_consume.
2. Changing constty from nonnull to null, with atomic_cas_ptr, is
allowed in any context -- printf(9) uses this to disable a broken
constty.
3. Reading constty under constty_lock is allowed with
atomic_load_relaxed, because while constty_lock is held, it can
only be made null by some other thread/CPU, never made nonnull.
4. Reading constty outside constty_lock is allowed with
atomic_load_consume in a pserialize read section -- constty is
only ever made nonnull with atomic_store_release, in (1).
ttyclose will wait for all these pserialize read sections to
complete before flushing the tty.
5. To continue to use a struct tty pointer in (4) after the
pserialize read section has completed, caller must use tty_acquire
during the pserialize read section and then tty_release when done.
ttyclose will wait for all these references to drain before
returning.
These access rules allow us to serialize TIOCCONS, and safely destroy
ttys, without putting any locks on the access paths like printf(9)
that use constty. Once we set D_MPSAFE, operations on /dev/console
will contend only with other users of the same tty as constty, which
will be an improvement over contending with all other kernel lock
users in the system.
Changes second time around:
- Fix initialization of ok in cons.c cn_redirect.
- Fix reversed sense of conditional in subr_prf.c putone.
To generate a diff of this commit:
cvs rdiff -u -r1.86 -r1.87 src/sys/dev/cons.c
cvs rdiff -u -r1.191 -r1.192 src/sys/kern/subr_prf.c
cvs rdiff -u -r1.303 -r1.304 src/sys/kern/tty.c
cvs rdiff -u -r1.99 -r1.100 src/sys/sys/tty.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/cons.c
diff -u src/sys/dev/cons.c:1.86 src/sys/dev/cons.c:1.87
--- src/sys/dev/cons.c:1.86 Tue Oct 4 05:20:01 2022
+++ src/sys/dev/cons.c Thu Oct 6 19:58:41 2022
@@ -1,4 +1,4 @@
-/* $NetBSD: cons.c,v 1.86 2022/10/04 05:20:01 riastradh Exp $ */
+/* $NetBSD: cons.c,v 1.87 2022/10/06 19:58:41 riastradh Exp $ */
/*
* Copyright (c) 1988 University of Utah.
@@ -39,7 +39,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: cons.c,v 1.86 2022/10/04 05:20:01 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: cons.c,v 1.87 2022/10/06 19:58:41 riastradh Exp $");
#include <sys/param.h>
#include <sys/proc.h>
@@ -54,6 +54,8 @@ __KERNEL_RCSID(0, "$NetBSD: cons.c,v 1.8
#include <sys/kauth.h>
#include <sys/mutex.h>
#include <sys/module.h>
+#include <sys/atomic.h>
+#include <sys/pserialize.h>
#include <dev/cons.h>
@@ -67,7 +69,8 @@ dev_type_ioctl(cnioctl);
dev_type_poll(cnpoll);
dev_type_kqfilter(cnkqfilter);
-static bool cn_redirect(dev_t *, int, int *);
+static bool cn_redirect(dev_t *, int, int *, struct tty **);
+static void cn_release(struct tty *);
const struct cdevsw cons_cdevsw = {
.d_open = cnopen,
@@ -86,7 +89,7 @@ const struct cdevsw cons_cdevsw = {
static struct kmutex cn_lock;
-struct tty *constty = NULL; /* virtual console output device */
+struct tty *volatile constty; /* virtual console output device */
struct consdev *cn_tab; /* physical console device info */
struct vnode *cn_devvp[2]; /* vnode for underlying device. */
@@ -199,6 +202,7 @@ out: mutex_exit(&cn_lock);
int
cnread(dev_t dev, struct uio *uio, int flag)
{
+ struct tty *ctp = NULL;
int error;
/*
@@ -208,25 +212,31 @@ cnread(dev_t dev, struct uio *uio, int f
* input (except a shell in single-user mode, but then,
* one wouldn't TIOCCONS then).
*/
- if (!cn_redirect(&dev, 1, &error))
+ if (!cn_redirect(&dev, 1, &error, &ctp))
return error;
- return cdev_read(dev, uio, flag);
+ error = cdev_read(dev, uio, flag);
+ cn_release(ctp);
+ return error;
}
int
cnwrite(dev_t dev, struct uio *uio, int flag)
{
+ struct tty *ctp = NULL;
int error;
/* Redirect output, if that's appropriate. */
- if (!cn_redirect(&dev, 0, &error))
+ if (!cn_redirect(&dev, 0, &error, &ctp))
return error;
- return cdev_write(dev, uio, flag);
+ error = cdev_write(dev, uio, flag);
+ cn_release(ctp);
+ return error;
}
int
cnioctl(dev_t dev, u_long cmd, void *data, int flag, struct lwp *l)
{
+ struct tty *ctp = NULL;
int error;
error = 0;
@@ -235,29 +245,41 @@ cnioctl(dev_t dev, u_long cmd, void *dat
* Superuser can always use this to wrest control of console
* output from the "virtual" console.
*/
- if (cmd == TIOCCONS && constty != NULL) {
+ if (cmd == TIOCCONS) {
+ struct tty *tp;
+
+ mutex_enter(&constty_lock);
+ tp = atomic_load_relaxed(&constty);
+ if (tp == NULL) {
+ mutex_exit(&constty_lock);
+ goto passthrough; /* XXX ??? */
+ }
error = kauth_authorize_device_tty(l->l_cred,
- KAUTH_DEVICE_TTY_VIRTUAL, constty);
+ KAUTH_DEVICE_TTY_VIRTUAL, tp);
if (!error)
- constty = NULL;
- return (error);
+ atomic_store_relaxed(&constty, NULL);
+ mutex_exit(&constty_lock);
+ return error;
}
-
+passthrough:
/*
* Redirect the ioctl, if that's appropriate.
* Note that strange things can happen, if a program does
* ioctls on /dev/console, then the console is redirected
* out from under it.
*/
- if (!cn_redirect(&dev, 0, &error))
+ if (!cn_redirect(&dev, 0, &error, &ctp))
return error;
- return cdev_ioctl(dev, cmd, data, flag, l);
+ error = cdev_ioctl(dev, cmd, data, flag, l);
+ cn_release(ctp);
+ return error;
}
/*ARGSUSED*/
int
cnpoll(dev_t dev, int events, struct lwp *l)
{
+ struct tty *ctp = NULL;
int error;
/*
@@ -265,15 +287,18 @@ cnpoll(dev_t dev, int events, struct lwp
* I don't want to think of the possible side effects
* of console redirection here.
*/
- if (!cn_redirect(&dev, 0, &error))
+ if (!cn_redirect(&dev, 0, &error, &ctp))
return POLLHUP;
- return cdev_poll(dev, events, l);
+ error = cdev_poll(dev, events, l);
+ cn_release(ctp);
+ return error;
}
/*ARGSUSED*/
int
cnkqfilter(dev_t dev, struct knote *kn)
{
+ struct tty *ctp = NULL;
int error;
/*
@@ -281,9 +306,11 @@ cnkqfilter(dev_t dev, struct knote *kn)
* I don't want to think of the possible side effects
* of console redirection here.
*/
- if (!cn_redirect(&dev, 0, &error))
+ if (!cn_redirect(&dev, 0, &error, &ctp))
return error;
- return cdev_kqfilter(dev, kn);
+ error = cdev_kqfilter(dev, kn);
+ cn_release(ctp);
+ return error;
}
int
@@ -429,28 +456,44 @@ cnhalt(void)
/*
* Redirect output, if that's appropriate. If there's no real console,
* return ENXIO.
- *
- * Call with tty_mutex held.
*/
static bool
-cn_redirect(dev_t *devp, int is_read, int *error)
+cn_redirect(dev_t *devp, int is_read, int *error, struct tty **ctpp)
{
dev_t dev = *devp;
+ struct tty *ctp;
+ int s;
+ bool ok = false;
*error = ENXIO;
- if (constty != NULL && minor(dev) == 0 &&
+ *ctpp = NULL;
+ s = pserialize_read_enter();
+ if ((ctp = atomic_load_consume(&constty)) != NULL && minor(dev) == 0 &&
(cn_tab == NULL || (cn_tab->cn_pri != CN_REMOTE))) {
if (is_read) {
*error = 0;
- return false;
+ goto out;
}
- dev = constty->t_dev;
+ tty_acquire(ctp);
+ *ctpp = ctp;
+ dev = ctp->t_dev;
} else if (cn_tab == NULL)
- return false;
+ goto out;
else
dev = cn_tab->cn_dev;
+ ok = true;
*devp = dev;
- return true;
+out: pserialize_read_exit(s);
+ return ok;
+}
+
+static void
+cn_release(struct tty *ctp)
+{
+
+ if (ctp == NULL)
+ return;
+ tty_release(ctp);
}
MODULE(MODULE_CLASS_DRIVER, cons, NULL);
Index: src/sys/kern/subr_prf.c
diff -u src/sys/kern/subr_prf.c:1.191 src/sys/kern/subr_prf.c:1.192
--- src/sys/kern/subr_prf.c:1.191 Tue Oct 4 05:20:01 2022
+++ src/sys/kern/subr_prf.c Thu Oct 6 19:58:41 2022
@@ -1,4 +1,4 @@
-/* $NetBSD: subr_prf.c,v 1.191 2022/10/04 05:20:01 riastradh Exp $ */
+/* $NetBSD: subr_prf.c,v 1.192 2022/10/06 19:58:41 riastradh Exp $ */
/*-
* Copyright (c) 1986, 1988, 1991, 1993
@@ -37,7 +37,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: subr_prf.c,v 1.191 2022/10/04 05:20:01 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_prf.c,v 1.192 2022/10/06 19:58:41 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_ddb.h"
@@ -103,7 +103,6 @@ static void kprintf_internal(const char
* globals
*/
-extern struct tty *constty; /* pointer to console "window" tty */
extern int log_open; /* subr_log: is /dev/klog open? */
const char *panicstr; /* arg to first call to panic (used as a flag
to indicate that panic has already been called). */
@@ -401,22 +400,35 @@ addlog(const char *fmt, ...)
static void
putone(int c, int flags, struct tty *tp)
{
+ struct tty *ctp;
+ int s;
+
+ /*
+ * Ensure whatever constty points to can't go away while we're
+ * trying to use it.
+ */
+ s = pserialize_read_enter();
+
if (panicstr)
- constty = NULL;
+ atomic_store_relaxed(&constty, NULL);
- if ((flags & TOCONS) && tp == NULL && constty) {
- tp = constty;
+ if ((flags & TOCONS) &&
+ (ctp = atomic_load_consume(&constty)) != NULL &&
+ tp == NULL) {
+ tp = ctp;
flags |= TOTTY;
}
if ((flags & TOTTY) && tp &&
tputchar(c, flags, tp) < 0 &&
- (flags & TOCONS) && tp == constty)
- constty = NULL;
+ (flags & TOCONS))
+ atomic_cas_ptr(&constty, tp, NULL);
if ((flags & TOLOG) &&
c != '\0' && c != '\r' && c != 0177)
logputchar(c);
- if ((flags & TOCONS) && constty == NULL && c != '\0')
+ if ((flags & TOCONS) && ctp == NULL && c != '\0')
(*v_putc)(c);
+
+ pserialize_read_exit(s);
}
static void
Index: src/sys/kern/tty.c
diff -u src/sys/kern/tty.c:1.303 src/sys/kern/tty.c:1.304
--- src/sys/kern/tty.c:1.303 Tue Oct 4 05:20:01 2022
+++ src/sys/kern/tty.c Thu Oct 6 19:58:41 2022
@@ -1,4 +1,4 @@
-/* $NetBSD: tty.c,v 1.303 2022/10/04 05:20:01 riastradh Exp $ */
+/* $NetBSD: tty.c,v 1.304 2022/10/06 19:58:41 riastradh Exp $ */
/*-
* Copyright (c) 2008, 2020 The NetBSD Foundation, Inc.
@@ -63,7 +63,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: tty.c,v 1.303 2022/10/04 05:20:01 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: tty.c,v 1.304 2022/10/06 19:58:41 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_compat_netbsd.h"
@@ -99,6 +99,9 @@ __KERNEL_RCSID(0, "$NetBSD: tty.c,v 1.30
#include <sys/module.h>
#include <sys/bitops.h>
#include <sys/compat_stub.h>
+#include <sys/atomic.h>
+#include <sys/condvar.h>
+#include <sys/pserialize.h>
#ifdef COMPAT_60
#include <compat/sys/ttycom.h>
@@ -209,6 +212,9 @@ static void *tty_sigsih;
struct ttylist_head ttylist = TAILQ_HEAD_INITIALIZER(ttylist);
int tty_count;
kmutex_t tty_lock;
+kmutex_t constty_lock;
+static struct pserialize *constty_psz;
+static kcondvar_t ttyref_cv;
struct ptm_pty *ptm = NULL;
@@ -442,13 +448,28 @@ ttycancel(struct tty *tp)
int
ttyclose(struct tty *tp)
{
- extern struct tty *constty; /* Temporary virtual console. */
struct session *sess;
- mutex_spin_enter(&tty_lock);
+ /*
+ * Make sure this is not the constty. Without constty_lock it
+ * is always allowed to transition from nonnull to null.
+ */
+ (void)atomic_cas_ptr(&constty, tp, NULL);
- if (constty == tp)
- constty = NULL;
+ /*
+ * We don't know if this has _ever_ been the constty: another
+ * thread may have kicked it out as constty before we started
+ * to close.
+ *
+ * So we wait for all users that might be acquiring references
+ * to finish doing so -- after that, no more references can be
+ * made, at which point we can safely flush the tty, wait for
+ * the existing references to drain, and finally free or reuse
+ * the tty.
+ */
+ pserialize_perform(constty_psz);
+
+ mutex_spin_enter(&tty_lock);
ttyflush(tp, FREAD | FWRITE);
@@ -458,6 +479,9 @@ ttyclose(struct tty *tp)
sess = tp->t_session;
tp->t_session = NULL;
+ while (tp->t_refcnt)
+ cv_wait(&ttyref_cv, &tty_lock);
+
mutex_spin_exit(&tty_lock);
if (sess != NULL) {
@@ -474,6 +498,44 @@ ttyclose(struct tty *tp)
}
/*
+ * tty_acquire(tp), tty_release(tp)
+ *
+ * Acquire a reference to tp that prevents it from being closed
+ * until released. Caller must guarantee tp has not yet been
+ * closed, e.g. by obtaining tp from constty during a pserialize
+ * read section. Caller must not hold tty_lock.
+ */
+void
+tty_acquire(struct tty *tp)
+{
+ unsigned refcnt __diagused;
+
+ refcnt = atomic_inc_uint_nv(&tp->t_refcnt);
+ KASSERT(refcnt < UINT_MAX);
+}
+
+void
+tty_release(struct tty *tp)
+{
+ unsigned old, new;
+
+ KDASSERT(mutex_ownable(&tty_lock));
+
+ do {
+ old = atomic_load_relaxed(&tp->t_refcnt);
+ if (old == 1) {
+ mutex_spin_enter(&tty_lock);
+ if (atomic_dec_uint_nv(&tp->t_refcnt) == 0)
+ cv_broadcast(&ttyref_cv);
+ mutex_spin_exit(&tty_lock);
+ return;
+ }
+ KASSERT(old != 0);
+ new = old - 1;
+ } while (atomic_cas_uint(&tp->t_refcnt, old, new) != old);
+}
+
+/*
* This macro is used in canonical mode input processing, where a read
* request shall not return unless a 'line delimiter' ('\n') or 'break'
* (EOF, EOL, EOL2) character (or a signal) has been received. As EOL2
@@ -941,7 +1003,6 @@ ttyoutput(int c, struct tty *tp)
int
ttioctl(struct tty *tp, u_long cmd, void *data, int flag, struct lwp *l)
{
- extern struct tty *constty; /* Temporary virtual console. */
struct proc *p;
struct linesw *lp;
int s, error;
@@ -1044,32 +1105,47 @@ ttioctl(struct tty *tp, u_long cmd, void
mutex_spin_exit(&tty_lock);
break;
}
- case TIOCCONS: /* become virtual console */
+ case TIOCCONS: { /* become virtual console */
+ struct tty *ctp;
+
+ mutex_enter(&constty_lock);
+ error = 0;
+ ctp = atomic_load_relaxed(&constty);
if (*(int *)data) {
- if (constty && constty != tp &&
- ISSET(constty->t_state, TS_CARR_ON | TS_ISOPEN) ==
- (TS_CARR_ON | TS_ISOPEN))
- return EBUSY;
+ if (ctp != NULL && ctp != tp &&
+ ISSET(ctp->t_state, TS_CARR_ON | TS_ISOPEN) ==
+ (TS_CARR_ON | TS_ISOPEN)) {
+ error = EBUSY;
+ goto unlock_constty;
+ }
pb = pathbuf_create("/dev/console");
if (pb == NULL) {
- return ENOMEM;
+ error = ENOMEM;
+ goto unlock_constty;
}
NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, pb);
if ((error = namei(&nd)) != 0) {
pathbuf_destroy(pb);
- return error;
+ goto unlock_constty;
}
error = VOP_ACCESS(nd.ni_vp, VREAD, l->l_cred);
vput(nd.ni_vp);
pathbuf_destroy(pb);
if (error)
- return error;
+ goto unlock_constty;
- constty = tp;
- } else if (tp == constty)
- constty = NULL;
+ KASSERT(atomic_load_relaxed(&constty) == ctp ||
+ atomic_load_relaxed(&constty) == NULL);
+ atomic_store_release(&constty, tp);
+ } else if (tp == ctp) {
+ atomic_store_relaxed(&constty, NULL);
+ }
+unlock_constty: mutex_exit(&constty_lock);
+ if (error)
+ return error;
break;
+ }
case TIOCDRAIN: /* wait till output drained */
if ((error = ttywait(tp)) != 0)
return (error);
@@ -2968,6 +3044,8 @@ tty_init(void)
{
mutex_init(&tty_lock, MUTEX_DEFAULT, IPL_VM);
+ mutex_init(&constty_lock, MUTEX_DEFAULT, IPL_NONE);
+ constty_psz = pserialize_create();
tty_sigsih = softint_establish(SOFTINT_CLOCK, ttysigintr, NULL);
KASSERT(tty_sigsih != NULL);
Index: src/sys/sys/tty.h
diff -u src/sys/sys/tty.h:1.99 src/sys/sys/tty.h:1.100
--- src/sys/sys/tty.h:1.99 Tue Oct 4 05:20:02 2022
+++ src/sys/sys/tty.h Thu Oct 6 19:58:41 2022
@@ -1,4 +1,4 @@
-/* $NetBSD: tty.h,v 1.99 2022/10/04 05:20:02 riastradh Exp $ */
+/* $NetBSD: tty.h,v 1.100 2022/10/06 19:58:41 riastradh Exp $ */
/*-
* Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -149,6 +149,7 @@ struct tty {
int t_sigcount; /* # pending signals */
TAILQ_ENTRY(tty) t_sigqueue; /* entry on pending signal list */
void *t_softc; /* pointer to driver's softc. */
+ volatile unsigned t_refcnt; /* reference count for constty */
};
#ifdef TTY_ALLOW_PRIVATE
@@ -252,6 +253,8 @@ TAILQ_HEAD(ttylist_head, tty); /* the t
#ifdef _KERNEL
extern kmutex_t tty_lock;
+extern kmutex_t constty_lock;
+extern struct tty *volatile constty;
extern int tty_count; /* number of ttys in global ttylist */
extern struct ttychars ttydefaults;
@@ -313,6 +316,8 @@ void tty_free(struct tty *);
u_char *firstc(struct clist *, int *);
bool ttypull(struct tty *);
int tty_unit(dev_t);
+void tty_acquire(struct tty *);
+void tty_release(struct tty *);
int clalloc(struct clist *, int, int);
void clfree(struct clist *);
