[alsa-devel] [patch 1/2] OSS: soundcard: locking bug in sound_ioctl()
We shouldn't return directly here because we're still holding the &soundcard_mutex.
This bug goes all the way back to the start of git. It's strange that no one has complained about it as a runtime bug.
CC: stable@kernel.org Signed-off-by: Dan Carpenter error27@gmail.com
diff --git a/sound/oss/soundcard.c b/sound/oss/soundcard.c index 938ed94..a5ab61e 100644 --- a/sound/oss/soundcard.c +++ b/sound/oss/soundcard.c @@ -392,11 +392,11 @@ static long sound_ioctl(struct file *file, unsigned int cmd, unsigned long arg) case SND_DEV_DSP: case SND_DEV_DSP16: case SND_DEV_AUDIO: - return audio_ioctl(dev, file, cmd, p); + ret = audio_ioctl(dev, file, cmd, p); break;
case SND_DEV_MIDIN: - return MIDIbuf_ioctl(dev, file, cmd, p); + ret = MIDIbuf_ioctl(dev, file, cmd, p); break;
}
On Sunday 10 October 2010 19:33:52 Dan Carpenter wrote:
We shouldn't return directly here because we're still holding the &soundcard_mutex.
This bug goes all the way back to the start of git. It's strange that no one has complained about it as a runtime bug.
CC: stable@kernel.org Signed-off-by: Dan Carpenter error27@gmail.com
It was only recently converted to a mutex from the BKL, which is much more friendly to misusage because it is automatically released when the kernel sleeps or when the program exits.
The behavior was already broken with the BKL but the problem was far less visible. I fear we might be seeing more of these as fallout from the BKL removal. Sparse should be able to detect most of these cases though, so maybe we can look more carefully for them.
Acked-by: Arnd Bergmann arnd@arndb.de
Arnd
On Sunday 10 October 2010 20:39:34 Arnd Bergmann wrote:
On Sunday 10 October 2010 19:33:52 Dan Carpenter wrote:
We shouldn't return directly here because we're still holding the &soundcard_mutex.
This bug goes all the way back to the start of git. It's strange that no one has complained about it as a runtime bug.
CC: stable@kernel.org Signed-off-by: Dan Carpenter error27@gmail.com
It was only recently converted to a mutex from the BKL, which is much more friendly to misusage because it is automatically released when the kernel sleeps or when the program exits.
The behavior was already broken with the BKL but the problem was far less visible. I fear we might be seeing more of these as fallout from the BKL removal. Sparse should be able to detect most of these cases though, so maybe we can look more carefully for them.
Hmm, actually sparse does *not* warn about sound_ioctl returning in different lock contexts. Sparse developers: is there a known limitation in sparse for this? I expected to see context warnings because sound_ioctl normally releases soundcard_mutex (previously lock_kernel) in some cases returns while holding the lock.
Arnd
On Mon, 2010-10-11 at 10:13 +0200, Arnd Bergmann wrote:
Hmm, actually sparse does *not* warn about sound_ioctl returning in different lock contexts. Sparse developers: is there a known limitation in sparse for this? I expected to see context warnings because sound_ioctl normally releases soundcard_mutex (previously lock_kernel) in some cases returns while holding the lock.
Arnd, mutexes aren't annotated in the kernel source to make use of sparse's context checking.
johannes
On Monday 11 October 2010, Johannes Berg wrote:
On Mon, 2010-10-11 at 10:13 +0200, Arnd Bergmann wrote:
Hmm, actually sparse does not warn about sound_ioctl returning in different lock contexts. Sparse developers: is there a known limitation in sparse for this? I expected to see context warnings because sound_ioctl normally releases soundcard_mutex (previously lock_kernel) in some cases returns while holding the lock.
Arnd, mutexes aren't annotated in the kernel source to make use of sparse's context checking.
D'oh. I never realized this was only done for some types of locks. Is there a reason why we don't want mutexes to be annotated or do we just need someone to do it?
Arnd
On Mon, 2010-10-11 at 12:50 +0200, Arnd Bergmann wrote:
On Monday 11 October 2010, Johannes Berg wrote:
On Mon, 2010-10-11 at 10:13 +0200, Arnd Bergmann wrote:
Hmm, actually sparse does not warn about sound_ioctl returning in different lock contexts. Sparse developers: is there a known limitation in sparse for this? I expected to see context warnings because sound_ioctl normally releases soundcard_mutex (previously lock_kernel) in some cases returns while holding the lock.
Arnd, mutexes aren't annotated in the kernel source to make use of sparse's context checking.
D'oh. I never realized this was only done for some types of locks. Is there a reason why we don't want mutexes to be annotated or do we just need someone to do it?
I don't know. Could be related to trylock issues, could be just historic since semaphores can't really be annotated, or could be something else entirely... I would expect a huge amount of warnings from sparse though if you "just" annotate them since there are things like rtnl_lock() which would have to propagate context.
johannes
At Sun, 10 Oct 2010 19:33:52 +0200, Dan Carpenter wrote:
We shouldn't return directly here because we're still holding the &soundcard_mutex.
This bug goes all the way back to the start of git. It's strange that no one has complained about it as a runtime bug.
CC: stable@kernel.org Signed-off-by: Dan Carpenter error27@gmail.com
Applied now. Thanks.
Takashi
diff --git a/sound/oss/soundcard.c b/sound/oss/soundcard.c index 938ed94..a5ab61e 100644 --- a/sound/oss/soundcard.c +++ b/sound/oss/soundcard.c @@ -392,11 +392,11 @@ static long sound_ioctl(struct file *file, unsigned int cmd, unsigned long arg) case SND_DEV_DSP: case SND_DEV_DSP16: case SND_DEV_AUDIO:
return audio_ioctl(dev, file, cmd, p);
ret = audio_ioctl(dev, file, cmd, p);break;
case SND_DEV_MIDIN:
return MIDIbuf_ioctl(dev, file, cmd, p);
ret = MIDIbuf_ioctl(dev, file, cmd, p);break;
}
participants (4)
-
Arnd Bergmann -
Dan Carpenter -
Johannes Berg -
Takashi Iwai