The attached patch series makes /dev/console and /dev/constty MP-safe, with careful rules around constty access so it doesn't add contention or mutexes to the paths used by printf(9).
This passes some manual testing in qemu with TIOCCONS on a viocon(4) device, but since this touches a very intimate part of the kernel, I figure I should post it for review and testing before barging ahead and committing. So please give this a whirl on whatever systems you have that are a pain to get diagnostics out of, or on systems with X and xconsole or xterm that use TIOCCONS, so we can find console/constty bugs early before you need those diagnostics for something else!
>From b1dac0b91583b768a457ae946751e7c69c4554a1 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell <riastr...@netbsd.org> Date: Sat, 1 Oct 2022 14:00:43 +0000 Subject: [PATCH 1/5] cons(9): New function cn_set_tab. Increment of progress toward eliminating bare access to cn_tab so we can make more things MP-safe without the kernel lock (and maybe some day better formalize console detection and switching). --- sys/dev/cons.c | 16 ++++++++++++++++ sys/dev/cons.h | 2 ++ 2 files changed, 18 insertions(+) diff --git a/sys/dev/cons.c b/sys/dev/cons.c index eef38cbbc510..270fe6e1bbee 100644 --- a/sys/dev/cons.c +++ b/sys/dev/cons.c @@ -87,6 +87,22 @@ struct tty *constty = NULL; /* virtual console output device */ struct consdev *cn_tab; /* physical console device info */ struct vnode *cn_devvp[2]; /* vnode for underlying device. */ +void +cn_set_tab(struct consdev *tab) +{ + + /* + * This is a point that we should have KASSERT(cold) or add + * synchronization in case this can happen after cold boot. + * However, cn_tab initialization is so critical to any + * diagnostics or debugging that we need to tread carefully + * about introducing new ways to crash. So let's put the + * assertion in only after we've audited most or all of the + * cn_tab updates. + */ + cn_tab = tab; +} + int cnopen(dev_t dev, int flag, int mode, struct lwp *l) { diff --git a/sys/dev/cons.h b/sys/dev/cons.h index 7ccdc5d6e74c..0d0356a3e1ff 100644 --- a/sys/dev/cons.h +++ b/sys/dev/cons.h @@ -76,6 +76,8 @@ struct consdev { extern struct consdev constab[]; extern struct consdev *cn_tab; +void cn_set_tab(struct consdev *); + void cninit(void); int cngetc(void); int cngetsn(char *, int); >From 7d7b7315a8443f9c619d332ba1b897b34ec1f65b Mon Sep 17 00:00:00 2001 From: Taylor R Campbell <riastr...@netbsd.org> Date: Sat, 1 Oct 2022 14:03:22 +0000 Subject: [PATCH 2/5] cons(9): Serialize open and close. Kernel lock wasn't enough for this -- cdevvp, vn_lock, or VOP_OPEN could block, allowing another thread to re-enter open. --- sys/dev/cons.c | 49 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/sys/dev/cons.c b/sys/dev/cons.c index 270fe6e1bbee..1bb05a90fae1 100644 --- a/sys/dev/cons.c +++ b/sys/dev/cons.c @@ -53,6 +53,7 @@ __KERNEL_RCSID(0, "$NetBSD: cons.c,v 1.79 2022/08/22 00:20:56 riastradh Exp $"); #include <sys/vnode.h> #include <sys/kauth.h> #include <sys/mutex.h> +#include <sys/module.h> #include <dev/cons.h> @@ -83,6 +84,8 @@ const struct cdevsw cons_cdevsw = { .d_flag = D_TTY }; +static struct kmutex cn_lock; + struct tty *constty = NULL; /* virtual console output device */ struct consdev *cn_tab; /* physical console device info */ struct vnode *cn_devvp[2]; /* vnode for underlying device. */ @@ -113,8 +116,12 @@ cnopen(dev_t dev, int flag, int mode, struct lwp *l) if (unit > 1) return ENODEV; - if (cn_tab == NULL) - return (0); + mutex_enter(&cn_lock); + + if (cn_tab == NULL) { + error = 0; + goto out; + } /* * always open the 'real' console device, so we don't get nailed @@ -145,15 +152,19 @@ cnopen(dev_t dev, int flag, int mode, struct lwp *l) */ panic("cnopen: cn_tab->cn_dev == dev"); } - if (cn_devvp[unit] != NULLVP) - return 0; + if (cn_devvp[unit] != NULLVP) { + error = 0; + goto out; + } if ((error = cdevvp(cndev, &cn_devvp[unit])) != 0) { printf("cnopen: unable to get vnode reference\n"); - return error; + goto out; } vn_lock(cn_devvp[unit], LK_EXCLUSIVE | LK_RETRY); error = VOP_OPEN(cn_devvp[unit], flag, kauth_cred_get()); VOP_UNLOCK(cn_devvp[unit]); + +out: mutex_exit(&cn_lock); return error; } @@ -165,8 +176,12 @@ cnclose(dev_t dev, int flag, int mode, struct lwp *l) unit = minor(dev); - if (cn_tab == NULL) - return (0); + mutex_enter(&cn_lock); + + if (cn_tab == NULL) { + error = 0; + goto out; + } vp = cn_devvp[unit]; cn_devvp[unit] = NULL; @@ -174,6 +189,8 @@ cnclose(dev_t dev, int flag, int mode, struct lwp *l) error = VOP_CLOSE(vp, flag, kauth_cred_get()); VOP_UNLOCK(vp); vrele(vp); + +out: mutex_exit(&cn_lock); return error; } @@ -433,3 +450,21 @@ cn_redirect(dev_t *devp, int is_read, int *error) *devp = dev; return true; } + +MODULE(MODULE_CLASS_DRIVER, cons, NULL); + +static int +cons_modcmd(modcmd_t cmd, void *arg) +{ + + switch (cmd) { + case MODULE_CMD_INIT: + mutex_init(&cn_lock, MUTEX_DEFAULT, IPL_NONE); + return 0; + case MODULE_CMD_FINI: + mutex_destroy(&cn_lock); + return 0; + default: + return ENOTTY; + } +} >From 2360273d41e06cc8b0676824edc9643b17b69621 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell <riastr...@netbsd.org> Date: Sat, 1 Oct 2022 14:06:14 +0000 Subject: [PATCH 3/5] cons(9): Check the unit number on close too. Races between multiple opens, some of which fail, might lead to closing a bad unit number -- not clear there's a good way to prevent this. --- sys/dev/cons.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sys/dev/cons.c b/sys/dev/cons.c index 1bb05a90fae1..483d4d5b1c32 100644 --- a/sys/dev/cons.c +++ b/sys/dev/cons.c @@ -175,6 +175,8 @@ cnclose(dev_t dev, int flag, int mode, struct lwp *l) int unit, error; unit = minor(dev); + if (unit > 1) + return ENODEV; mutex_enter(&cn_lock); >From 50494b30028537731f7e04d5a32883d93884ae1d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell <riastr...@netbsd.org> Date: Sat, 1 Oct 2022 19:54:41 +0000 Subject: [PATCH 4/5] constty(4): Make MP-safe. Access to the global constty variable is coordinated as follows: 1. Changing constty from null to nonnull is allowed only under the new adaptive constty_lock in thread context. This serializes TIOCCONS operations. 2. Changing constty from nonnull to null 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 under constty_lock. 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. --- sys/dev/cons.c | 99 ++++++++++++++++++++++++++++----------- sys/kern/subr_prf.c | 26 ++++++++--- sys/kern/tty.c | 111 +++++++++++++++++++++++++++++++++++++------- sys/sys/tty.h | 5 ++ 4 files changed, 190 insertions(+), 51 deletions(-) diff --git a/sys/dev/cons.c b/sys/dev/cons.c index 483d4d5b1c32..b84d04dd5888 100644 --- a/sys/dev/cons.c +++ b/sys/dev/cons.c @@ -54,6 +54,8 @@ __KERNEL_RCSID(0, "$NetBSD: cons.c,v 1.79 2022/08/22 00:20:56 riastradh Exp $"); #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 flag) * 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 *data, int flag, struct lwp *l) * 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 *l) * 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,46 @@ 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; *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; + ok = false; + goto out; } - dev = constty->t_dev; - } else if (cn_tab == NULL) - return false; - else + tty_acquire(ctp); + *ctpp = ctp; + dev = ctp->t_dev; + } else if (cn_tab == NULL) { + ok = false; + goto out; + } else { dev = cn_tab->cn_dev; + } *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); diff --git a/sys/kern/subr_prf.c b/sys/kern/subr_prf.c index a41d41f5c69b..e87e6efc8501 100644 --- a/sys/kern/subr_prf.c +++ b/sys/kern/subr_prf.c @@ -103,7 +103,6 @@ static void kprintf_internal(const char *, int, void *, 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 diff --git a/sys/kern/tty.c b/sys/kern/tty.c index d02691b10035..2be6a2b6f356 100644 --- a/sys/kern/tty.c +++ b/sys/kern/tty.c @@ -99,6 +99,9 @@ __KERNEL_RCSID(0, "$NetBSD: tty.c,v 1.301 2022/04/07 21:46:51 riastradh Exp $"); #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); + + /* + * 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); - if (constty == tp) - constty = NULL; + 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) { @@ -473,6 +497,44 @@ ttyclose(struct tty *tp) ndflush(q, (q)->c_cc); \ } +/* + * 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' @@ -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,46 @@ ttioctl(struct tty *tp, u_long cmd, void *data, int flag, struct lwp *l) mutex_spin_exit(&tty_lock); break; } - case TIOCCONS: /* become virtual console */ - if (*(int *)data) { - if (constty && constty != tp && - ISSET(constty->t_state, TS_CARR_ON | TS_ISOPEN) == - (TS_CARR_ON | TS_ISOPEN)) - return EBUSY; + case TIOCCONS: { /* become virtual console */ + struct tty *ctp; + mutex_enter(&constty_lock); + error = 0; + ctp = atomic_load_relaxed(&constty); + if (*(int *)data) do { + if (ctp != NULL && ctp != tp && + ISSET(ctp->t_state, TS_CARR_ON | TS_ISOPEN) == + (TS_CARR_ON | TS_ISOPEN)) { + error = EBUSY; + break; + } pb = pathbuf_create("/dev/console"); if (pb == NULL) { - return ENOMEM; + error = ENOMEM; + break; } NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, pb); if ((error = namei(&nd)) != 0) { pathbuf_destroy(pb); - return error; + break; } error = VOP_ACCESS(nd.ni_vp, VREAD, l->l_cred); vput(nd.ni_vp); pathbuf_destroy(pb); if (error) - return error; + break; - constty = tp; - } else if (tp == constty) - constty = NULL; + KASSERT(atomic_load_relaxed(&constty) == tp || + atomic_load_relaxed(&constty) == NULL); + atomic_store_release(&constty, tp); + } while (0); else if (tp == ctp) { + atomic_store_relaxed(&constty, NULL); + } + mutex_exit(&constty_lock); + if (error) + return error; break; + } case TIOCDRAIN: /* wait till output drained */ if ((error = ttywait(tp)) != 0) return (error); @@ -2969,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); diff --git a/sys/sys/tty.h b/sys/sys/tty.h index 17a3a97e5df0..8adc032164f1 100644 --- a/sys/sys/tty.h +++ b/sys/sys/tty.h @@ -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 ttylist is a TAILQ */ #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 *); >From c081e0374b79c6276fabaf868625bc576b20cc1b Mon Sep 17 00:00:00 2001 From: Taylor R Campbell <riastr...@netbsd.org> Date: Sat, 1 Oct 2022 20:03:28 +0000 Subject: [PATCH 5/5] console(4), constty(4): Rip off the kernel lock. --- sys/dev/cons.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/dev/cons.c b/sys/dev/cons.c index b84d04dd5888..f4f9a1602221 100644 --- a/sys/dev/cons.c +++ b/sys/dev/cons.c @@ -84,7 +84,7 @@ const struct cdevsw cons_cdevsw = { .d_mmap = nommap, .d_kqfilter = cnkqfilter, .d_discard = nodiscard, - .d_flag = D_TTY + .d_flag = D_TTY|D_MPSAFE, }; static struct kmutex cn_lock;