Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a8b9be9f authored by Sean Tranchetti's avatar Sean Tranchetti
Browse files

genetlink: remove genl_bind 



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.

Change-Id: Ic7fbc60896384d8fe2d46b56419f14493e5782b6
Fixes: c380d9a7 ("genetlink: pass multicast bind/unbind to families")
Suggested-by: default avatarCong Wang <xiyou.wangcong@gmail.com>
Acked-by: default avatarJohannes Berg <johannes.berg@intel.com>
Reported-by: default avatarkernel test robot <lkp@intel.com>
Git-repo: git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
Git-commit: 1e82a62fec613844da9e558f3493540a5b7a7b67
Signed-off-by: default avatarSean Tranchetti <stranche@codeaurora.org>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent c77acc02

include/net/genetlink.h

+0 −8
Original line number Original line Diff line number Diff line
@@ -34,12 +34,6 @@ struct genl_info; 
 *	do additional, common, filtering and return an error

 *	do additional, common, filtering and return an error

 * @post_doit: called after an operation's doit callback, it may

 * @post_doit: called after an operation's doit callback, it may

 *	undo operations done by pre_doit, for example release locks

 *	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 (private)

 * @attrbuf: buffer to store parsed attributes (private)

 * @mcgrps: multicast groups used by this family

 * @mcgrps: multicast groups used by this family

 * @n_mcgrps: number of multicast groups

 * @n_mcgrps: number of multicast groups
@@ -62,8 +56,6 @@ struct genl_family { 
	void			(*post_doit)(const struct genl_ops *ops,

	void			(*post_doit)(const struct genl_ops *ops,

					     struct sk_buff *skb,

					     struct sk_buff *skb,

					     struct genl_info *info);

					     struct genl_info *info);

	int			(*mcast_bind)(struct net *net, int group);

	void			(*mcast_unbind)(struct net *net, int group);

	struct nlattr **	attrbuf;	/* private */

	struct nlattr **	attrbuf;	/* private */

	const struct genl_ops *	ops;

	const struct genl_ops *	ops;

	const struct genl_multicast_group *mcgrps;

	const struct genl_multicast_group *mcgrps;

net/netlink/genetlink.c

+0 −49
Original line number Original line Diff line number Diff line
@@ -961,60 +961,11 @@ static struct genl_family genl_ctrl __ro_after_init = { 
	.netnsok = true,

	.netnsok = true,

};

};





static int genl_bind(struct net *net, int group)

{

	struct genl_family *f;

	int err = 0;

	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)

static int __net_init genl_pernet_init(struct net *net)

{

{

	struct netlink_kernel_cfg cfg = {

	struct netlink_kernel_cfg cfg = {

		.input		= genl_rcv,

		.input		= genl_rcv,

		.flags		= NL_CFG_F_NONROOT_RECV,

		.flags		= NL_CFG_F_NONROOT_RECV,

		.bind		= genl_bind,

		.unbind		= genl_unbind,

	};

	};





	/* we'll bump the group number right afterwards */

	/* we'll bump the group number right afterwards */