Re: aconnect occasionally causes kernel oopses

From: Takashi Iwai <tiwai@suse.de>
To: folkert <folkert@vanheusden.com>
Cc: alsa-devel@alsa-project.org
Subject: Re: aconnect occasionally causes kernel oopses
Date: Tue, 03 Aug 2021 09:49:00 +0200	[thread overview]
Message-ID: <s5hlf5jvuhf.wl-tiwai@suse.de> (raw)
In-Reply-To: <20210803074451.GY890690@belle.intranet.vanheusden.com>

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

On Tue, 03 Aug 2021 09:44:51 +0200,
folkert wrote:
> 
> > > To which kernel version should I apply it?
> > > Because:
> > > 
> > > folkert@oensoens:~/linux-5.11.0$ patch -p1 < ~/alsa-patch.diff 
> > > patching file sound/core/seq/seq_ports.c
> > > patching file sound/core/seq/seq_ports.h
> > > patching file sound/core/seq/seq_ports.c
> > > Hunk #1 succeeded at 526 (offset 12 lines).
> > > Hunk #2 succeeded at 538 (offset 12 lines).
> > > Hunk #3 succeeded at 547 with fuzz 2 (offset 12 lines).
> > > Hunk #4 FAILED at 602.
> > > 1 out of 4 hunks FAILED -- saving rejects to file sound/core/seq/seq_ports.c.rej
> > 
> > Did you scratch the previous patch?  AFAIK, the latest patch should be
> > applicable to 5.11 and older cleanly.
> 
> I did.
> It also fails on 5.13.7.

Weird, here it's applied cleanly on my machine with every kernel
version.  Double-check whether you really have any other changes.
I attach the patch again but with an attachment just to be sure.


Takashi


[-- Attachment #2: 0001-ALSA-seq-Fix-racy-deletion-of-subscriber.patch --]
[-- Type: application/octet-stream, Size: 4266 bytes --]

From a474c4b6224556b39e20a3d13bdb56b5e6984f78 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Tue, 3 Aug 2021 08:43:21 +0200
Subject: [PATCH] ALSA: seq: Fix racy deletion of subscriber

It turned out that the current implementation of the port subscription
is racy.  The subscription contains two linked lists, and we have to
add to or delete from both lists.  Since both connection and
disconnection procedures perform the same order for those two lists
(i.e. src list, then dest list), when a deletion happens during a
connection procedure, the src list may be deleted before the dest list
addition completes, and this may lead to a use-after-free or an Oops,
even though the access to both lists are protected via mutex.

The simple workaround for this race is to change the access order for
the disconnection, namely, dest list, then src list.  This assures
that the connection has been established when disconnecting, and also
the concurrent deletion can be avoided.

Reported-by: folkert <folkert@vanheusden.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20210801182754.GP890690@belle.intranet.vanheusden.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/seq/seq_ports.c | 39 ++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c
index b9c2ce2b8d5a..84d78630463e 100644
--- a/sound/core/seq/seq_ports.c
+++ b/sound/core/seq/seq_ports.c
@@ -514,10 +514,11 @@ static int check_and_subscribe_port(struct snd_seq_client *client,
 	return err;
 }
 
-static void delete_and_unsubscribe_port(struct snd_seq_client *client,
-					struct snd_seq_client_port *port,
-					struct snd_seq_subscribers *subs,
-					bool is_src, bool ack)
+/* called with grp->list_mutex held */
+static void __delete_and_unsubscribe_port(struct snd_seq_client *client,
+					  struct snd_seq_client_port *port,
+					  struct snd_seq_subscribers *subs,
+					  bool is_src, bool ack)
 {
 	struct snd_seq_port_subs_info *grp;
 	struct list_head *list;
@@ -525,7 +526,6 @@ static void delete_and_unsubscribe_port(struct snd_seq_client *client,
 
 	grp = is_src ? &port->c_src : &port->c_dest;
 	list = is_src ? &subs->src_list : &subs->dest_list;
-	down_write(&grp->list_mutex);
 	write_lock_irq(&grp->list_lock);
 	empty = list_empty(list);
 	if (!empty)
@@ -535,6 +535,18 @@ static void delete_and_unsubscribe_port(struct snd_seq_client *client,
 
 	if (!empty)
 		unsubscribe_port(client, port, grp, &subs->info, ack);
+}
+
+static void delete_and_unsubscribe_port(struct snd_seq_client *client,
+					struct snd_seq_client_port *port,
+					struct snd_seq_subscribers *subs,
+					bool is_src, bool ack)
+{
+	struct snd_seq_port_subs_info *grp;
+
+	grp = is_src ? &port->c_src : &port->c_dest;
+	down_write(&grp->list_mutex);
+	__delete_and_unsubscribe_port(client, port, subs, is_src, ack);
 	up_write(&grp->list_mutex);
 }
 
@@ -590,27 +602,30 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector,
 			    struct snd_seq_client_port *dest_port,
 			    struct snd_seq_port_subscribe *info)
 {
-	struct snd_seq_port_subs_info *src = &src_port->c_src;
+	struct snd_seq_port_subs_info *dest = &dest_port->c_dest;
 	struct snd_seq_subscribers *subs;
 	int err = -ENOENT;
 
-	down_write(&src->list_mutex);
+	/* always start from deleting the dest port for avoiding concurrent
+	 * deletions
+	 */
+	down_write(&dest->list_mutex);
 	/* look for the connection */
-	list_for_each_entry(subs, &src->list_head, src_list) {
+	list_for_each_entry(subs, &dest->list_head, dest_list) {
 		if (match_subs_info(info, &subs->info)) {
-			atomic_dec(&subs->ref_count); /* mark as not ready */
+			__delete_and_unsubscribe_port(dest_client, dest_port,
+						      subs, false,
+						      connector->number != dest_client->number);
 			err = 0;
 			break;
 		}
 	}
-	up_write(&src->list_mutex);
+	up_write(&dest->list_mutex);
 	if (err < 0)
 		return err;
 
 	delete_and_unsubscribe_port(src_client, src_port, subs, true,
 				    connector->number != src_client->number);
-	delete_and_unsubscribe_port(dest_client, dest_port, subs, false,
-				    connector->number != dest_client->number);
 	kfree(subs);
 	return 0;
 }
-- 
2.26.2


[-- Attachment #3: Type: text/plain, Size: 1 bytes --]


