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; }