Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/dev/audio Prevent a race between audiodetach and fileops...



details:   https://anonhg.NetBSD.org/src/rev/c28793d20ff3
branches:  trunk
changeset: 745122:c28793d20ff3
user:      isaki <isaki%NetBSD.org@localhost>
date:      Sun Feb 23 07:17:01 2020 +0000

description:
Prevent a race between audiodetach and fileops methods using psref(9).
Fix PR kern/54427.
Thank you so much riastradh@

diffstat:

 sys/dev/audio/audio.c    |  364 +++++++++++++++++++++++++++++++++-------------
 sys/dev/audio/audiodef.h |    5 +-
 sys/dev/audio/audiovar.h |   11 +-
 3 files changed, 270 insertions(+), 110 deletions(-)

diffs (truncated from 794 to 300 lines):

diff -r 0d8b1f238534 -r c28793d20ff3 sys/dev/audio/audio.c
--- a/sys/dev/audio/audio.c     Sun Feb 23 06:15:27 2020 +0000
+++ b/sys/dev/audio/audio.c     Sun Feb 23 07:17:01 2020 +0000
@@ -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 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 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 @@
        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 @@
        { 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 @@
                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 @@
 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 @@
        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 @@
                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);
-
-       /* locate the major number */
-       maj = cdevsw_lookup_major(&audio_cdevsw);
+
+       /* Prevent new users */
+       SLIST_FOREACH(file, &sc->sc_files, entry) {
+               atomic_store_relaxed(&file->dying, true);
+       }
 
        /*
-        * Nuke the vnodes for any open instances (calls close).
-        * Will wait until any activity on the device nodes has ceased.
+        * 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.
         */
-       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);
+       pserialize_perform(sc->sc_psz);
+       mutex_exit(sc->sc_lock);
+       psref_target_destroy(&sc->sc_psref, audio_psref_class);
+
+       /*
+        * 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 all open instances.
+        * Here, we no longer need any locks to traverse sc_files.
+        */
+       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 @@
 }
 
 /*
+ * 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 @@
 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;
-
-       /* audio_{enter,exit}_exclusive() is called by lower audio_close() */
-
-       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;
-       }
-       /* f_audioctx has already been freed in lower *_close() */
+       error = 0;
+
+       /*
+        * audioclose() must
+        * - unplug track from the trackmixer (and unplug anything from softc),
+        *   if sc exists.
+        * - free all memory objects, regardless of sc.
+        */
+
+       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);
+       }
+
+       /* 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 @@
        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 @@
                break;
        }
 
+       audio_file_exit(sc, &sc_ref);
        return error;
 }
 
@@ -1612,15 +1699,19 @@
        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;



Home | Main Index | Thread Index | Old Index