[Added Daniel and Clemens in the loop]
At Thu, 11 Oct 2012 17:17:59 +0200,
Matthieu CASTET wrote:
>
> Hi,
>
> while doing some monkey tests on a product we found races in usb audio code when
> the device in unplugged from usb (on linus master tree).
>
> This can be reproduced with usb_audio_show_race.diff and CONFIG_DEBUG_SLAB.
> With this patch, start a stream :
> # arecord -D hw:0 -r 44100 -c 2 -f S16_LE > /dev/null
> you will see the kernel log : "in snd_usb_hw_params sleeping"
> Unplug the device before "in snd_usb_hw_params sleeping exit", and you will see
> an oops in snd_pcm_hw_params
>
>
> Instead of using CONFIG_DEBUG_SLAB, usb_audio_show_use_after_free.diff can show
> use after free by setting usb_device pointer to NULL in
> snd_usb_audio_disconnect. [1]
>
>
> In order to protect from all the races, before using any usb_device we need to
> check if it is not freed.
>
> What's the best way to do that ?
>
> A trival fix would be to take a mutex in all snd_pcm_ops callback that access usb :
>
> {
> mutex_lock(&chip->shutdown_mutex);
> if (!chip->dev) {
> mutex_unlock(&chip->shutdown_mutex);
> return -ENODEV;
> }
> [...]
> mutex_unlock(&chip->shutdown_mutex);
> }
>
>
> But that will serialize all snd_pcm_ops callbacks.
We had already some protections but they don't cover the recent code
rewrites at all, so this bad result for now.
Yes, the easiest and maybe sane way would be to put the mutex in each
ops except for trigger. open, close, hw_params, hw_free and prepare
are ops that don't need much parallelism, so this can be protected
well. Or, we may introduce a new mutex per stream if we really want
parallel operations, but I don't think it's worth.
The trigger callback is an atomic ops, so it won't need the check.
Maybe the spinlock is needed to avoid the race in
snd_usb_stream_disconnect().
> Another solution could be to use refcounting or rwlock patern :
> - snd_pcm_ops callbacks call rdlock_trylock/rdlock_unlock
> - usb_driver disconnect callback take rwlock_lock and never release it
I don't think this is needed.
So... the below is a quick hack I did without testing at all.
Hopefully this can give some advance.
Takashi
---
diff --git a/sound/usb/card.c b/sound/usb/card.c
index 561bb74..115484e 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -131,9 +131,13 @@ static void snd_usb_stream_disconnect(struct list_head *head)
subs = &as->substream[idx];
if (!subs->num_formats)
continue;
+ if (subs->pcm_substream)
+ snd_pcm_stream_lock_irq(subs->pcm_substream);
subs->interface = -1;
subs->data_endpoint = NULL;
subs->sync_endpoint = NULL;
+ if (subs->pcm_substream)
+ snd_pcm_stream_unlock_irq(subs->pcm_substream);
}
}
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 55e19e1..01e82ac 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -444,7 +444,6 @@ static int configure_endpoint(struct snd_usb_substream *subs)
{
int ret;
- mutex_lock(&subs->stream->chip->shutdown_mutex);
/* format changed */
stop_endpoints(subs, 0, 0, 0);
ret = snd_usb_endpoint_set_params(subs->data_endpoint,
@@ -455,7 +454,7 @@ static int configure_endpoint(struct snd_usb_substream *subs)
subs->cur_audiofmt,
subs->sync_endpoint);
if (ret < 0)
- goto unlock;
+ return ret;
if (subs->sync_endpoint)
ret = snd_usb_endpoint_set_params(subs->data_endpoint,
@@ -466,8 +465,6 @@ static int configure_endpoint(struct snd_usb_substream *subs)
subs->cur_audiofmt,
NULL);
-unlock:
- mutex_unlock(&subs->stream->chip->shutdown_mutex);
return ret;
}
@@ -488,10 +485,15 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
struct audioformat *fmt;
int ret;
+ mutex_lock(&subs->stream->chip->shutdown_mutex);
+ if (subs->stream->chip->shutdown) {
+ ret = -ENODEV;
+ goto unlock;
+ }
ret = snd_pcm_lib_alloc_vmalloc_buffer(substream,
params_buffer_bytes(hw_params));
if (ret < 0)
- return ret;
+ goto unlock;
subs->pcm_format = params_format(hw_params);
subs->period_bytes = params_period_bytes(hw_params);
@@ -502,17 +504,20 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
if (!fmt) {
snd_printd(KERN_DEBUG "cannot set format: format = %#x, rate = %d, channels = %d\n",
subs->pcm_format, subs->cur_rate, subs->channels);
- return -EINVAL;
+ ret = -EINVAL;
+ goto unlock;
}
if ((ret = set_format(subs, fmt)) < 0)
- return ret;
+ goto unlock;
subs->interface = fmt->iface;
subs->altset_idx = fmt->altset_idx;
subs->need_setup_ep = true;
- return 0;
+ unlock:
+ mutex_unlock(&subs->stream->chip->shutdown_mutex);
+ return ret;
}
/*
@@ -547,17 +552,26 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
struct usb_interface *iface;
int ret;
+ mutex_lock(&subs->stream->chip->shutdown_mutex);
+ if (subs->stream->chip->shutdown) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
if (! subs->cur_audiofmt) {
snd_printk(KERN_ERR "usbaudio: no format is specified!\n");
- return -ENXIO;
+ ret = -ENXIO;
+ goto unlock;
}
- if (snd_BUG_ON(!subs->data_endpoint))
- return -EIO;
+ if (snd_BUG_ON(!subs->data_endpoint)) {
+ ret = -EIO;
+ goto unlock;
+ }
ret = set_format(subs, subs->cur_audiofmt);
if (ret < 0)
- return ret;
+ goto unlock;
iface = usb_ifnum_to_if(subs->dev, subs->cur_audiofmt->iface);
alts = &iface->altsetting[subs->cur_audiofmt->altset_idx];
@@ -567,12 +581,12 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
subs->cur_audiofmt,
subs->cur_rate);
if (ret < 0)
- return ret;
+ goto unlock;
if (subs->need_setup_ep) {
ret = configure_endpoint(subs);
if (ret < 0)
- return ret;
+ goto unlock;
subs->need_setup_ep = false;
}
@@ -592,8 +606,10 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
/* for playback, submit the URBs now; otherwise, the first hwptr_done
* updates for all URBs would happen at the same time when starting */
if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
- return start_endpoints(subs, 1);
+ ret = start_endpoints(subs, 1);
+ unlock:
+ mutex_unlock(&subs->stream->chip->shutdown_mutex);
return 0;
}
@@ -980,14 +996,18 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
struct snd_pcm_runtime *runtime = substream->runtime;
struct snd_usb_substream *subs = &as->substream[direction];
+ int ret;
+ mutex_lock(&as->chip->shutdown_mutex);
subs->interface = -1;
subs->altset_idx = 0;
runtime->hw = snd_usb_hardware;
runtime->private_data = subs;
subs->pcm_substream = substream;
/* runtime PM is also done there */
- return setup_hw_info(runtime, subs);
+ ret = setup_hw_info(runtime, subs);
+ mutex_unlock(&as->chip->shutdown_mutex);
+ return ret;
}
static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
@@ -995,6 +1015,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
struct snd_usb_substream *subs = &as->substream[direction];
+ mutex_lock(&as->chip->shutdown_mutex);
stop_endpoints(subs, 0, 0, 0);
if (!as->chip->shutdown && subs->interface >= 0) {
@@ -1004,6 +1025,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
subs->pcm_substream = NULL;
snd_usb_autosuspend(subs->stream->chip);
+ mutex_unlock(&as->chip->shutdown_mutex);
return 0;
}
@@ -1222,6 +1244,9 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
{
struct snd_usb_substream *subs = substream->runtime->private_data;
+ if (subs->stream->chip->shutdown)
+ return -ENODEV;
+
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
