aconnect occasionally causes kernel oopses
Takashi Iwai
tiwai at suse.de
Tue Aug 3 09:43:35 CEST 2021
On Tue, 03 Aug 2021 09:40:50 +0200,
folkert wrote:
>
> 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
Did you scratch the previous patch? AFAIK, the latest patch should be
applicable to 5.11 and older cleanly.
Takashi
>
> 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