| From foo@baz Fri 17 Jul 2020 11:04:44 AM CEST |
| From: Sean Tranchetti <stranche@codeaurora.org> |
| Date: Tue, 30 Jun 2020 11:50:17 -0600 |
| Subject: genetlink: remove genl_bind |
| |
| From: Sean Tranchetti <stranche@codeaurora.org> |
| |
| [ Upstream commit 1e82a62fec613844da9e558f3493540a5b7a7b67 ] |
| |
| A potential deadlock can occur during registering or unregistering a |
| new generic netlink family between the main nl_table_lock and the |
| cb_lock where each thread wants the lock held by the other, as |
| demonstrated below. |
| |
| 1) Thread 1 is performing a netlink_bind() operation on a socket. As part |
| of this call, it will call netlink_lock_table(), incrementing the |
| nl_table_users count to 1. |
| 2) Thread 2 is registering (or unregistering) a genl_family via the |
| genl_(un)register_family() API. The cb_lock semaphore will be taken for |
| writing. |
| 3) Thread 1 will call genl_bind() as part of the bind operation to handle |
| subscribing to GENL multicast groups at the request of the user. It will |
| attempt to take the cb_lock semaphore for reading, but it will fail and |
| be scheduled away, waiting for Thread 2 to finish the write. |
| 4) Thread 2 will call netlink_table_grab() during the (un)registration |
| call. However, as Thread 1 has incremented nl_table_users, it will not |
| be able to proceed, and both threads will be stuck waiting for the |
| other. |
| |
| genl_bind() is a noop, unless a genl_family implements the mcast_bind() |
| function to handle setting up family-specific multicast operations. Since |
| no one in-tree uses this functionality as Cong pointed out, simply removing |
| the genl_bind() function will remove the possibility for deadlock, as there |
| is no attempt by Thread 1 above to take the cb_lock semaphore. |
| |
| Fixes: c380d9a7afff ("genetlink: pass multicast bind/unbind to families") |
| Suggested-by: Cong Wang <xiyou.wangcong@gmail.com> |
| Acked-by: Johannes Berg <johannes.berg@intel.com> |
| Reported-by: kernel test robot <lkp@intel.com> |
| Signed-off-by: Sean Tranchetti <stranche@codeaurora.org> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| include/net/genetlink.h | 8 ------- |
| net/netlink/genetlink.c | 52 ------------------------------------------------ |
| 2 files changed, 60 deletions(-) |
| |
| --- a/include/net/genetlink.h |
| +++ b/include/net/genetlink.h |
| @@ -33,12 +33,6 @@ struct genl_info; |
| * do additional, common, filtering and return an error |
| * @post_doit: called after an operation's doit callback, it may |
| * undo operations done by pre_doit, for example release locks |
| - * @mcast_bind: a socket bound to the given multicast group (which |
| - * is given as the offset into the groups array) |
| - * @mcast_unbind: a socket was unbound from the given multicast group. |
| - * Note that unbind() will not be called symmetrically if the |
| - * generic netlink family is removed while there are still open |
| - * sockets. |
| * @attrbuf: buffer to store parsed attributes |
| * @family_list: family list |
| * @mcgrps: multicast groups used by this family (private) |
| @@ -61,8 +55,6 @@ struct genl_family { |
| void (*post_doit)(const struct genl_ops *ops, |
| struct sk_buff *skb, |
| struct genl_info *info); |
| - int (*mcast_bind)(struct net *net, int group); |
| - void (*mcast_unbind)(struct net *net, int group); |
| struct nlattr ** attrbuf; /* private */ |
| const struct genl_ops * ops; /* private */ |
| const struct genl_multicast_group *mcgrps; /* private */ |
| --- a/net/netlink/genetlink.c |
| +++ b/net/netlink/genetlink.c |
| @@ -992,63 +992,11 @@ static const struct genl_multicast_group |
| { .name = "notify", }, |
| }; |
| |
| -static int genl_bind(struct net *net, int group) |
| -{ |
| - int i, err = -ENOENT; |
| - |
| - down_read(&cb_lock); |
| - for (i = 0; i < GENL_FAM_TAB_SIZE; i++) { |
| - struct genl_family *f; |
| - |
| - list_for_each_entry(f, genl_family_chain(i), family_list) { |
| - if (group >= f->mcgrp_offset && |
| - group < f->mcgrp_offset + f->n_mcgrps) { |
| - int fam_grp = group - f->mcgrp_offset; |
| - |
| - if (!f->netnsok && net != &init_net) |
| - err = -ENOENT; |
| - else if (f->mcast_bind) |
| - err = f->mcast_bind(net, fam_grp); |
| - else |
| - err = 0; |
| - break; |
| - } |
| - } |
| - } |
| - up_read(&cb_lock); |
| - |
| - return err; |
| -} |
| - |
| -static void genl_unbind(struct net *net, int group) |
| -{ |
| - int i; |
| - |
| - down_read(&cb_lock); |
| - for (i = 0; i < GENL_FAM_TAB_SIZE; i++) { |
| - struct genl_family *f; |
| - |
| - list_for_each_entry(f, genl_family_chain(i), family_list) { |
| - if (group >= f->mcgrp_offset && |
| - group < f->mcgrp_offset + f->n_mcgrps) { |
| - int fam_grp = group - f->mcgrp_offset; |
| - |
| - if (f->mcast_unbind) |
| - f->mcast_unbind(net, fam_grp); |
| - break; |
| - } |
| - } |
| - } |
| - up_read(&cb_lock); |
| -} |
| - |
| static int __net_init genl_pernet_init(struct net *net) |
| { |
| struct netlink_kernel_cfg cfg = { |
| .input = genl_rcv, |
| .flags = NL_CFG_F_NONROOT_RECV, |
| - .bind = genl_bind, |
| - .unbind = genl_unbind, |
| }; |
| |
| /* we'll bump the group number right afterwards */ |