Re: sound: deadlock between snd_rawmidi_kernel_open/snd_seq_port_connect

[Dmitry Vyukov]

On Wed, Feb 3, 2016 at 8:47 AM, Takashi Iwai <tiwai@xxxxxxx> wrote:
>> > This looks like a false-positive report to me.  Of course, we should
>> > annotate the mutex there for nested locks, though.
>>
>>
>> Takashi, can you please annotate it for lockdep? I hit it on every run.
>
> The lock had an annotation but alas it didn't seem enough.
> In anyway, it's not good to have double locks if it's avoidable.  So I
> worked on it now, and below is the current result of the hack.
>
> The change became a bit more intrusive than wished, but it should be
> still simple enough.  I put this on top of topic/core-fixes branch.


I don't see the deadlock reports now. Thanks!


> thanks,
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@xxxxxxx>
> Subject: [PATCH] ALSA: seq: Fix lockdep warnings due to double mutex locks
>
> 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.
>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
>  sound/core/seq/seq_clientmgr.c |   3 +
>  sound/core/seq/seq_ports.c     | 233 +++++++++++++++++++++++------------------
>  2 files changed, 133 insertions(+), 103 deletions(-)
>
> diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
> index 13cfa815732d..58e79e02f217 100644
> --- a/sound/core/seq/seq_clientmgr.c
> +++ b/sound/core/seq/seq_clientmgr.c
> @@ -678,6 +678,9 @@ static int deliver_to_subscribers(struct snd_seq_client *client,
>         else
>                 down_read(&grp->list_mutex);
>         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;
>                 if (subs->info.flags & SNDRV_SEQ_PORT_SUBS_TIMESTAMP)
>                         /* convert time according to flag with subscription */
> diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c
> index 55170a20ae72..921fb2bd8fad 100644
> --- a/sound/core/seq/seq_ports.c
> +++ b/sound/core/seq/seq_ports.c
> @@ -173,10 +173,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,
>                           struct snd_seq_client_port *port,
>                           struct snd_seq_port_subs_info *grp,
> @@ -203,6 +199,20 @@ static struct snd_seq_client_port *get_client_port(struct snd_seq_addr *addr,
>         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
>   * this is called from port_delete, for each src and dest list.
> @@ -210,7 +220,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,
>                                   struct snd_seq_client_port *port,
>                                   struct snd_seq_port_subs_info *grp,
> -                                 int grptype)
> +                                 int is_src)
>  {
>         struct list_head *p, *n;
>
> @@ -219,15 +229,13 @@ static void clear_subscriber_list(struct snd_seq_client *client,
>                 struct snd_seq_client *c;
>                 struct snd_seq_client_port *aport;
>
> -               if (grptype == SRC_LIST) {
> -                       subs = list_entry(p, struct snd_seq_subscribers, src_list);
> +               subs = get_subscriber(p, is_src);
> +               if (is_src)
>                         aport = get_client_port(&subs->info.dest, &c);
> -               } else {
> -                       subs = list_entry(p, struct snd_seq_subscribers, dest_list);
> +               else
>                         aport = get_client_port(&subs->info.sender, &c);
> -               }
> -               list_del(p);
> -               unsubscribe_port(client, port, grp, &subs->info, 0);
> +               delete_and_unsubscribe_port(client, port, subs, is_src, false);
> +
>                 if (!aport) {
>                         /* looks like the connected port is being deleted.
>                          * we decrease the counter, and when both ports are deleted
> @@ -235,21 +243,14 @@ static void clear_subscriber_list(struct snd_seq_client *client,
>                          */
>                         if (atomic_dec_and_test(&subs->ref_count))
>                                 kfree(subs);
> -               } else {
> -                       /* 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);
> +                       continue;
>                 }
> +
> +               /* 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);
>         }
>  }
>
> @@ -262,8 +263,8 @@ static int port_delete(struct snd_seq_client *client,
>         snd_use_lock_sync(&port->use_lock);
>
>         /* clear subscribers info */
> -       clear_subscriber_list(client, port, &port->c_src, SRC_LIST);
> -       clear_subscriber_list(client, port, &port->c_dest, DEST_LIST);
> +       clear_subscriber_list(client, port, &port->c_src, true);
> +       clear_subscriber_list(client, port, &port->c_dest, false);
>
>         if (port->private_free)
>                 port->private_free(port->private_data);
> @@ -479,85 +480,120 @@ static int match_subs_info(struct snd_seq_port_subscribe *r,
>         return 0;
>  }
>
> -
> -/* 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)
> +static int check_and_subscribe_port(struct snd_seq_client *client,
> +                                   struct snd_seq_client_port *port,
> +                                   struct snd_seq_subscribers *subs,
> +                                   bool is_src, bool exclusive, bool ack)
>  {
> -       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, *s;
> -       int err, src_called = 0;
> -       unsigned long flags;
> -       int exclusive;
> +       struct snd_seq_port_subs_info *grp;
> +       struct list_head *p;
> +       struct snd_seq_subscribers *s;
> +       int err;
>
> -       subs = kzalloc(sizeof(*subs), GFP_KERNEL);
> -       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;
> +       grp = is_src ? &port->c_src : &port->c_dest;
>         err = -EBUSY;
> +       down_write(&grp->list_mutex);
>         if (exclusive) {
> -               if (! list_empty(&src->list_head) || ! list_empty(&dest->list_head))
> +               if (!list_empty(&grp->list_head))
>                         goto __error;
>         } else {
> -               if (src->exclusive || dest->exclusive)
> +               if (grp->exclusive)
>                         goto __error;
>                 /* check whether already exists */
> -               list_for_each_entry(s, &src->list_head, src_list) {
> -                       if (match_subs_info(info, &s->info))
> -                               goto __error;
> -               }
> -               list_for_each_entry(s, &dest->list_head, dest_list) {
> -                       if (match_subs_info(info, &s->info))
> +               list_for_each(p, &grp->list_head) {
> +                       s = get_subscriber(p, is_src);
> +                       if (match_subs_info(&subs->info, &s->info))
>                                 goto __error;
>                 }
>         }
>
> -       if ((err = subscribe_port(src_client, src_port, src, info,
> -                                 connector->number != src_client->number)) < 0)
> -               goto __error;
> -       src_called = 1;
> -
> -       if ((err = subscribe_port(dest_client, dest_port, dest, info,
> -                                 connector->number != dest_client->number)) < 0)
> +       err = subscribe_port(client, port, grp, &subs->info, ack);
> +       if (err < 0) {
> +               grp->exclusive = 0;
>                 goto __error;
> +       }
>
>         /* add to list */
> -       write_lock_irqsave(&src->list_lock, flags);
> -       // write_lock(&dest->list_lock); // no other lock yet
> -       list_add_tail(&subs->src_list, &src->list_head);
> -       list_add_tail(&subs->dest_list, &dest->list_head);
> -       // write_unlock(&dest->list_lock); // no other lock yet
> -       write_unlock_irqrestore(&src->list_lock, flags);
> +       write_lock_irq(&grp->list_lock);
> +       if (is_src)
> +               list_add_tail(&subs->src_list, &grp->list_head);
> +       else
> +               list_add_tail(&subs->dest_list, &grp->list_head);
> +       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;
>
> - __error:
> -       if (src_called)
> -               unsubscribe_port(src_client, src_port, src, info,
> -                                connector->number != src_client->number);
> + error_dest:
> +       delete_and_unsubscribe_port(src_client, src_port, subs, true,
> +                                   connector->number != src_client->number);
> + error:
>         kfree(subs);
> -       up_write(&dest->list_mutex);
> -       up_write(&src->list_mutex);
>         return err;
>  }
>
> -
>  /* remove the connection */
>  int snd_seq_port_disconnect(struct snd_seq_client *connector,
>                             struct snd_seq_client *src_client,
> @@ -567,37 +603,28 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector,
>                             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;
> -       unsigned long flags;
>
>         down_write(&src->list_mutex);
> -       down_write_nested(&dest->list_mutex, SINGLE_DEPTH_NESTING);
> -
>         /* look for the connection */
>         list_for_each_entry(subs, &src->list_head, src_list) {
>                 if (match_subs_info(info, &subs->info)) {
> -                       write_lock_irqsave(&src->list_lock, flags);
> -                       // 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);
> +                       atomic_dec(&subs->ref_count); /* mark as not ready */
>                         err = 0;
>                         break;
>                 }
>         }
> -
> -       up_write(&dest->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;
>  }
>
>
> --
> 2.7.0
>