* Re: [bug report] ALSA: pcm: Return -EBUSY for OSS ioctls changing busy streams
2018-03-27 12:35 ` Takashi Iwai
@ 2018-03-27 14:23 ` Takashi Iwai
0 siblings, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2018-03-27 14:23 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel
On Tue, 27 Mar 2018 14:35:50 +0200,
Takashi Iwai wrote:
>
> On Tue, 27 Mar 2018 14:27:30 +0200,
> Dan Carpenter wrote:
> >
> > Hello Takashi Iwai,
> >
> > The patch 40cab6e88cb0: "ALSA: pcm: Return -EBUSY for OSS ioctls
> > changing busy streams" from Mar 23, 2018, leads to the following
> > static checker warning:
> >
> > sound/core/oss/pcm_oss.c:1741 snd_pcm_oss_set_rate()
> > warn: inconsistent returns 'mutex:&runtime->oss.params_lock'.
> > Locked on: line 1734
> > Unlocked on: line 1732
> > line 1741
> >
> > sound/core/oss/pcm_oss.c
> > 1717 static int snd_pcm_oss_set_rate(struct snd_pcm_oss_file *pcm_oss_file, int rate)
> > 1718 {
> > 1719 int idx;
> > 1720
> > 1721 for (idx = 1; idx >= 0; --idx) {
> > 1722 struct snd_pcm_substream *substream = pcm_oss_file->streams[idx];
> > 1723 struct snd_pcm_runtime *runtime;
> > 1724 if (substream == NULL)
> > 1725 continue;
> > 1726 runtime = substream->runtime;
> > 1727 if (rate < 1000)
> > 1728 rate = 1000;
> > 1729 else if (rate > 192000)
> > 1730 rate = 192000;
> > 1731 if (mutex_lock_interruptible(&runtime->oss.params_lock))
> > 1732 return -ERESTARTSYS;
> > 1733 if (atomic_read(&runtime->oss.rw_ref))
> > 1734 return -EBUSY;
> > ^^^^^^^^^^^^^
> > Unlock before returning?
>
> My bad, it has to be checked before mutex_lock.
> There is one another place doing the same mistake.
>
> The fix patch is below.
Thinking of it again, a revised version below should work better.
The previous fix had a slight race window between the atomic check and
the mutex lock.
Takashi
-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: pcm: Fix mutex unbalance in OSS emulation ioctls
The previous fix 40cab6e88cb0 ("ALSA: pcm: Return -EBUSY for OSS
ioctls changing busy streams") introduced some mutex unbalance; the
check of runtime->oss.rw_ref was inserted in a wrong place after the
mutex lock.
This patch fixes the inconsistency by rewriting with the helper
functions to lock/unlock parameters with the stream check.
Fixes: 40cab6e88cb0 ("ALSA: pcm: Return -EBUSY for OSS ioctls changing busy streams")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/core/oss/pcm_oss.c | 67 ++++++++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 25 deletions(-)
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
index a9082f219561..f19b4586e33b 100644
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -823,6 +823,23 @@ static int choose_rate(struct snd_pcm_substream *substream,
return snd_pcm_hw_param_near(substream, params, SNDRV_PCM_HW_PARAM_RATE, best_rate, NULL);
}
+/* parameter locking: returns immediately if tried during streaming */
+static int lock_params(struct snd_pcm_runtime *runtime)
+{
+ if (mutex_lock_interruptible(&runtime->oss.params_lock))
+ return -ERESTARTSYS;
+ if (atomic_read(&runtime->oss.rw_ref)) {
+ mutex_unlock(&runtime->oss.params_lock);
+ return -EBUSY;
+ }
+ return 0;
+}
+
+static void unlock_params(struct snd_pcm_runtime *runtime)
+{
+ mutex_unlock(&runtime->oss.params_lock);
+}
+
/* call with params_lock held */
static int snd_pcm_oss_change_params_locked(struct snd_pcm_substream *substream)
{
@@ -1721,6 +1738,8 @@ static int snd_pcm_oss_set_rate(struct snd_pcm_oss_file *pcm_oss_file, int rate)
for (idx = 1; idx >= 0; --idx) {
struct snd_pcm_substream *substream = pcm_oss_file->streams[idx];
struct snd_pcm_runtime *runtime;
+ int err;
+
if (substream == NULL)
continue;
runtime = substream->runtime;
@@ -1728,15 +1747,14 @@ static int snd_pcm_oss_set_rate(struct snd_pcm_oss_file *pcm_oss_file, int rate)
rate = 1000;
else if (rate > 192000)
rate = 192000;
- if (mutex_lock_interruptible(&runtime->oss.params_lock))
- return -ERESTARTSYS;
- if (atomic_read(&runtime->oss.rw_ref))
- return -EBUSY;
+ err = lock_params(runtime);
+ if (err < 0)
+ return err;
if (runtime->oss.rate != rate) {
runtime->oss.params = 1;
runtime->oss.rate = rate;
}
- mutex_unlock(&runtime->oss.params_lock);
+ unlock_params(runtime);
}
return snd_pcm_oss_get_rate(pcm_oss_file);
}
@@ -1761,18 +1779,19 @@ static int snd_pcm_oss_set_channels(struct snd_pcm_oss_file *pcm_oss_file, unsig
for (idx = 1; idx >= 0; --idx) {
struct snd_pcm_substream *substream = pcm_oss_file->streams[idx];
struct snd_pcm_runtime *runtime;
+ int err;
+
if (substream == NULL)
continue;
runtime = substream->runtime;
- if (mutex_lock_interruptible(&runtime->oss.params_lock))
- return -ERESTARTSYS;
- if (atomic_read(&runtime->oss.rw_ref))
- return -EBUSY;
+ err = lock_params(runtime);
+ if (err < 0)
+ return err;
if (runtime->oss.channels != channels) {
runtime->oss.params = 1;
runtime->oss.channels = channels;
}
- mutex_unlock(&runtime->oss.params_lock);
+ unlock_params(runtime);
}
return snd_pcm_oss_get_channels(pcm_oss_file);
}
@@ -1845,6 +1864,7 @@ static int snd_pcm_oss_get_formats(struct snd_pcm_oss_file *pcm_oss_file)
static int snd_pcm_oss_set_format(struct snd_pcm_oss_file *pcm_oss_file, int format)
{
int formats, idx;
+ int err;
if (format != AFMT_QUERY) {
formats = snd_pcm_oss_get_formats(pcm_oss_file);
@@ -1858,15 +1878,14 @@ static int snd_pcm_oss_set_format(struct snd_pcm_oss_file *pcm_oss_file, int for
if (substream == NULL)
continue;
runtime = substream->runtime;
- if (atomic_read(&runtime->oss.rw_ref))
- return -EBUSY;
- if (mutex_lock_interruptible(&runtime->oss.params_lock))
- return -ERESTARTSYS;
+ err = lock_params(runtime);
+ if (err < 0)
+ return err;
if (runtime->oss.format != format) {
runtime->oss.params = 1;
runtime->oss.format = format;
}
- mutex_unlock(&runtime->oss.params_lock);
+ unlock_params(runtime);
}
}
return snd_pcm_oss_get_format(pcm_oss_file);
@@ -1914,12 +1933,11 @@ static int snd_pcm_oss_set_subdivide(struct snd_pcm_oss_file *pcm_oss_file, int
if (substream == NULL)
continue;
runtime = substream->runtime;
- if (atomic_read(&runtime->oss.rw_ref))
- return -EBUSY;
- if (mutex_lock_interruptible(&runtime->oss.params_lock))
- return -ERESTARTSYS;
+ err = lock_params(runtime);
+ if (err < 0)
+ return err;
err = snd_pcm_oss_set_subdivide1(substream, subdivide);
- mutex_unlock(&runtime->oss.params_lock);
+ unlock_params(runtime);
if (err < 0)
return err;
}
@@ -1954,12 +1972,11 @@ static int snd_pcm_oss_set_fragment(struct snd_pcm_oss_file *pcm_oss_file, unsig
if (substream == NULL)
continue;
runtime = substream->runtime;
- if (atomic_read(&runtime->oss.rw_ref))
- return -EBUSY;
- if (mutex_lock_interruptible(&runtime->oss.params_lock))
- return -ERESTARTSYS;
+ err = lock_params(runtime);
+ if (err < 0)
+ return err;
err = snd_pcm_oss_set_fragment1(substream, val);
- mutex_unlock(&runtime->oss.params_lock);
+ unlock_params(runtime);
if (err < 0)
return err;
}
--
2.16.2
^ permalink raw reply related [flat|nested] 3+ messages in thread