aconnect occasionally causes kernel oopses
folkert
folkert at vanheusden.com
Tue Aug 3 09:40:50 CEST 2021
Hi,
To which kernel version should I apply it?
Because:
folkert at 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
On Tue, Aug 03, 2021 at 08:55:36AM +0200, Takashi Iwai wrote:
> 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
Folkert van Heusden
--
MultiTail is a versatile tool for watching logfiles and output of
commands. Filtering, coloring, merging, diff-view, etc.
http://www.vanheusden.com/multitail/
----------------------------------------------------------------------
Phone: +31-6-41278122, PGP-key: 1F28D8AE, www.vanheusden.com
More information about the Alsa-devel
mailing list