Commit e53ec492 authored by Takashi Iwai's avatar Takashi Iwai Committed by Greg Kroah-Hartman

ALSA: seq: Fix lockdep warnings due to double mutex locks

commit 7f0973e9 upstream.

The port subscription code uses double mutex locks for source and
destination ports, and this may become racy once when wrongly set up.
It leads to lockdep warning splat, typically triggered by fuzzer like
syzkaller, although the actual deadlock hasn't been seen, so far.

This patch simplifies the handling by reducing to two single locks, so
that no lockdep warning will be trigger any longer.

By splitting to two actions, a still-in-progress element shall be
added in one list while handling another.  For ignoring this element,
a new check is added in deliver_to_subscribers().

Along with it, the code to add/remove the subscribers list element was
cleaned up and refactored.

BugLink: http://lkml.kernel.org/r/CACT4Y+aKQXV7xkBW9hpQbzaDO7LrUvohxWh-UwMxXjDy-yBD=A@mail.gmail.comReported-by: default avatarDmitry Vyukov <dvyukov@google.com>
Tested-by: default avatarDmitry Vyukov <dvyukov@google.com>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 14bdca3f
...@@ -678,6 +678,9 @@ static int deliver_to_subscribers(struct snd_seq_client *client, ...@@ -678,6 +678,9 @@ static int deliver_to_subscribers(struct snd_seq_client *client,
else else
down_read(&grp->list_mutex); down_read(&grp->list_mutex);
list_for_each_entry(subs, &grp->list_head, src_list) { list_for_each_entry(subs, &grp->list_head, src_list) {
/* both ports ready? */
if (atomic_read(&subs->ref_count) != 2)
continue;
event->dest = subs->info.dest; event->dest = subs->info.dest;
if (subs->info.flags & SNDRV_SEQ_PORT_SUBS_TIMESTAMP) if (subs->info.flags & SNDRV_SEQ_PORT_SUBS_TIMESTAMP)
/* convert time according to flag with subscription */ /* convert time according to flag with subscription */
......
...@@ -175,10 +175,6 @@ struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client, ...@@ -175,10 +175,6 @@ struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client,
} }
/* */ /* */
enum group_type {
SRC_LIST, DEST_LIST
};
static int subscribe_port(struct snd_seq_client *client, static int subscribe_port(struct snd_seq_client *client,
struct snd_seq_client_port *port, struct snd_seq_client_port *port,
struct snd_seq_port_subs_info *grp, struct snd_seq_port_subs_info *grp,
...@@ -205,6 +201,20 @@ static struct snd_seq_client_port *get_client_port(struct snd_seq_addr *addr, ...@@ -205,6 +201,20 @@ static struct snd_seq_client_port *get_client_port(struct snd_seq_addr *addr,
return NULL; return NULL;
} }
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);
static inline struct snd_seq_subscribers *
get_subscriber(struct list_head *p, bool is_src)
{
if (is_src)
return list_entry(p, struct snd_seq_subscribers, src_list);
else
return list_entry(p, struct snd_seq_subscribers, dest_list);
}
/* /*
* remove all subscribers on the list * remove all subscribers on the list
* this is called from port_delete, for each src and dest list. * this is called from port_delete, for each src and dest list.
...@@ -212,7 +222,7 @@ static struct snd_seq_client_port *get_client_port(struct snd_seq_addr *addr, ...@@ -212,7 +222,7 @@ static struct snd_seq_client_port *get_client_port(struct snd_seq_addr *addr,
static void clear_subscriber_list(struct snd_seq_client *client, static void clear_subscriber_list(struct snd_seq_client *client,
struct snd_seq_client_port *port, struct snd_seq_client_port *port,
struct snd_seq_port_subs_info *grp, struct snd_seq_port_subs_info *grp,
int grptype) int is_src)
{ {
struct list_head *p, *n; struct list_head *p, *n;
...@@ -221,15 +231,13 @@ static void clear_subscriber_list(struct snd_seq_client *client, ...@@ -221,15 +231,13 @@ static void clear_subscriber_list(struct snd_seq_client *client,
struct snd_seq_client *c; struct snd_seq_client *c;
struct snd_seq_client_port *aport; struct snd_seq_client_port *aport;
if (grptype == SRC_LIST) { subs = get_subscriber(p, is_src);
subs = list_entry(p, struct snd_seq_subscribers, src_list); if (is_src)
aport = get_client_port(&subs->info.dest, &c); aport = get_client_port(&subs->info.dest, &c);
} else { else
subs = list_entry(p, struct snd_seq_subscribers, dest_list);
aport = get_client_port(&subs->info.sender, &c); aport = get_client_port(&subs->info.sender, &c);
} delete_and_unsubscribe_port(client, port, subs, is_src, false);
list_del(p);
unsubscribe_port(client, port, grp, &subs->info, 0);
if (!aport) { if (!aport) {
/* looks like the connected port is being deleted. /* looks like the connected port is being deleted.
* we decrease the counter, and when both ports are deleted * we decrease the counter, and when both ports are deleted
...@@ -237,21 +245,14 @@ static void clear_subscriber_list(struct snd_seq_client *client, ...@@ -237,21 +245,14 @@ static void clear_subscriber_list(struct snd_seq_client *client,
*/ */
if (atomic_dec_and_test(&subs->ref_count)) if (atomic_dec_and_test(&subs->ref_count))
kfree(subs); kfree(subs);
} else { continue;
/* ok we got the connected port */
struct snd_seq_port_subs_info *agrp;
agrp = (grptype == SRC_LIST) ? &aport->c_dest : &aport->c_src;
down_write(&agrp->list_mutex);
if (grptype == SRC_LIST)
list_del(&subs->dest_list);
else
list_del(&subs->src_list);
up_write(&agrp->list_mutex);
unsubscribe_port(c, aport, agrp, &subs->info, 1);
kfree(subs);
snd_seq_port_unlock(aport);
snd_seq_client_unlock(c);
} }
/* ok we got the connected port */
delete_and_unsubscribe_port(c, aport, subs, !is_src, true);
kfree(subs);
snd_seq_port_unlock(aport);
snd_seq_client_unlock(c);
} }
} }
...@@ -264,8 +265,8 @@ static int port_delete(struct snd_seq_client *client, ...@@ -264,8 +265,8 @@ static int port_delete(struct snd_seq_client *client,
snd_use_lock_sync(&port->use_lock); snd_use_lock_sync(&port->use_lock);
/* clear subscribers info */ /* clear subscribers info */
clear_subscriber_list(client, port, &port->c_src, SRC_LIST); clear_subscriber_list(client, port, &port->c_src, true);
clear_subscriber_list(client, port, &port->c_dest, DEST_LIST); clear_subscriber_list(client, port, &port->c_dest, false);
if (port->private_free) if (port->private_free)
port->private_free(port->private_data); port->private_free(port->private_data);
...@@ -484,85 +485,120 @@ static int match_subs_info(struct snd_seq_port_subscribe *r, ...@@ -484,85 +485,120 @@ static int match_subs_info(struct snd_seq_port_subscribe *r,
return 0; return 0;
} }
static int check_and_subscribe_port(struct snd_seq_client *client,
/* connect two ports */ struct snd_seq_client_port *port,
int snd_seq_port_connect(struct snd_seq_client *connector, struct snd_seq_subscribers *subs,
struct snd_seq_client *src_client, bool is_src, bool exclusive, bool ack)
struct snd_seq_client_port *src_port,
struct snd_seq_client *dest_client,
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 *grp;
struct snd_seq_port_subs_info *dest = &dest_port->c_dest; struct list_head *p;
struct snd_seq_subscribers *subs, *s; struct snd_seq_subscribers *s;
int err, src_called = 0; int err;
unsigned long flags;
int exclusive;
subs = kzalloc(sizeof(*subs), GFP_KERNEL); grp = is_src ? &port->c_src : &port->c_dest;
if (! subs)
return -ENOMEM;
subs->info = *info;
atomic_set(&subs->ref_count, 2);
down_write(&src->list_mutex);
down_write_nested(&dest->list_mutex, SINGLE_DEPTH_NESTING);
exclusive = info->flags & SNDRV_SEQ_PORT_SUBS_EXCLUSIVE ? 1 : 0;
err = -EBUSY; err = -EBUSY;
down_write(&grp->list_mutex);
if (exclusive) { if (exclusive) {
if (! list_empty(&src->list_head) || ! list_empty(&dest->list_head)) if (!list_empty(&grp->list_head))
goto __error; goto __error;
} else { } else {
if (src->exclusive || dest->exclusive) if (grp->exclusive)
goto __error; goto __error;
/* check whether already exists */ /* check whether already exists */
list_for_each_entry(s, &src->list_head, src_list) { list_for_each(p, &grp->list_head) {
if (match_subs_info(info, &s->info)) s = get_subscriber(p, is_src);
goto __error; if (match_subs_info(&subs->info, &s->info))
}
list_for_each_entry(s, &dest->list_head, dest_list) {
if (match_subs_info(info, &s->info))
goto __error; goto __error;
} }
} }
if ((err = subscribe_port(src_client, src_port, src, info, err = subscribe_port(client, port, grp, &subs->info, ack);
connector->number != src_client->number)) < 0) if (err < 0) {
goto __error; grp->exclusive = 0;
src_called = 1;
if ((err = subscribe_port(dest_client, dest_port, dest, info,
connector->number != dest_client->number)) < 0)
goto __error; goto __error;
}
/* add to list */ /* add to list */
write_lock_irqsave(&src->list_lock, flags); write_lock_irq(&grp->list_lock);
// write_lock(&dest->list_lock); // no other lock yet if (is_src)
list_add_tail(&subs->src_list, &src->list_head); list_add_tail(&subs->src_list, &grp->list_head);
list_add_tail(&subs->dest_list, &dest->list_head); else
// write_unlock(&dest->list_lock); // no other lock yet list_add_tail(&subs->dest_list, &grp->list_head);
write_unlock_irqrestore(&src->list_lock, flags); grp->exclusive = exclusive;
atomic_inc(&subs->ref_count);
write_unlock_irq(&grp->list_lock);
err = 0;
__error:
up_write(&grp->list_mutex);
return err;
}
src->exclusive = dest->exclusive = exclusive; 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);
write_lock_irq(&grp->list_lock);
if (is_src)
list_del(&subs->src_list);
else
list_del(&subs->dest_list);
grp->exclusive = 0;
write_unlock_irq(&grp->list_lock);
up_write(&grp->list_mutex);
unsubscribe_port(client, port, grp, &subs->info, ack);
}
/* connect two ports */
int snd_seq_port_connect(struct snd_seq_client *connector,
struct snd_seq_client *src_client,
struct snd_seq_client_port *src_port,
struct snd_seq_client *dest_client,
struct snd_seq_client_port *dest_port,
struct snd_seq_port_subscribe *info)
{
struct snd_seq_subscribers *subs;
bool exclusive;
int err;
subs = kzalloc(sizeof(*subs), GFP_KERNEL);
if (!subs)
return -ENOMEM;
subs->info = *info;
atomic_set(&subs->ref_count, 0);
INIT_LIST_HEAD(&subs->src_list);
INIT_LIST_HEAD(&subs->dest_list);
exclusive = !!(info->flags & SNDRV_SEQ_PORT_SUBS_EXCLUSIVE);
err = check_and_subscribe_port(src_client, src_port, subs, true,
exclusive,
connector->number != src_client->number);
if (err < 0)
goto error;
err = check_and_subscribe_port(dest_client, dest_port, subs, false,
exclusive,
connector->number != dest_client->number);
if (err < 0)
goto error_dest;
up_write(&dest->list_mutex);
up_write(&src->list_mutex);
return 0; return 0;
__error: error_dest:
if (src_called) delete_and_unsubscribe_port(src_client, src_port, subs, true,
unsubscribe_port(src_client, src_port, src, info, connector->number != src_client->number);
connector->number != src_client->number); error:
kfree(subs); kfree(subs);
up_write(&dest->list_mutex);
up_write(&src->list_mutex);
return err; return err;
} }
/* remove the connection */ /* remove the connection */
int snd_seq_port_disconnect(struct snd_seq_client *connector, int snd_seq_port_disconnect(struct snd_seq_client *connector,
struct snd_seq_client *src_client, struct snd_seq_client *src_client,
...@@ -572,37 +608,28 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector, ...@@ -572,37 +608,28 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector,
struct snd_seq_port_subscribe *info) struct snd_seq_port_subscribe *info)
{ {
struct snd_seq_port_subs_info *src = &src_port->c_src; 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; struct snd_seq_subscribers *subs;
int err = -ENOENT; int err = -ENOENT;
unsigned long flags;
down_write(&src->list_mutex); down_write(&src->list_mutex);
down_write_nested(&dest->list_mutex, SINGLE_DEPTH_NESTING);
/* look for the connection */ /* look for the connection */
list_for_each_entry(subs, &src->list_head, src_list) { list_for_each_entry(subs, &src->list_head, src_list) {
if (match_subs_info(info, &subs->info)) { if (match_subs_info(info, &subs->info)) {
write_lock_irqsave(&src->list_lock, flags); atomic_dec(&subs->ref_count); /* mark as not ready */
// write_lock(&dest->list_lock); // no lock yet
list_del(&subs->src_list);
list_del(&subs->dest_list);
// write_unlock(&dest->list_lock);
write_unlock_irqrestore(&src->list_lock, flags);
src->exclusive = dest->exclusive = 0;
unsubscribe_port(src_client, src_port, src, info,
connector->number != src_client->number);
unsubscribe_port(dest_client, dest_port, dest, info,
connector->number != dest_client->number);
kfree(subs);
err = 0; err = 0;
break; break;
} }
} }
up_write(&dest->list_mutex);
up_write(&src->list_mutex); up_write(&src->list_mutex);
return err; 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;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment