On Fri, Jun 26, 2020 at 5:32 PM Sean Tranchetti <[email protected]> wrote:
>
> 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.
>
> To avoid this scenario, the locks should be acquired in the same order by
> both threads. Since both the register and unregister functions need to take
> the nl_table_lock in their processing, it makes sense to explicitly acquire
> them before they lock the genl_mutex and the cb_lock. In unregistering, no
> other change is needed aside from this locking change.
Like the kernel test robot reported, you can not call genl_lock_all while
holding netlink_table_grab() which is effectively a write lock.
To me, it seems genl_bind() can be just removed as there is no one
in-tree uses family->mcast_bind(). Can you test the attached patch?
It seems sufficient to fix this deadlock.
Thanks.
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index ad71ed4f55ff..6e5f1e1aa822 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -35,12 +35,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.
* @mcgrps: multicast groups used by this family
* @n_mcgrps: number of multicast groups
* @mcgrp_offset: starting number of multicast group IDs in this family
@@ -63,8 +57,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);
const struct genl_ops * ops;
const struct genl_multicast_group *mcgrps;
unsigned int n_ops;
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index a914b9365a46..9395ee8a868d 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1144,60 +1144,11 @@ static struct genl_family genl_ctrl __ro_after_init = {
.netnsok = true,
};
-static int genl_bind(struct net *net, int group)
-{
- struct genl_family *f;
- int err = -ENOENT;
- unsigned int id;
-
- down_read(&cb_lock);
-
- idr_for_each_entry(&genl_fam_idr, f, id) {
- 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)
-{
- struct genl_family *f;
- unsigned int id;
-
- down_read(&cb_lock);
-
- idr_for_each_entry(&genl_fam_idr, f, id) {
- 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 */
