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: Mark Hills <mark@xwax.org>
Link: https://lore.kernel.org/r/20200708101848.3457-2-mark@xwax.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index bee3271..f2bc37c 100644 (file)
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -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 {

diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h
index be4d048..6fd283e 100644 (file)
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -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