linux-kernel - [PATCH 4.14 36/67] ALSA: pcm: Fix starvation on down_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20181211151632.228099839@linuxfoundation.org>
Date:   Tue, 11 Dec 2018 16:41:36 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     linux-kernel@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        stable@...r.kernel.org, Wonmin Jung <wonmin.jung@....com>,
        Chanho Min <chanho.min@....com>, Takashi Iwai <tiwai@...e.de>
Subject: [PATCH 4.14 36/67] ALSA: pcm: Fix starvation on down_write_nonblock()

4.14-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Chanho Min <chanho.min@....com>

commit b888a5f713e4d17faaaff24316585a4eb07f35b7 upstream.

Commit 67ec1072b053 ("ALSA: pcm: Fix rwsem deadlock for non-atomic PCM
stream") fixes deadlock for non-atomic PCM stream. But, This patch
causes antother stuck.
If writer is RT thread and reader is a normal thread, the reader
thread will be difficult to get scheduled. It may not give chance to
release readlocks and writer gets stuck for a long time if they are
pinned to single cpu.

The deadlock described in the previous commit is because the linux
rwsem queues like a FIFO. So, we might need non-FIFO writelock, not
non-block one.

My suggestion is that the writer gives reader a chance to be scheduled
by using the minimum msleep() instaed of spinning without blocking by
writer. Also, The *_nonblock may be changed to *_nonfifo appropriately
to this concept.
In terms of performance, when trylock is failed, this minimum periodic
msleep will have the same performance as the tick-based
schedule()/wake_up_q().

[ Although this has a fairly high performance penalty, the relevant
  code path became already rare due to the previous commit ("ALSA:
  pcm: Call snd_pcm_unlink() conditionally at closing").  That is, now
  this unconditional msleep appears only when using linked streams,
  and this must be a rare case.  So we accept this as a quick
  workaround until finding a more suitable one -- tiwai ]

Fixes: 67ec1072b053 ("ALSA: pcm: Fix rwsem deadlock for non-atomic PCM stream")
Suggested-by: Wonmin Jung <wonmin.jung@....com>
Signed-off-by: Chanho Min <chanho.min@....com>
Cc: <stable@...r.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@...e.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>

---
 sound/core/pcm_native.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -36,6 +36,7 @@
 #include <sound/timer.h>
 #include <sound/minors.h>
 #include <linux/uio.h>
+#include <linux/delay.h>

 #include "pcm_local.h"

@@ -91,12 +92,12 @@ static DECLARE_RWSEM(snd_pcm_link_rwsem)
  * and this may lead to a deadlock when the code path takes read sem
  * twice (e.g. one in snd_pcm_action_nonatomic() and another in
  * snd_pcm_stream_lock()).  As a (suboptimal) workaround, let writer to
- * spin until it gets the lock.
+ * sleep until all the readers are completed without blocking by writer.
  */
-static inline void down_write_nonblock(struct rw_semaphore *lock)
+static inline void down_write_nonfifo(struct rw_semaphore *lock)
 {
 	while (!down_write_trylock(lock))
-		cond_resched();
+		msleep(1);
 }

 /**
@@ -1935,7 +1936,7 @@ static int snd_pcm_link(struct snd_pcm_s
 		res = -ENOMEM;
 		goto _nolock;
 	}
-	down_write_nonblock(&snd_pcm_link_rwsem);
+	down_write_nonfifo(&snd_pcm_link_rwsem);
 	write_lock_irq(&snd_pcm_link_rwlock);
 	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN ||
 	    substream->runtime->status->state != substream1->runtime->status->state ||
@@ -1982,7 +1983,7 @@ static int snd_pcm_unlink(struct snd_pcm
 	struct snd_pcm_substream *s;
 	int res = 0;

-	down_write_nonblock(&snd_pcm_link_rwsem);
+	down_write_nonfifo(&snd_pcm_link_rwsem);
 	write_lock_irq(&snd_pcm_link_rwlock);
 	if (!snd_pcm_stream_linked(substream)) {
 		res = -EALREADY;