Subject: Re: kern/32563: audioctl of death
To: None <jmcneill@netbsd.org, gnats-admin@netbsd.org,>
From: Jared D. McNeill <jmcneill@invisible.ca>
List: netbsd-bugs
Date: 04/18/2006 13:00:08
The following reply was made to PR kern/32563; it has been noted by GNATS.

From: "Jared D. McNeill" <jmcneill@invisible.ca>
To: Christian Biere <christianbiere@gmx.de>
Cc: gnats-bugs@netbsd.org
Subject: Re: kern/32563: audioctl of death
Date: Tue, 18 Apr 2006 09:55:51 -0300 (ADT)

 On Tue, 18 Apr 2006, Christian Biere wrote:
 > Sure, this way please.
 
 The following patch isn't perfect (we can't rely on having a giant lock 
 for all of MI audio), but it should at least show the concept.
 
 I still need to figure out where to keep this simple_lock on a 
 per-instance basis; the audio filters don't have any knowledge of MI audio 
 or the hw driver, but they need to be locked to prevent changes to the 
 filter list on the fly.
 
 I'll have another look at this tonight.
 
 Index: audio.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/audio.c,v
 retrieving revision 1.202
 diff -u -r1.202 audio.c
 --- audio.c	28 Mar 2006 17:38:29 -0000	1.202
 +++ audio.c	18 Apr 2006 10:34:17 -0000
 @@ -81,6 +81,7 @@
   #include <sys/conf.h>
   #include <sys/audioio.h>
   #include <sys/device.h>
 +#include <sys/lock.h>
 
   #include <dev/audio_if.h>
   #include <dev/audiovar.h>
 @@ -103,6 +104,9 @@
 
   int	audio_blk_ms = AUDIO_BLK_MS;
 
 +/* XXX */
 +struct simplelock audio_filtlock = SIMPLELOCK_INITIALIZER;
 +
   int	audiosetinfo(struct audio_softc *, struct audio_info *);
   int	audiogetinfo(struct audio_softc *, struct audio_info *);
 
 @@ -157,9 +161,11 @@
   static void audio_stream_dtor(audio_stream_t *);
   static int audio_stream_ctor(audio_stream_t *, const audio_params_t *, int);
   static void stream_filter_list_append
 -	(stream_filter_list_t *, stream_filter_factory_t, const audio_params_t *);
 +	(stream_filter_list_t *, stream_filter_factory_t,
 +	 const audio_params_t *);
   static void stream_filter_list_prepend
 -	(stream_filter_list_t *, stream_filter_factory_t, const audio_params_t *);
 +	(stream_filter_list_t *, stream_filter_factory_t,
 +	 const audio_params_t *);
   static void stream_filter_list_set
   	(stream_filter_list_t *, int, stream_filter_factory_t,
   	 const audio_params_t *);
 @@ -658,6 +664,8 @@
   	audio_params_t *to_param;
   	int i, n;
 
 +	simple_lock(&audio_filtlock);
 +
   	memset(pf, 0, sizeof(pf));
   	memset(ps, 0, sizeof(ps));
   	from_param = pp;
 @@ -681,6 +689,7 @@
   				pf[i]->dtor(pf[i]);
   			audio_stream_dtor(&ps[i]);
   		}
 +		simple_unlock(&audio_filtlock);
   		return EINVAL;
   	}
 
 @@ -710,6 +719,8 @@
   	}
   	audio_print_params("[HW]", &sc->sc_pr.s.param);
   #endif /* AUDIO_DEBUG */
 +
 +	simple_unlock(&audio_filtlock);
   	return 0;
   }
 
 @@ -788,12 +799,16 @@
   {
   	int i;
 
 +	simple_lock(&audio_filtlock);
 +
   	for (i = 0; i < sc->sc_npfilters; i++) {
   		sc->sc_pfilters[i]->dtor(sc->sc_pfilters[i]);
   		sc->sc_pfilters[i] = NULL;
   		audio_stream_dtor(&sc->sc_pstreams[i]);
   	}
   	sc->sc_npfilters = 0;
 +
 +	simple_unlock(&audio_filtlock);
   }
 
   static void
 @@ -845,6 +860,8 @@
   			  const audio_params_t *param)
   {
 
 +	simple_lock(&audio_filtlock);
 +
   	if (list->req_size >= AUDIO_MAX_FILTERS) {
   		printf("%s: increase AUDIO_MAX_FILTERS in sys/dev/audio_if.h\n",
   		       __func__);
 @@ -853,6 +870,8 @@
   	list->filters[list->req_size].factory = factory;
   	list->filters[list->req_size].param = *param;
   	list->req_size++;
 +
 +	simple_unlock(&audio_filtlock);
   }
 
   static void
 @@ -860,14 +879,20 @@
   		       stream_filter_factory_t factory,
   		       const audio_params_t *param)
   {
 +
 +	simple_lock(&audio_filtlock);
 +
   	if (i < 0 || i >= AUDIO_MAX_FILTERS) {
   		printf("%s: invalid index: %d\n", __func__, i);
   		return;
   	}
 +
   	list->filters[i].factory = factory;
   	list->filters[i].param = *param;
   	if (list->req_size <= i)
   		list->req_size = i + 1;
 +
 +	simple_unlock(&audio_filtlock);
   }
 
   static void
 @@ -876,6 +901,8 @@
   			   const audio_params_t *param)
   {
 
 +	simple_lock(&audio_filtlock);
 +
   	if (list->req_size >= AUDIO_MAX_FILTERS) {
   		printf("%s: increase AUDIO_MAX_FILTERS in sys/dev/audio_if.h\n",
   		       __func__);
 @@ -886,6 +913,8 @@
   	list->filters[0].factory = factory;
   	list->filters[0].param = *param;
   	list->req_size++;
 +
 +	simple_unlock(&audio_filtlock);
   }
 
   int
 @@ -1897,12 +1926,21 @@
   		 * work with a temporary audio_stream_t to narrow
   		 * splaudio() enclosure
   		 */
 +
 +		simple_lock(&audio_filtlock);
   		if (sc->sc_npfilters > 0) {
   			filter = sc->sc_pfilters[0];
   			filter->set_fetcher(filter, &ufetcher.base);
   		}
   		cc = stream.end - stream.start;
 +		if (sc->sc_npfilters > 0) {
 +			fetcher = &sc->sc_pfilters[sc->sc_npfilters - 1]->base;
 +		} else {
 +			fetcher = &ufetcher.base;
 +		}
   		error = fetcher->fetch_to(fetcher, &stream, cc);
 +		simple_unlock(&audio_filtlock);
 +
   		s = splaudio();
   		if (sc->sc_npfilters > 0) {
   			cb->fstamp += audio_stream_get_used(sc->sc_pustream)
 @@ -1947,6 +1985,7 @@
   			audio_fill_silence(&cb->s.param, einp, cc);
   		}
   	}
 +
   	return error;
   }