Module Name: src Committed By: isaki Date: Sun Feb 23 07:17:01 UTC 2020
Modified Files: src/sys/dev/audio: audio.c audiodef.h audiovar.h Log Message: Prevent a race between audiodetach and fileops methods using psref(9). Fix PR kern/54427. Thank you so much riastradh@ To generate a diff of this commit: cvs rdiff -u -r1.55 -r1.56 src/sys/dev/audio/audio.c cvs rdiff -u -r1.9 -r1.10 src/sys/dev/audio/audiodef.h cvs rdiff -u -r1.7 -r1.8 src/sys/dev/audio/audiovar.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/audio/audio.c diff -u src/sys/dev/audio/audio.c:1.55 src/sys/dev/audio/audio.c:1.56 --- src/sys/dev/audio/audio.c:1.55 Sun Feb 23 04:24:56 2020 +++ src/sys/dev/audio/audio.c Sun Feb 23 07:17:01 2020 @@ -1,4 +1,4 @@ -/* $NetBSD: audio.c,v 1.55 2020/02/23 04:24:56 isaki Exp $ */ +/* $NetBSD: audio.c,v 1.56 2020/02/23 07:17:01 isaki Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -142,7 +142,7 @@ */ #include <sys/cdefs.h> -__KERNEL_RCSID(0, "$NetBSD: audio.c,v 1.55 2020/02/23 04:24:56 isaki Exp $"); +__KERNEL_RCSID(0, "$NetBSD: audio.c,v 1.56 2020/02/23 07:17:01 isaki Exp $"); #ifdef _KERNEL_OPT #include "audio.h" @@ -499,6 +499,8 @@ static void audio_softintr_wr(void *); static int audio_enter_exclusive(struct audio_softc *); static void audio_exit_exclusive(struct audio_softc *); +static struct audio_softc *audio_file_enter(audio_file_t *, struct psref *); +static void audio_file_exit(struct audio_softc *, struct psref *); static int audio_track_waitio(struct audio_softc *, audio_track_t *); static int audioclose(struct file *); @@ -519,6 +521,7 @@ static int filt_audioread_event(struct static int audio_open(dev_t, struct audio_softc *, int, int, struct lwp *, audio_file_t **); static int audio_close(struct audio_softc *, audio_file_t *); +static int audio_unlink(struct audio_softc *, audio_file_t *); static int audio_read(struct audio_softc *, struct uio *, int, audio_file_t *); static int audio_write(struct audio_softc *, struct uio *, int, audio_file_t *); static void audio_file_clear(struct audio_softc *, audio_file_t *); @@ -530,7 +533,6 @@ static int audio_mmap(struct audio_softc struct uvm_object **, int *, audio_file_t *); static int audioctl_open(dev_t, struct audio_softc *, int, int, struct lwp *); -static int audioctl_close(struct audio_softc *, audio_file_t *); static void audio_pintr(void *); static void audio_rintr(void *); @@ -810,6 +812,8 @@ static const struct portname otable[] = { 0, 0 } }; +static struct psref_class *audio_psref_class __read_mostly; + CFATTACH_DECL3_NEW(audio, sizeof(struct audio_softc), audiomatch, audioattach, audiodetach, audioactivate, audiorescan, audiochilddet, DVF_DETACH_SHUTDOWN); @@ -998,6 +1002,9 @@ audioattach(device_t parent, device_t se goto bad; } + sc->sc_psz = pserialize_create(); + psref_target_init(&sc->sc_psref, audio_psref_class); + selinit(&sc->sc_wsel); selinit(&sc->sc_rsel); @@ -1255,7 +1262,7 @@ static int audiodetach(device_t self, int flags) { struct audio_softc *sc; - int maj, mn; + struct audio_file *file; int error; sc = device_private(self); @@ -1270,6 +1277,9 @@ audiodetach(device_t self, int flags) if (error) return error; + /* delete sysctl nodes */ + sysctl_teardown(&sc->sc_log); + mutex_enter(sc->sc_lock); sc->sc_dying = true; cv_broadcast(&sc->sc_exlockcv); @@ -1277,23 +1287,36 @@ audiodetach(device_t self, int flags) cv_broadcast(&sc->sc_pmixer->outcv); if (sc->sc_rmixer) cv_broadcast(&sc->sc_rmixer->outcv); - mutex_exit(sc->sc_lock); - /* delete sysctl nodes */ - sysctl_teardown(&sc->sc_log); + /* Prevent new users */ + SLIST_FOREACH(file, &sc->sc_files, entry) { + atomic_store_relaxed(&file->dying, true); + } + + /* + * Wait for existing users to drain. + * - pserialize_perform waits for all pserialize_read sections on + * all CPUs; after this, no more new psref_acquire can happen. + * - psref_target_destroy waits for all extant acquired psrefs to + * be psref_released. + */ + pserialize_perform(sc->sc_psz); + mutex_exit(sc->sc_lock); + psref_target_destroy(&sc->sc_psref, audio_psref_class); - /* locate the major number */ - maj = cdevsw_lookup_major(&audio_cdevsw); + /* + * We are now guaranteed that there are no calls to audio fileops + * that hold sc, and any new calls with files that were for sc will + * fail. Thus, we now have exclusive access to the softc. + */ /* - * Nuke the vnodes for any open instances (calls close). - * Will wait until any activity on the device nodes has ceased. + * Nuke all open instances. + * Here, we no longer need any locks to traverse sc_files. */ - mn = device_unit(self); - vdevgone(maj, mn | SOUND_DEVICE, mn | SOUND_DEVICE, VCHR); - vdevgone(maj, mn | AUDIO_DEVICE, mn | AUDIO_DEVICE, VCHR); - vdevgone(maj, mn | AUDIOCTL_DEVICE, mn | AUDIOCTL_DEVICE, VCHR); - vdevgone(maj, mn | MIXER_DEVICE, mn | MIXER_DEVICE, VCHR); + while ((file = SLIST_FIRST(&sc->sc_files)) != NULL) { + audio_unlink(sc, file); + } pmf_event_deregister(self, PMFE_AUDIO_VOLUME_DOWN, audio_volume_down, true); @@ -1440,6 +1463,48 @@ audio_exit_exclusive(struct audio_softc } /* + * Acquire sc from file, and increment the psref count. + * If successful, returns sc. Otherwise returns NULL. + */ +struct audio_softc * +audio_file_enter(audio_file_t *file, struct psref *refp) +{ + int s; + bool dying; + + /* psref(9) forbids to migrate CPUs */ + curlwp_bind(); + + /* Block audiodetach while we acquire a reference */ + s = pserialize_read_enter(); + + /* If close or audiodetach already ran, tough -- no more audio */ + dying = atomic_load_relaxed(&file->dying); + if (dying) { + pserialize_read_exit(s); + return NULL; + } + + /* Acquire a reference */ + psref_acquire(refp, &file->sc->sc_psref, audio_psref_class); + + /* Now sc won't go away until we drop the reference count */ + pserialize_read_exit(s); + + return file->sc; +} + +/* + * Decrement the psref count. + */ +void +audio_file_exit(struct audio_softc *sc, struct psref *refp) +{ + + psref_release(refp, &sc->sc_psref, audio_psref_class); +} + +/* * Wait for I/O to complete, releasing sc_lock. * Must be called with sc_lock held. */ @@ -1540,34 +1605,51 @@ static int audioclose(struct file *fp) { struct audio_softc *sc; + struct psref sc_ref; audio_file_t *file; int error; dev_t dev; KASSERT(fp->f_audioctx); file = fp->f_audioctx; - sc = file->sc; dev = file->dev; + error = 0; - /* audio_{enter,exit}_exclusive() is called by lower audio_close() */ + /* + * audioclose() must + * - unplug track from the trackmixer (and unplug anything from softc), + * if sc exists. + * - free all memory objects, regardless of sc. + */ - device_active(sc->sc_dev, DVA_SYSTEM); - switch (AUDIODEV(dev)) { - case SOUND_DEVICE: - case AUDIO_DEVICE: - error = audio_close(sc, file); - break; - case AUDIOCTL_DEVICE: - error = audioctl_close(sc, file); - break; - case MIXER_DEVICE: - error = mixer_close(sc, file); - break; - default: - error = ENXIO; - break; + sc = audio_file_enter(file, &sc_ref); + if (sc) { + switch (AUDIODEV(dev)) { + case SOUND_DEVICE: + case AUDIO_DEVICE: + error = audio_close(sc, file); + break; + case AUDIOCTL_DEVICE: + error = 0; + break; + case MIXER_DEVICE: + error = mixer_close(sc, file); + break; + default: + error = ENXIO; + break; + } + + audio_file_exit(sc, &sc_ref); } - /* f_audioctx has already been freed in lower *_close() */ + + /* Free memory objects anyway */ + TRACEF(2, file, "free memory"); + if (file->ptrack) + audio_track_destroy(file->ptrack); + if (file->rtrack) + audio_track_destroy(file->rtrack); + kmem_free(file, sizeof(*file)); fp->f_audioctx = NULL; return error; @@ -1578,15 +1660,19 @@ audioread(struct file *fp, off_t *offp, int ioflag) { struct audio_softc *sc; + struct psref sc_ref; audio_file_t *file; int error; dev_t dev; KASSERT(fp->f_audioctx); file = fp->f_audioctx; - sc = file->sc; dev = file->dev; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; + if (fp->f_flag & O_NONBLOCK) ioflag |= IO_NDELAY; @@ -1604,6 +1690,7 @@ audioread(struct file *fp, off_t *offp, break; } + audio_file_exit(sc, &sc_ref); return error; } @@ -1612,15 +1699,19 @@ audiowrite(struct file *fp, off_t *offp, int ioflag) { struct audio_softc *sc; + struct psref sc_ref; audio_file_t *file; int error; dev_t dev; KASSERT(fp->f_audioctx); file = fp->f_audioctx; - sc = file->sc; dev = file->dev; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; + if (fp->f_flag & O_NONBLOCK) ioflag |= IO_NDELAY; @@ -1638,6 +1729,7 @@ audiowrite(struct file *fp, off_t *offp, break; } + audio_file_exit(sc, &sc_ref); return error; } @@ -1645,6 +1737,7 @@ static int audioioctl(struct file *fp, u_long cmd, void *addr) { struct audio_softc *sc; + struct psref sc_ref; audio_file_t *file; struct lwp *l = curlwp; int error; @@ -1652,9 +1745,12 @@ audioioctl(struct file *fp, u_long cmd, KASSERT(fp->f_audioctx); file = fp->f_audioctx; - sc = file->sc; dev = file->dev; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; + switch (AUDIODEV(dev)) { case SOUND_DEVICE: case AUDIO_DEVICE: @@ -1676,23 +1772,32 @@ audioioctl(struct file *fp, u_long cmd, break; } + audio_file_exit(sc, &sc_ref); return error; } static int audiostat(struct file *fp, struct stat *st) { + struct audio_softc *sc; + struct psref sc_ref; audio_file_t *file; KASSERT(fp->f_audioctx); file = fp->f_audioctx; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; + memset(st, 0, sizeof(*st)); st->st_dev = file->dev; st->st_uid = kauth_cred_geteuid(fp->f_cred); st->st_gid = kauth_cred_getegid(fp->f_cred); st->st_mode = S_IFCHR; + + audio_file_exit(sc, &sc_ref); return 0; } @@ -1700,6 +1805,7 @@ static int audiopoll(struct file *fp, int events) { struct audio_softc *sc; + struct psref sc_ref; audio_file_t *file; struct lwp *l = curlwp; int revents; @@ -1707,9 +1813,12 @@ audiopoll(struct file *fp, int events) KASSERT(fp->f_audioctx); file = fp->f_audioctx; - sc = file->sc; dev = file->dev; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; + switch (AUDIODEV(dev)) { case SOUND_DEVICE: case AUDIO_DEVICE: @@ -1724,6 +1833,7 @@ audiopoll(struct file *fp, int events) break; } + audio_file_exit(sc, &sc_ref); return revents; } @@ -1731,15 +1841,19 @@ static int audiokqfilter(struct file *fp, struct knote *kn) { struct audio_softc *sc; + struct psref sc_ref; audio_file_t *file; dev_t dev; int error; KASSERT(fp->f_audioctx); file = fp->f_audioctx; - sc = file->sc; dev = file->dev; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; + switch (AUDIODEV(dev)) { case SOUND_DEVICE: case AUDIO_DEVICE: @@ -1754,6 +1868,7 @@ audiokqfilter(struct file *fp, struct kn break; } + audio_file_exit(sc, &sc_ref); return error; } @@ -1762,15 +1877,19 @@ audiommap(struct file *fp, off_t *offp, int *advicep, struct uvm_object **uobjp, int *maxprotp) { struct audio_softc *sc; + struct psref sc_ref; audio_file_t *file; dev_t dev; int error; KASSERT(fp->f_audioctx); file = fp->f_audioctx; - sc = file->sc; dev = file->dev; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; + mutex_enter(sc->sc_lock); device_active(sc->sc_dev, DVA_SYSTEM); /* XXXJDM */ mutex_exit(sc->sc_lock); @@ -1788,6 +1907,7 @@ audiommap(struct file *fp, off_t *offp, break; } + audio_file_exit(sc, &sc_ref); return error; } @@ -1826,13 +1946,16 @@ int audiobellclose(audio_file_t *file) { struct audio_softc *sc; + struct psref sc_ref; int error; - sc = file->sc; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; - device_active(sc->sc_dev, DVA_SYSTEM); error = audio_close(sc, file); + audio_file_exit(sc, &sc_ref); return error; } @@ -1841,20 +1964,25 @@ int audiobellsetrate(audio_file_t *file, u_int sample_rate) { struct audio_softc *sc; + struct psref sc_ref; struct audio_info ai; int error; - sc = file->sc; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; AUDIO_INITINFO(&ai); ai.play.sample_rate = sample_rate; error = audio_enter_exclusive(sc); if (error) - return error; + goto done; error = audio_file_setinfo(sc, file, &ai); audio_exit_exclusive(sc); +done: + audio_file_exit(sc, &sc_ref); return error; } @@ -1863,10 +1991,16 @@ int audiobellwrite(audio_file_t *file, struct uio *uio) { struct audio_softc *sc; + struct psref sc_ref; int error; - sc = file->sc; + sc = audio_file_enter(file, &sc_ref); + if (sc == NULL) + return EIO; + error = audio_write(sc, uio, 0, file); + + audio_file_exit(sc, &sc_ref); return error; } @@ -2131,7 +2265,30 @@ bad1: int audio_close(struct audio_softc *sc, audio_file_t *file) { - audio_track_t *oldtrack; + + /* Protect entering new fileops to this file */ + atomic_store_relaxed(&file->dying, true); + + /* + * Drain first. + * It must be done before unlinking(acquiring exclusive lock). + */ + if (file->ptrack) { + mutex_enter(sc->sc_lock); + audio_track_drain(sc, file->ptrack); + mutex_exit(sc->sc_lock); + } + + return audio_unlink(sc, file); +} + +/* + * Unlink this file, but not freeing memory here. + * Must be called without sc_lock nor sc_exlock held. + */ +int +audio_unlink(struct audio_softc *sc, audio_file_t *file) +{ int error; TRACEF(1, file, "%spid=%d.%d po=%d ro=%d", @@ -2142,46 +2299,50 @@ audio_close(struct audio_softc *sc, audi "sc->sc_popens=%d, sc->sc_ropens=%d", sc->sc_popens, sc->sc_ropens); + mutex_enter(sc->sc_lock); /* - * Drain first. - * It must be done before acquiring exclusive lock. + * Acquire exclusive lock to protect counters. + * Does not use audio_enter_exclusive() due to sc_dying. */ - if (file->ptrack) { - mutex_enter(sc->sc_lock); - audio_track_drain(sc, file->ptrack); - mutex_exit(sc->sc_lock); + while (__predict_false(sc->sc_exlock != 0)) { + error = cv_timedwait_sig(&sc->sc_exlockcv, sc->sc_lock, + mstohz(AUDIO_TIMEOUT)); + /* XXX what should I do on error? */ + if (error == EWOULDBLOCK) { + mutex_exit(sc->sc_lock); + device_printf(sc->sc_dev, + "%s: cv_timedwait_sig failed %d", __func__, error); + return error; + } } + sc->sc_exlock = 1; - /* Then, acquire exclusive lock to protect counters. */ - /* XXX what should I do when an error occurs? */ - error = audio_enter_exclusive(sc); - if (error) - return error; + device_active(sc->sc_dev, DVA_SYSTEM); + + mutex_enter(sc->sc_intr_lock); + SLIST_REMOVE(&sc->sc_files, file, audio_file, entry); + mutex_exit(sc->sc_intr_lock); if (file->ptrack) { + TRACET(3, file->ptrack, "dropframes=%" PRIu64, + file->ptrack->dropframes); + + KASSERT(sc->sc_popens > 0); + sc->sc_popens--; + /* Call hw halt_output if this is the last playback track. */ - if (sc->sc_popens == 1 && sc->sc_pbusy) { + if (sc->sc_popens == 0 && sc->sc_pbusy) { error = audio_pmixer_halt(sc); if (error) { device_printf(sc->sc_dev, - "halt_output failed with %d\n", error); + "halt_output failed with %d (ignored)\n", + error); } } - /* Destroy the track. */ - oldtrack = file->ptrack; - mutex_enter(sc->sc_intr_lock); - file->ptrack = NULL; - mutex_exit(sc->sc_intr_lock); - TRACET(3, oldtrack, "dropframes=%" PRIu64, - oldtrack->dropframes); - audio_track_destroy(oldtrack); - - KASSERT(sc->sc_popens > 0); - sc->sc_popens--; - /* Restore mixing volume if all tracks are gone. */ if (sc->sc_popens == 0) { + /* intr_lock is not necessary, but just manners. */ mutex_enter(sc->sc_intr_lock); sc->sc_pmixer->volume = 256; sc->sc_pmixer->voltimer = 0; @@ -2189,26 +2350,22 @@ audio_close(struct audio_softc *sc, audi } } if (file->rtrack) { + TRACET(3, file->rtrack, "dropframes=%" PRIu64, + file->rtrack->dropframes); + + KASSERT(sc->sc_ropens > 0); + sc->sc_ropens--; + /* Call hw halt_input if this is the last recording track. */ - if (sc->sc_ropens == 1 && sc->sc_rbusy) { + if (sc->sc_ropens == 0 && sc->sc_rbusy) { error = audio_rmixer_halt(sc); if (error) { device_printf(sc->sc_dev, - "halt_input failed with %d\n", error); + "halt_input failed with %d (ignored)\n", + error); } } - /* Destroy the track. */ - oldtrack = file->rtrack; - mutex_enter(sc->sc_intr_lock); - file->rtrack = NULL; - mutex_exit(sc->sc_intr_lock); - TRACET(3, oldtrack, "dropframes=%" PRIu64, - oldtrack->dropframes); - audio_track_destroy(oldtrack); - - KASSERT(sc->sc_ropens > 0); - sc->sc_ropens--; } /* Call hw close if this is the last track. */ @@ -2223,14 +2380,9 @@ audio_close(struct audio_softc *sc, audi kauth_cred_free(sc->sc_cred); } - mutex_enter(sc->sc_intr_lock); - SLIST_REMOVE(&sc->sc_files, file, audio_file, entry); - mutex_exit(sc->sc_intr_lock); - TRACE(3, "done"); audio_exit_exclusive(sc); - kmem_free(file, sizeof(*file)); return 0; } @@ -3092,14 +3244,6 @@ audioctl_open(dev_t dev, struct audio_so return error; } -static int -audioctl_close(struct audio_softc *sc, audio_file_t *file) -{ - - kmem_free(file, sizeof(*file)); - return 0; -} - /* * Free 'mem' if available, and initialize the pointer. * For this reason, this is implemented as macro. @@ -7693,7 +7837,6 @@ mixer_close(struct audio_softc *sc, audi mixer_async_remove(sc, curproc->p_pid); mutex_exit(sc->sc_lock); - kmem_free(file, sizeof(*file)); return 0; } @@ -8445,9 +8588,11 @@ audio_modcmd(modcmd_t cmd, void *arg) { int error = 0; -#ifdef _MODULE switch (cmd) { case MODULE_CMD_INIT: + /* XXX interrupt level? */ + audio_psref_class = psref_class_create("audio", IPL_SOFTSERIAL); +#ifdef _MODULE error = devsw_attach(audio_cd.cd_name, NULL, &audio_bmajor, &audio_cdevsw, &audio_cmajor); if (error) @@ -8458,20 +8603,23 @@ audio_modcmd(modcmd_t cmd, void *arg) if (error) { devsw_detach(NULL, &audio_cdevsw); } +#endif break; case MODULE_CMD_FINI: +#ifdef _MODULE devsw_detach(NULL, &audio_cdevsw); error = config_fini_component(cfdriver_ioconf_audio, cfattach_ioconf_audio, cfdata_ioconf_audio); if (error) devsw_attach(audio_cd.cd_name, NULL, &audio_bmajor, &audio_cdevsw, &audio_cmajor); +#endif + psref_class_destroy(audio_psref_class); break; default: error = ENOTTY; break; } -#endif return error; } Index: src/sys/dev/audio/audiodef.h diff -u src/sys/dev/audio/audiodef.h:1.9 src/sys/dev/audio/audiodef.h:1.10 --- src/sys/dev/audio/audiodef.h:1.9 Sat Feb 22 06:58:39 2020 +++ src/sys/dev/audio/audiodef.h Sun Feb 23 07:17:01 2020 @@ -1,4 +1,4 @@ -/* $NetBSD: audiodef.h,v 1.9 2020/02/22 06:58:39 isaki Exp $ */ +/* $NetBSD: audiodef.h,v 1.10 2020/02/23 07:17:01 isaki Exp $ */ /* * Copyright (C) 2017 Tetsuya Isaki. All rights reserved. @@ -202,6 +202,9 @@ struct audio_file { /* process who wants audio SIGIO. */ pid_t async_audio; + /* true when closing */ + bool dying; + SLIST_ENTRY(audio_file) entry; }; Index: src/sys/dev/audio/audiovar.h diff -u src/sys/dev/audio/audiovar.h:1.7 src/sys/dev/audio/audiovar.h:1.8 --- src/sys/dev/audio/audiovar.h:1.7 Sat Jan 11 04:53:10 2020 +++ src/sys/dev/audio/audiovar.h Sun Feb 23 07:17:01 2020 @@ -1,4 +1,4 @@ -/* $NetBSD: audiovar.h,v 1.7 2020/01/11 04:53:10 isaki Exp $ */ +/* $NetBSD: audiovar.h,v 1.8 2020/02/23 07:17:01 isaki Exp $ */ /*- * Copyright (c) 2002 The NetBSD Foundation, Inc. @@ -69,6 +69,8 @@ #include <sys/condvar.h> #include <sys/proc.h> +#include <sys/pserialize.h> +#include <sys/psref.h> #include <sys/queue.h> #include <dev/audio/audio_if.h> @@ -218,6 +220,13 @@ struct audio_softc { kcondvar_t sc_exlockcv; /* + * Passive reference to prevent a race between detach and fileops. + * pserialize_perform(sc_psz) must be protected by sc_lock. + */ + pserialize_t sc_psz; + struct psref_target sc_psref; + + /* * Must be protected by sc_lock (?) */ bool sc_dying;