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

[Takashi Iwai]

At Tue, 22 Feb 2011 10:21:18 +0100,
Clemens Ladisch wrote:
> 
> From: Takashi Iwai <tiwai at 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 at intel.com>
> [CL: also serialize hw_params callback]
> Signed-off-by: Clemens Ladisch <clemens at 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;
>