From: Florian Westphal fw@strlen.de
commit f2764bd4f6a8dffaec3e220728385d9756b3c2cb upstream.
When I added support to allow generic netlink multicast groups to be restricted to subscribers with CAP_NET_ADMIN I was unaware that a genl_bind implementation already existed in the past.
It was reverted due to ABBA deadlock:
1. ->netlink_bind gets called with the table lock held. 2. genetlink bind callback is invoked, it grabs the genl lock.
But when a new genl subsystem is (un)registered, these two locks are taken in reverse order.
One solution would be to revert again and add a comment in genl referring 1e82a62fec613, "genetlink: remove genl_bind").
This would need a second change in mptcp to not expose the raw token value anymore, e.g. by hashing the token with a secret key so userspace can still associate subflow events with the correct mptcp connection.
However, Paolo Abeni reminded me to double-check why the netlink table is locked in the first place.
I can't find one. netlink_bind() is already called without this lock when userspace joins a group via NETLINK_ADD_MEMBERSHIP setsockopt. Same holds for the netlink_unbind operation.
Digging through the history, commit f773608026ee1 ("netlink: access nlk groups safely in netlink bind and getname") expanded the lock scope.
commit 3a20773beeeeade ("net: netlink: cap max groups which will be considered in netlink_bind()") ... removed the nlk->ngroups access that the lock scope extension was all about.
Reduce the lock scope again and always call ->netlink_bind without the table lock.
The Fixes tag should be vs. the patch mentioned in the link below, but that one got squash-merged into the patch that came earlier in the series.
Fixes: 4d54cc32112d8d ("mptcp: avoid lock_fast usage in accept path") Link: https://lore.kernel.org/mptcp/20210213000001.379332-8-mathew.j.martineau@lin... Cc: Cong Wang xiyou.wangcong@gmail.com Cc: Xin Long lucien.xin@gmail.com Cc: Johannes Berg johannes.berg@intel.com Cc: Sean Tranchetti stranche@codeaurora.org Cc: Paolo Abeni pabeni@redhat.com Cc: Pablo Neira Ayuso pablo@netfilter.org Signed-off-by: Florian Westphal fw@strlen.de Signed-off-by: David S. Miller davem@davemloft.net Signed-off-by: Ido Schimmel idosch@nvidia.com --- net/netlink/af_netlink.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 9737c3229c12..901358a5b593 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1021,7 +1021,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, return -EINVAL; }
- netlink_lock_table(); if (nlk->netlink_bind && groups) { int group;
@@ -1033,13 +1032,14 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, if (!err) continue; netlink_undo_bind(group, groups, sk); - goto unlock; + return err; } }
/* No need for barriers here as we return to user-space without * using any of the bound attributes. */ + netlink_lock_table(); if (!bound) { err = nladdr->nl_pid ? netlink_insert(sk, nladdr->nl_pid) :