aconnect occasionally causes kernel oopses
Takashi Iwai
tiwai at suse.de
Tue Aug 3 08:55:36 CEST 2021
On Mon, 02 Aug 2021 21:53:49 +0200,
folkert wrote:
>
> > > [ kernel bug if repeatingly aconnect'ing midi devices ]
> > >
> > > > Does this happen if you do reconnect of kernel sequencer client?
> > > > You can use snd-virmidi as well as snd-dummy.
> > > > I'm asking it because it'll simplify the test a lot, which will be
> > > > almost self-contained.
> > >
> > > Like this?
> > >
> > > root at lappiemctopface:~# aplaymidi -l
> > > Port Client name Port name
> > > 14:0 Midi Through Midi Through Port-0
> > > 20:0 Virtual Raw MIDI 1-0 VirMIDI 1-0
> > > 21:0 Virtual Raw MIDI 1-1 VirMIDI 1-1
> > > 22:0 Virtual Raw MIDI 1-2 VirMIDI 1-2
> > > 23:0 Virtual Raw MIDI 1-3 VirMIDI 1-3
> > > 128:0 rtpmidi lappiemctopface Network
> > > 128:1 rtpmidi lappiemctopface metronoom
> > > 128:2 rtpmidi lappiemctopface AppleMidi2IPMidiBridge
> > > 128:3 rtpmidi lappiemctopface oensoens
> > > 130:0 FLUID Synth (11462) Synth input port (11462:0)
> > >
> > > and then:
> > >
> > > root at lappiemctopface:~# cat test.sh
> > > while true
> > > do
> > > aconnect 20:0 21:0
> > > aconnect -d 20:0 21:0
> > > done
> > >
> > > root at lappiemctopface:~# for i in `seq 0 3` ; do (./test.sh &) ; done
> > >
> > > This hard locks-up my laptop: it doesn't even respond to capslock (led
> > > on/off) anymore nor the ctrl+prtscr+alt+b combination.
> >
> > Thanks, that's really helpful!
> > I see the possible race now.
>
> > Could you try the quick fix below?
>
> Something may have changed:
>
> folkert at oensoens:~$ aplaymidi -l
> ALSA lib seq_hw.c:466:(snd_seq_hw_open) open /dev/snd/seq failed: Permission denied
> Cannot open sequencer - Permission denied
>
> ???
Hrm, that's unexpected.
Meanwhile, I reconsidered the fix and a better idea came after the
sleep. Below is the new fix patch. Could you give it a try instead
of the previous one?
thanks,
Takashi
-- 8< --
From: Takashi Iwai <tiwai at suse.de>
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 at vanheusden.com>
Cc: <stable at vger.kernel.org>
Link: https://lore.kernel.org/r/20210801182754.GP890690@belle.intranet.vanheusden.com
Signed-off-by: Takashi Iwai <tiwai at 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
More information about the Alsa-devel
mailing list