Skip to content

Commit f2764bd

Browse files
Florian Westphaldavem330
authored andcommitted
netlink: don't call ->netlink_bind with table lock held
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 1e82a62, "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 f773608 ("netlink: access nlk groups safely in netlink bind and getname") expanded the lock scope. commit 3a20773 ("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: 4d54cc3 ("mptcp: avoid lock_fast usage in accept path") Link: https://lore.kernel.org/mptcp/20210213000001.379332-8-mathew.j.martineau@linux.intel.com/T/#u 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>
1 parent b022654 commit f2764bd

1 file changed

Lines changed: 2 additions & 2 deletions

File tree

net/netlink/af_netlink.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1019,7 +1019,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
10191019
return -EINVAL;
10201020
}
10211021

1022-
netlink_lock_table();
10231022
if (nlk->netlink_bind && groups) {
10241023
int group;
10251024

@@ -1031,13 +1030,14 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
10311030
if (!err)
10321031
continue;
10331032
netlink_undo_bind(group, groups, sk);
1034-
goto unlock;
1033+
return err;
10351034
}
10361035
}
10371036

10381037
/* No need for barriers here as we return to user-space without
10391038
* using any of the bound attributes.
10401039
*/
1040+
netlink_lock_table();
10411041
if (!bound) {
10421042
err = nladdr->nl_pid ?
10431043
netlink_insert(sk, nladdr->nl_pid) :

0 commit comments

Comments
 (0)