Re: [alsa-devel] ALSA: usb-audio: fix oops due to cleanup race when disconnecting

At Tue, 22 Feb 2011 10:21:18 +0100, Clemens Ladisch wrote:

From: Takashi Iwai tiwai@suse.de

When a USB audio device is disconnected, snd_usb_audio_disconnect() kills all audio URBs. At the same time, the application, after being notified of the disconnection, might close the device, in which case ALSA calls the .hw_free callback, which should free the URBs too.

Commit de1b8b93a0ba prevented snd_usb_hw_free() from freeing the URBs to avoid a hang that resulted from this race, but this introduced another race because the URB callbacks could now be executed after snd_usb_hw_free() has returned, and try to access already freed data.

Fix the first race by introducing a mutex to serialize the disconnect callback and all PCM callbacks that manage URBs (hw_free and hw_params).

Reported-by: Pierre-Louis Bossart pierre-louis.bossart@intel.com [CL: also serialize hw_params callback] Signed-off-by: Clemens Ladisch clemens@ladisch.de

Ah, I forgot to put my sign-off since I was waiting for any test result... Has anyone tried it?

thanks,

Takashi

diff --git a/sound/usb/card.c b/sound/usb/card.c index 800f7cb..c0f8270 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -323,6 +323,7 @@ static int snd_usb_audio_create(struct usb_device *dev, int idx, return -ENOMEM; }

• mutex_init(&chip->shutdown_mutex); chip->index = idx; chip->dev = dev; chip->card = card;

@@ -531,6 +532,7 @@ static void snd_usb_audio_disconnect(struct usb_device *dev, void *ptr) chip = ptr; card = chip->card; mutex_lock(&register_mutex);

• mutex_lock(&chip->shutdown_mutex); chip->shutdown = 1; chip->num_interfaces--; if (chip->num_interfaces <= 0) {

@@ -548,9 +550,11 @@ static void snd_usb_audio_disconnect(struct usb_device *dev, void *ptr) snd_usb_mixer_disconnect(p); } usb_chip[chip->index] = NULL;

• mutex_unlock(&chip->shutdown_mutex);
  

  mutex_unlock(&register_mutex); snd_card_free_when_closed(card); } else {

• mutex_unlock(&chip->shutdown_mutex);
  

  mutex_unlock(&register_mutex); }

} diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 4132522..e3f6805 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -361,6 +361,7 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, }

if (changed) {

• mutex_lock(&subs->stream->chip->shutdown_mutex);
  

  /* format changed */ snd_usb_release_substream_urbs(subs, 0); /* influenced: period_bytes, channels, rate, format, */

@@ -368,6 +369,7 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, params_rate(hw_params), snd_pcm_format_physical_width(params_format(hw_params)) * params_channels(hw_params));

• mutex_unlock(&subs->stream->chip->shutdown_mutex);
  

  }

  return ret;

@@ -385,8 +387,9 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) subs->cur_audiofmt = NULL; subs->cur_rate = 0; subs->period_bytes = 0;

• if (!subs->stream->chip->shutdown)

• snd_usb_release_substream_urbs(subs, 0);
  

• mutex_lock(&subs->stream->chip->shutdown_mutex);
• snd_usb_release_substream_urbs(subs, 0);
• mutex_unlock(&subs->stream->chip->shutdown_mutex); return snd_pcm_lib_free_vmalloc_buffer(substream);

}

diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h index db3eb21..6e66fff 100644 --- a/sound/usb/usbaudio.h +++ b/sound/usb/usbaudio.h @@ -36,6 +36,7 @@ struct snd_usb_audio { struct snd_card *card; u32 usb_id; int shutdown;

• struct mutex shutdown_mutex; unsigned int txfr_quirk:1; /* Subframe boundaries on transfers */ int num_interfaces; int num_suspended_intf;