Commit 027c7002 authored by Mark Hills's avatar Mark Hills Committed by Takashi Iwai
Browse files

ALSA: echoaudio: Race conditions around "opencount" 



Use of atomics does not make these statements robust:

       atomic_inc(&chip->opencount);
       if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
               chip->can_set_rate=0;

and

       if (atomic_read(&chip->opencount)) {
               if (chip->opencount) {
                       changed = -EAGAIN;
               } else {
                       changed = set_digital_mode(chip, dmode);

It would be necessary to atomically increment or decrement the value
and use the returned result. And yet we still need to prevent other
threads making use of "can_set_rate" while we set it.

However in all but one case the atomic is misleading as they are already
running with "mode_mutex" held.

Decisions are made on mode setting are often intrinsically connected
to "opencount" because some operations are not permitted unless
there is sole ownership.

So instead simplify this, and use "mode_mutex" as a lock for all reference
counting and mode setting.

Signed-off-by: default avatarMark Hills <mark@xwax.org>
Link: https://lore.kernel.org/r/20200708101848.3457-2-mark@xwax.org


Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent db091b0e

sound/pci/echoaudio/echoaudio.c

+42 −34
Original line number Diff line number Diff line
@@ -245,13 +245,20 @@ static int hw_rule_sample_rate(struct snd_pcm_hw_params *params, 
						      SNDRV_PCM_HW_PARAM_RATE);

	struct echoaudio *chip = rule->private;

	struct snd_interval fixed;

	int err;



	mutex_lock(&chip->mode_mutex);



	if (!chip->can_set_rate) {

	if (chip->can_set_rate) {

		err = 0;

	} else {

		snd_interval_any(&fixed);

		fixed.min = fixed.max = chip->sample_rate;

		return snd_interval_refine(rate, &fixed);

		err = snd_interval_refine(rate, &fixed);

	}

	return 0;



	mutex_unlock(&chip->mode_mutex);

	return err;

}
@@ -322,7 +329,7 @@ static int pcm_open(struct snd_pcm_substream *substream, 
				       SNDRV_PCM_HW_PARAM_RATE, -1)) < 0)

		return err;



	/* Finally allocate a page for the scatter-gather list */

	/* Allocate a page for the scatter-gather list */

	if ((err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,

				       &chip->pci->dev,

				       PAGE_SIZE, &pipe->sgpage)) < 0) {
@@ -330,6 +337,17 @@ static int pcm_open(struct snd_pcm_substream *substream, 
		return err;

	}



	/*

	 * Sole ownership required to set the rate

	 */



	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",

		chip->opencount, chip->can_set_rate, chip->rate_set);



	chip->opencount++;

	if (chip->opencount > 1 && chip->rate_set)

		chip->can_set_rate = 0;



	return 0;

}
@@ -353,12 +371,7 @@ static int pcm_analog_in_open(struct snd_pcm_substream *substream) 
				       hw_rule_capture_format_by_channels, NULL,

				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)

		return err;

	atomic_inc(&chip->opencount);

	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)

		chip->can_set_rate=0;

	dev_dbg(chip->card->dev, "pcm_analog_in_open  cs=%d  oc=%d  r=%d\n",

		chip->can_set_rate, atomic_read(&chip->opencount),

		chip->sample_rate);



	return 0;

}
@@ -388,12 +401,7 @@ static int pcm_analog_out_open(struct snd_pcm_substream *substream) 
				       NULL,

				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)

		return err;

	atomic_inc(&chip->opencount);

	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)

		chip->can_set_rate=0;

	dev_dbg(chip->card->dev, "pcm_analog_out_open  cs=%d  oc=%d  r=%d\n",

		chip->can_set_rate, atomic_read(&chip->opencount),

		chip->sample_rate);



	return 0;

}
@@ -429,10 +437,6 @@ static int pcm_digital_in_open(struct snd_pcm_substream *substream) 
				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)

		goto din_exit;



	atomic_inc(&chip->opencount);

	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)

		chip->can_set_rate=0;



din_exit:

	mutex_unlock(&chip->mode_mutex);

	return err;
@@ -471,9 +475,7 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream) 
				       NULL, SNDRV_PCM_HW_PARAM_CHANNELS,

				       -1)) < 0)

		goto dout_exit;

	atomic_inc(&chip->opencount);

	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)

		chip->can_set_rate=0;



dout_exit:

	mutex_unlock(&chip->mode_mutex);

	return err;
@@ -488,23 +490,29 @@ dout_exit: 
static int pcm_close(struct snd_pcm_substream *substream)

{

	struct echoaudio *chip = snd_pcm_substream_chip(substream);

	int oc;



	/* Nothing to do here. Audio is already off and pipe will be

	 * freed by its callback

	 */



	atomic_dec(&chip->opencount);

	oc = atomic_read(&chip->opencount);

	dev_dbg(chip->card->dev, "pcm_close  oc=%d  cs=%d  rs=%d\n", oc,

		chip->can_set_rate, chip->rate_set);

	if (oc < 2)

	mutex_lock(&chip->mode_mutex);



	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",

		chip->opencount, chip->can_set_rate, chip->rate_set);



	chip->opencount--;



	switch (chip->opencount) {

	case 1:

		chip->can_set_rate = 1;

	if (oc == 0)

		break;



	case 0:

		chip->rate_set = 0;

	dev_dbg(chip->card->dev, "pcm_close2 oc=%d  cs=%d  rs=%d\n", oc,

		chip->can_set_rate, chip->rate_set);

		break;

	}



	mutex_unlock(&chip->mode_mutex);

	return 0;

}
@@ -1409,7 +1417,7 @@ static int snd_echo_digital_mode_put(struct snd_kcontrol *kcontrol, 
		/* Do not allow the user to change the digital mode when a pcm

		device is open because it also changes the number of channels

		and the allowed sample rates */

		if (atomic_read(&chip->opencount)) {

		if (chip->opencount) {

			changed = -EAGAIN;

		} else {

			changed = set_digital_mode(chip, dmode);
@@ -1874,7 +1882,7 @@ static int snd_echo_create(struct snd_card *card, 
		chip->card = card;

		chip->pci = pci;

		chip->irq = -1;

		atomic_set(&chip->opencount, 0);

		chip->opencount = 0;

		mutex_init(&chip->mode_mutex);

		chip->can_set_rate = 1;

	} else {

sound/pci/echoaudio/echoaudio.h

+3 −3
Original line number Diff line number Diff line
@@ -336,7 +336,7 @@ struct echoaudio { 
	struct mutex mode_mutex;

	u16 num_digital_modes, digital_mode_list[6];

	u16 num_clock_sources, clock_source_list[10];

	atomic_t opencount;

	unsigned int opencount;  /* protected by mode_mutex */

	struct snd_kcontrol *clock_src_ctl;

	struct snd_pcm *analog_pcm, *digital_pcm;

	struct snd_card *card;
@@ -353,8 +353,8 @@ struct echoaudio { 
	struct timer_list timer;

	char tinuse;				/* Timer in use */

	char midi_full;				/* MIDI output buffer is full */

	char can_set_rate;

	char rate_set;

	char can_set_rate;                      /* protected by mode_mutex */

	char rate_set;                          /* protected by mode_mutex */



	/* This stuff is used mainly by the lowlevel code */

	struct comm_page *comm_page;	/* Virtual address of the memory

CRA Git | Maintained and supported by SUSTech CRA and CCSE