[PATCH net v4 0/2] genetlink: fix joining multicast groups no family owns

[PATCH net v4 0/2] genetlink: fix joining multicast groups no family owns

[Anil Kaushik]

genl_bind() checks the GENL_MCAST_CAP_* flags of a group, but the
membership bit is set later, after the lock is dropped. A socket can end
up a member of a group whose flags were never checked for it: when it
joins a group ID that no family owns, or when the family of the group is
unregistered between the check and the point where the bit is set.

Patch 1 rejects joins to group IDs that no family owns. Patch 2 counts,
per group, how many times its members were cleared, which genetlink does
when a family is unregistered. If the count changed after the check, the
group is checked again against its current owner, after the bind
callback is undone if that owner is the one it reached. The core cannot
always tell, see patch 2.

Tested as an unprivileged user, with an msleep() added after the bind
callback in netlink_setsockopt() and netlink_bind(), and a test family
that registers again with GENL_MCAST_CAP_NET_ADMIN:

                                         patch 1 only   v4
  setsockopt:
  unregister in the window,
  register later                         member         not member
  unregister and register in the window  member         not member
  another family unregistered
  (must still join)                      member         member
  bind():
  unregister and register in the window  member         not member

With the msleep() before the bind callback instead, and a test family
that registers again without the flag, the family is bound once, and not
twice as with v3:

                                         v3             v4
  binds while a member, setsockopt/bind  2              1
  binds once the socket is closed        1              0

The test family counts its bind and unbind callbacks: they stay
balanced in all these cases. If the family is replaced just after the
bind callback returned, and before the second read, with an msleep()
there, it gets one unbind too many. The socket is still rejected or
checked against the new family. Normal joins and rtnetlink bind() show
no change.

The test from v1 (join all free IDs, then load gtp and drop_monitor)
gives the same result as before.

I used an LLM to help write and review the code, the tests and the
commit messages of this series. I checked and tested every change
myself.

Changes in v4:
- Patch 2: read the count after ->netlink_bind() too, and call
  ->netlink_unbind() before checking again if the count had already
  changed: ->netlink_bind() can have reached the new owner, which was
  then bound twice (netdev Sashiko). The commit message says what the
  core still cannot tell.
- Patch 1: no change. It does not close the window on its own, patch 2
  does, and it keeps its tags (netdev Sashiko asked about the order).
- The deadlock netdev Sashiko found on the insert failure path of
  netlink_bind() exists without this series, it has its own patch:
  https://lore.kernel.org/netdev/20261102120929.4077030-1-anilkaushikwireless@gmail.com/

Changes in v3:
- Patch 2: check the group again in af_netlink.c instead of clearing
  the members when a family registers. This also covers the case where
  the family is unregistered and another one registers, both in the
  window, does not walk the sockets on every registration, and does not
  touch the members of a group another family still owns (Sashiko).
- Fixes: tag of both patches: the race exists since genl_bind() first
  checked a capability, in 4d54cc32112d (Sashiko).

Changes in v2:
- New patch 2 (Sashiko).
- Patch 1: no change, added Reviewed-by tags.

v3: https://lore.kernel.org/netdev/20261101101347.3910654-1-anilkaushikwireless@gmail.com/
v2: https://lore.kernel.org/netdev/20261031054632.3697295-1-anilkaushikwireless@gmail.com/
v1: https://lore.kernel.org/netdev/20261029062214.3407223-1-anilkaushikwireless@gmail.com/

Anil Kaushik (2):
  genetlink: Reject joining multicast groups no family owns
  netlink: Check a group again if it was released while joining it

 net/netlink/af_netlink.c | 79 +++++++++++++++++++++++++++++++++++++++-
 net/netlink/af_netlink.h |  1 +
 net/netlink/genetlink.c  |  3 +-
 3 files changed, 81 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH net v4 1/2] genetlink: Reject joining multicast groups no family owns

[Anil Kaushik]

genl_bind() checks the GENL_MCAST_CAP_* flags of a group only when a
family owns it. For a group ID that no family owns, it returns 0, so any
user can join it.

A family registered later can get that ID, and the socket stays a member:
nothing checks it again. An unprivileged user can join the free IDs, load
a family module with CTRL_CMD_GETFAMILY, which calls request_module(),
and then receive the notifications of a group with
GENL_MCAST_CAP_NET_ADMIN or GENL_MCAST_CAP_SYS_ADMIN. For example the
drop_monitor "events" group always gets ID 1, and is free until the
drop_monitor module is loaded.

Fix by returning -ENOENT from genl_bind() when no family owns the group.
Nothing is ever sent to such a group, so joining it has no use.

Found by the Sashiko review of the gtp multicast group fix. Tested as an
unprivileged user who joins all free IDs, then loads gtp and
drop_monitor:

Before:
  joins: ok=62 EPERM=2
  gtp/gtp (ID 15): member
  NET_DM/events (ID 1): member

After:
  joins: ok=19 ENOENT=43 EPERM=2
  gtp/gtp (ID 15): not member
  NET_DM/events (ID 1): not member

genl_bind() was added, with its first capability check, in the commit
in the Fixes tag.

Link: https://lore.kernel.org/netdev/179324692107.3134.14443689892607921337@kernel.org/
Fixes: 4d54cc32112d ("mptcp: avoid lock_fast usage in accept path")
Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
---
 net/netlink/genetlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 5cc1037d4917..f038f04ef4ea 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1825,7 +1825,7 @@ static int genl_bind(struct net *net, int group)
 {
 	const struct genl_family *family;
 	unsigned int id;
-	int ret = 0;
+	int ret = -ENOENT;
 
 	down_read(&cb_lock);
 
@@ -1841,6 +1841,7 @@ static int genl_bind(struct net *net, int group)
 			continue;
 
 		grp = &family->mcgrps[i];
+		ret = 0;
 		if ((grp->flags & GENL_MCAST_CAP_NET_ADMIN) &&
 		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
 			ret = -EPERM;
-- 
2.25.1

Re: [PATCH net v4 1/2] genetlink: Reject joining multicast groups no family owns

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In net/netlink/af_netlink.c, netlink_bind() lets one failing
  group abort the whole bind().

Pre-existing issues:
- [Medium] Kept only because requires_validation=true.

--

genetlink: Reject joining multicast groups no family owns

genl_bind() now returns -ENOENT when no registered family owns the
requested multicast group ID. Before this change such joins succeeded.
The socket also stayed a member if a family with privileged-only groups
later got the same ID.

Link: https://lore.kernel.org/netdev/179324692107.3134.14443689892607921337@kernel.org/

> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 5cc1037d4917e..f038f04ef4ead 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1825,7 +1825,7 @@ static int genl_bind(struct net *net, int group)
>  {
>  	const struct genl_family *family;
>  	unsigned int id;
> -	int ret = 0;
> +	int ret = -ENOENT;

[Severity: Medium]
Can a bind() with an nl_groups mask now fail as a whole when only one
bit in the mask has no owner?

netlink_bind() in net/netlink/af_netlink.c calls the bind hook once for
each set bit. On the first error it undoes the groups it already added
and returns:

net/netlink/af_netlink.c:netlink_bind() {
    ...
		for (group = 0; group < BITS_PER_TYPE(u32); group++) {
			if (!test_bit(group, &groups))
				continue;
			err = nlk->netlink_bind(net, group + 1);
			if (!err)
				continue;
			netlink_undo_bind(group, groups, sk);
			return err;
		}
    ...
}

Generic netlink group IDs are sparse. mc_group_start reserves only a few
bits:

static unsigned long mc_group_start = 0x3 | BIT(GENL_ID_CTRL) |
				      BIT(GENL_ID_VFS_DQUOT) |
				      BIT(GENL_ID_PMCRAID);

Other IDs are given out only when a family registers. So on a typical
system most IDs in 1..32 have no owner. genl's nlk->ngroups is at least
32, so the masking at the top of netlink_bind() does not remove these
bits.

Before this patch, a bind() on a NETLINK_GENERIC socket with a mask that
mixed owned and unowned IDs (for example nl_groups = ~0) returned 0 and
joined the owned groups.

With this patch, will that call return -ENOENT and join none of the
requested groups, including the valid owned ones?

The netlink core also still treats every ID below ngroups as valid in
netlink_setsockopt():

		if (!val || val - 1 >= nlk->ngroups)
			return -EINVAL;

genl_bind() now rejects many of those IDs, so the two layers disagree.

The commit message argues that joining an unowned group has no use, and
its test counts single-group joins. It does not mention the bind() mask
case.

The later commit "netlink: Check a group again if it was released while
joining it" keeps both the -ENOENT return and the all-or-nothing loop.
So this behaviour change is still there at the end of the series.

Should the bind() mask path skip unowned IDs instead of failing? If not,
should the commit message mention this uAPI change?

[ ... ]

> @@ -1841,6 +1841,7 @@ static int genl_bind(struct net *net, int group)
>  			continue;
>  
>  		grp = &family->mcgrps[i];
> +		ret = 0;
>  		if ((grp->flags & GENL_MCAST_CAP_NET_ADMIN) &&
>  		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
>  			ret = -EPERM;

[Severity: Medium]
This isn't a bug introduced by this patch, but is there still a window
where a socket can become a member of a group ID that was just freed?

genl_bind() checks ownership and the CAP flags under down_read(&cb_lock).
It drops cb_lock before returning. The caller adds the membership later,
under netlink_table_grab():

net/netlink/af_netlink.c:netlink_setsockopt() {
    ...
		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
			err = nlk->netlink_bind(sock_net(sk), val);
			if (err)
				return err;
		}
		netlink_table_grab();
		netlink_update_socket_mc(nlk, val,
					 optname == NETLINK_ADD_MEMBERSHIP);
    ...
}

netlink_bind() splits the check and the update in the same way for the
nl_groups mask.

Suppose genl_unregister_family() runs in that gap:

CPU1                                  CPU2
setsockopt(NETLINK_ADD_MEMBERSHIP)
  genl_bind() returns 0
  (cb_lock dropped)
                                      genl_unregister_family()
                                        genl_unregister_mc_groups()
                                          __netlink_clear_multicast_users()
                                          clear_bit(grp_id, mc_groups)
  netlink_table_grab()
  netlink_update_socket_mc()

__netlink_clear_multicast_users() clears only the sockets that are
members at that moment.

Would the socket on CPU1 then be added to the now-free ID?

A family registered later could get that ID, including one whose groups
have GENL_MCAST_CAP_NET_ADMIN or GENL_MCAST_CAP_SYS_ADMIN. It would then
deliver to the socket through do_one_broadcast(), and nothing checks the
membership again.

The commit message says:

  "A family registered later can get that ID, and the socket stays a
  member: nothing checks it again."

This patch alone does not close that window. The window is narrow,
because a family has to be unregistered during it, for example by rmmod
or by a module init that fails after registration.

The next commit in the series, "netlink: Check a group again if it was
released while joining it", appears to fix this:

- It adds a per-group generation counter, nl_table[].mc_gen, which
  __netlink_clear_multicast_users() increments.
- netlink_setsockopt() and netlink_bind() compare the counter again
  under netlink_table_grab() and retry if the group was released.
- On retry, genl_bind() returns -ENOENT for the freed ID.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102155819.4100930-1-anilkaushikwireless%40gmail.com

Re: [PATCH net v4 1/2] genetlink: Reject joining multicast groups no family owns

[Anil Kaushik]

Yes, this is a behaviour change, and I missed it in the commit message.
A bind() mask with an unowned ID now fails with -ENOENT and joins
nothing. I don't know of a program that does this.

Ido, Nicolas, is this OK to keep, or should bind() skip unowned IDs?
I can send a new version if needed.

The other issue, the window before the membership is added, is closed
by patch 2.

Anil

Re: [PATCH net v4 1/2] genetlink: Reject joining multicast groups no family owns

[Nicolas Dichtel]

Le 03/11/2026 à 17:12, Anil Kaushik a écrit :
> Yes, this is a behaviour change, and I missed it in the commit message.
> A bind() mask with an unowned ID now fails with -ENOENT and joins
> nothing. I don't know of a program that does this.
> 
> Ido, Nicolas, is this OK to keep, or should bind() skip unowned IDs?
> I can send a new version if needed.
It's ok for me, but if a regression occurs, a fix will be needed.
I let others speak.

Nicolas

Re: [PATCH net v4 1/2] genetlink: Reject joining multicast groups no family owns

[Jacob Keller]

On 11/3/2026 9:54 AM, Nicolas Dichtel wrote:
> Le 03/11/2026 à 17:12, Anil Kaushik a écrit :
>> Yes, this is a behaviour change, and I missed it in the commit message.
>> A bind() mask with an unowned ID now fails with -ENOENT and joins
>> nothing. I don't know of a program that does this.
>>
>> Ido, Nicolas, is this OK to keep, or should bind() skip unowned IDs?
>> I can send a new version if needed.
> It's ok for me, but if a regression occurs, a fix will be needed.
> I let others speak.
> 
> Nicolas

This sounds like it might fall under the "no breaking user space" even
if its quite silly... But I am not an expert here so I am not sure.

Thanks,
Jake

[PATCH net v4 2/2] netlink: Check a group again if it was released while joining it

[Anil Kaushik]

genl_bind() checks a group under cb_lock, but netlink_setsockopt() and
netlink_bind() set the membership bit only after it returns and the lock
is dropped. If the family is unregistered in between, its members are
cleared before this socket is one. The socket then becomes a member of a
group ID that no family owns, or that a family registered in the
meantime got, and the GENL_MCAST_CAP_* flags of that family are never
checked for it.

Count per group in the netlink table how many times its members were
cleared, which genetlink does when it frees the group IDs of a family.
Read the count before and after ->netlink_bind(), and again once the
table is grabbed. If it changed since the first read, the group was
released, so check it again against its current owner.

If the count changed only after ->netlink_bind() returned, the family
that was bound is gone, so ->netlink_unbind() is not called for it.
Otherwise it is called before the new check, as ->netlink_bind() most
likely reached the new owner. The core cannot tell for sure: if a family
is unregistered and another one registers just after ->netlink_bind()
returned, and before the second read, the new family gets an unbind
without a bind. The check is still done again. In the tree, only the
thermal family has these callbacks, and it is never unregistered.

bind() can then fail after it gave the socket its port ID. This only
happens when a family is unregistered meanwhile.

Found by the Sashiko review of the previous patch, not seen in
production. Triggered in a VM as an unprivileged user, with a test-only
msleep() after the bind callback, and a test family that registers again
with GENL_MCAST_CAP_NET_ADMIN:

Before:
  setsockopt() and bind(): the socket is a member of the group

After:
  setsockopt() and bind(): -EPERM, not a member
  setsockopt(), no family owns the group yet: -ENOENT, not a member

genl_bind() was added, with its first capability check, in the commit
in the Fixes tag.

Link: https://lore.kernel.org/netdev/179340003139.3131.11274717768928642724@kernel.org/
Fixes: 4d54cc32112d ("mptcp: avoid lock_fast usage in accept path")
Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
---
 net/netlink/af_netlink.c | 79 +++++++++++++++++++++++++++++++++++++++-
 net/netlink/af_netlink.h |  1 +
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index fec2879c497d..96bfebc57899 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -780,6 +780,8 @@ static int netlink_release(struct socket *sock)
 			old = nl_deref_protected(nl_table[sk->sk_protocol].listeners);
 			RCU_INIT_POINTER(nl_table[sk->sk_protocol].listeners, NULL);
 			kfree_rcu(old, rcu);
+			kfree(nl_table[sk->sk_protocol].mc_gen);
+			nl_table[sk->sk_protocol].mc_gen = NULL;
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
 			nl_table[sk->sk_protocol].unbind = NULL;
@@ -990,6 +992,9 @@ static int netlink_bind(struct socket *sock, struct sockaddr_unsized *addr,
 	struct net *net = sock_net(sk);
 	struct netlink_sock *nlk = nlk_sk(sk);
 	struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
+	struct netlink_table *tbl = &nl_table[sk->sk_protocol];
+	unsigned int bound_gen[BITS_PER_TYPE(u32)];
+	unsigned int gen[BITS_PER_TYPE(u32)];
 	int err = 0;
 	unsigned long groups;
 	bool bound;
@@ -1023,9 +1028,14 @@ static int netlink_bind(struct socket *sock, struct sockaddr_unsized *addr,
 			return -EINVAL;
 	}
 
+retry:
 	if (nlk->netlink_bind && groups) {
 		int group;
 
+		netlink_lock_table();
+		memcpy(gen, tbl->mc_gen, sizeof(gen));
+		netlink_unlock_table();
+
 		/* nl_groups is a u32, so cap the maximum groups we can bind */
 		for (group = 0; group < BITS_PER_TYPE(u32); group++) {
 			if (!test_bit(group, &groups))
@@ -1036,6 +1046,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr_unsized *addr,
 			netlink_undo_bind(group, groups, sk);
 			return err;
 		}
+
+		netlink_lock_table();
+		memcpy(bound_gen, tbl->mc_gen, sizeof(bound_gen));
+		netlink_unlock_table();
 	}
 
 	/* No need for barriers here as we return to user-space without
@@ -1057,6 +1071,26 @@ static int netlink_bind(struct socket *sock, struct sockaddr_unsized *addr,
 	netlink_unlock_table();
 
 	netlink_table_grab();
+	if (nlk->netlink_bind && groups) {
+		unsigned long gone = 0;
+		bool released = false;
+		int group;
+
+		for_each_set_bit(group, &groups, BITS_PER_TYPE(u32)) {
+			if (gen[group] != tbl->mc_gen[group])
+				released = true;
+			if (bound_gen[group] != tbl->mc_gen[group])
+				__set_bit(group, &gone);
+		}
+		/* see netlink_setsockopt(), undo all but gone and retry all */
+		if (released) {
+			netlink_table_ungrab();
+			netlink_undo_bind(BITS_PER_TYPE(u32), groups & ~gone,
+					  sk);
+			bound = true;
+			goto retry;
+		}
+	}
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(groups) -
 					 hweight32(nlk->groups[0]));
@@ -1686,6 +1720,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		break;
 	case NETLINK_ADD_MEMBERSHIP:
 	case NETLINK_DROP_MEMBERSHIP: {
+		struct netlink_table *tbl = &nl_table[sk->sk_protocol];
+		unsigned int gen = 0, bound_gen = 0;
 		int err;
 
 		if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV))
@@ -1695,12 +1731,34 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+retry:
 		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
+			netlink_lock_table();
+			gen = tbl->mc_gen[val - 1];
+			netlink_unlock_table();
 			err = nlk->netlink_bind(sock_net(sk), val);
 			if (err)
 				return err;
+			netlink_lock_table();
+			bound_gen = tbl->mc_gen[val - 1];
+			netlink_unlock_table();
 		}
 		netlink_table_grab();
+		/* The group was released around ->netlink_bind(), and may have
+		 * a new owner now, so check it again. If it was released only
+		 * after ->netlink_bind() returned, the owner that was bound is
+		 * gone: no ->netlink_unbind() for it. Otherwise assume that the
+		 * current owner was bound.
+		 */
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind &&
+		    gen != tbl->mc_gen[val - 1]) {
+			bool unbind = bound_gen == tbl->mc_gen[val - 1];
+
+			netlink_table_ungrab();
+			if (unbind && nlk->netlink_unbind)
+				nlk->netlink_unbind(sock_net(sk), val);
+			goto retry;
+		}
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
@@ -2031,6 +2089,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 	struct sock *sk;
 	struct netlink_sock *nlk;
 	struct listeners *listeners = NULL;
+	unsigned int *mc_gen = NULL;
 	unsigned int groups;
 
 	BUG_ON(!nl_table);
@@ -2052,7 +2111,8 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 		groups = cfg->groups;
 
 	listeners = kzalloc(sizeof(*listeners) + NLGRPSZ(groups), GFP_KERNEL);
-	if (!listeners)
+	mc_gen = kcalloc(groups, sizeof(*mc_gen), GFP_KERNEL);
+	if (!listeners || !mc_gen)
 		goto out_sock_release;
 
 	sk->sk_data_ready = netlink_data_ready;
@@ -2069,6 +2129,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 	if (!nl_table[unit].registered) {
 		nl_table[unit].groups = groups;
 		rcu_assign_pointer(nl_table[unit].listeners, listeners);
+		nl_table[unit].mc_gen = mc_gen;
 		nl_table[unit].module = module;
 		if (cfg) {
 			nl_table[unit].bind = cfg->bind;
@@ -2079,6 +2140,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 		nl_table[unit].registered = 1;
 	} else {
 		kfree(listeners);
+		kfree(mc_gen);
 		nl_table[unit].registered++;
 	}
 	netlink_table_ungrab();
@@ -2086,6 +2148,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 
 out_sock_release:
 	kfree(listeners);
+	kfree(mc_gen);
 	netlink_kernel_release(sk);
 	return NULL;
 
@@ -2113,6 +2176,17 @@ int __netlink_change_ngroups(struct sock *sk, unsigned int groups)
 	if (groups < 32)
 		groups = 32;
 
+	if (tbl->groups < groups) {
+		unsigned int *mc_gen;
+
+		mc_gen = kcalloc(groups, sizeof(*mc_gen), GFP_ATOMIC);
+		if (!mc_gen)
+			return -ENOMEM;
+		memcpy(mc_gen, tbl->mc_gen, tbl->groups * sizeof(*mc_gen));
+		kfree(tbl->mc_gen);
+		tbl->mc_gen = mc_gen;
+	}
+
 	if (NLGRPSZ(tbl->groups) < NLGRPSZ(groups)) {
 		new = kzalloc(sizeof(*new) + NLGRPSZ(groups), GFP_ATOMIC);
 		if (!new)
@@ -2157,6 +2231,9 @@ void __netlink_clear_multicast_users(struct sock *ksk, unsigned int group)
 	struct netlink_table *tbl = &nl_table[ksk->sk_protocol];
 	struct hlist_node *tmp;
 
+	/* the group can be given to another user, see netlink_setsockopt() */
+	tbl->mc_gen[group - 1]++;
+
 	sk_for_each_bound_safe(sk, tmp, &tbl->mc_list) {
 		/* bitmap predates the group, can't be a member */
 		if (group - 1 >= nlk_sk(sk)->ngroups)
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 778a3809361f..d5e2e404fd11 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -64,6 +64,7 @@ struct netlink_table {
 	struct listeners __rcu	*listeners;
 	unsigned int		flags;
 	unsigned int		groups;
+	unsigned int		*mc_gen;
 	struct mutex		*cb_mutex;
 	struct module		*module;
 	int			(*bind)(struct net *net, int group);
-- 
2.25.1

Re: [PATCH net v4 2/2] netlink: Check a group again if it was released while joining it

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] netlink_add_usersock_entry() (net/netlink/af_netlink.c:2962) sets
  up nl_table[NETLINK_USERSOCK]: groups=32, listeners, module,…
- [Low] The kernel-doc of netlink_change_ngroups() says the group count
  can be reduced: 'it does not implicitly clear listeners from groups…
- [Low] netlink_setsockopt() (lines 1753-1760) and netlink_bind() (lines
  1079-1091) decide whether to call ->netlink_unbind() by comparing…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  deadlock in the code being changed.

--

netlink: Check a group again if it was released while joining it

This patch adds a per-group generation counter (mc_gen) to the netlink
table. __netlink_clear_multicast_users() bumps it when genetlink releases
the group IDs of a family. netlink_bind() and netlink_setsockopt() read
the counter around ->netlink_bind() and again under netlink_table_grab(),
and redo the bind check if the group was released in between.

Link: https://lore.kernel.org/netdev/179340003139.3131.11274717768928642724@kernel.org/

> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 1281c42a3456a..75efc0cfb8052 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c

[ ... ]

> @@ -1036,6 +1046,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr_unsized *addr,
>  			netlink_undo_bind(group, groups, sk);
>  			return err;
>  		}
> +
> +		netlink_lock_table();
> +		memcpy(bound_gen, tbl->mc_gen, sizeof(bound_gen));
> +		netlink_unlock_table();
>  	}
>  
>  	/* No need for barriers here as we return to user-space without

[Severity: High]
This isn't a bug introduced by this patch, but can the port ID failure
path right after this comment deadlock against genetlink family
registration or unregistration?

net/netlink/af_netlink.c:netlink_bind() {
    ...
	netlink_lock_table();
	if (!bound) {
		err = nladdr->nl_pid ?
			netlink_insert(sk, nladdr->nl_pid) :
			netlink_autobind(sock);
		if (err) {
			netlink_undo_bind(BITS_PER_TYPE(u32), groups, sk);
			goto unlock;
		}
	}
    ...
}

netlink_undo_bind() runs while this thread still holds an nl_table_users
reference. For NETLINK_GENERIC, nlk->netlink_unbind is genl_unbind(),
which always does down_read(&cb_lock).

genl_register_family() and genl_unregister_family() take cb_lock for
writing through genl_lock_all(). They then call netlink_table_grab() from
genl_validate_assign_mc_groups() or genl_unregister_mc_groups(), and it
sleeps in TASK_UNINTERRUPTIBLE until nl_table_users reaches 0:

CPU1: genl_unregister_family()
  genl_lock_all()                  /* down_write(&cb_lock) */
CPU2: netlink_bind()
  netlink_lock_table()             /* nl_table_users = 1 */
CPU1: genl_unregister_mc_groups()
  netlink_table_grab()             /* waits for nl_table_users == 0 */
CPU2: netlink_insert() fails with -EADDRINUSE
  netlink_undo_bind()
    genl_unbind()
      down_read(&cb_lock)          /* blocks behind CPU1 */

Neither side can make progress. cb_lock stays write-held, which also
blocks genl_rcv().

An unprivileged user can get to the bind() side. It is enough to bind a
NETLINK_GENERIC socket to a group that has no GENL_MCAST_CAP_* flags,
such as the nlctrl notify group, with an nl_pid that is already in use.

The writer side can be a module unload. It can also be a family
registration triggered through request_module() from CTRL_CMD_GETFAMILY.

The comment in netlink_release() warns about the same lock ordering:

	/* must not acquire netlink_table_lock in any way again before unbind
	 * and notifying genetlink is done as otherwise it might deadlock
	 */

The new retry paths in this patch call netlink_table_ungrab() before
calling the unbind callbacks. Could this error path also call
netlink_undo_bind() only after netlink_unlock_table()?

[ ... ]

> @@ -1695,12 +1731,34 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
>  			return err;
>  		if (!val || val - 1 >= nlk->ngroups)
>  			return -EINVAL;
> +retry:
>  		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
> +			netlink_lock_table();
> +			gen = tbl->mc_gen[val - 1];
> +			netlink_unlock_table();
>  			err = nlk->netlink_bind(sock_net(sk), val);
>  			if (err)
>  				return err;
> +			netlink_lock_table();
> +			bound_gen = tbl->mc_gen[val - 1];
> +			netlink_unlock_table();
>  		}
>  		netlink_table_grab();

[ ... ]

> +		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind &&
> +		    gen != tbl->mc_gen[val - 1]) {
> +			bool unbind = bound_gen == tbl->mc_gen[val - 1];
> +
> +			netlink_table_ungrab();
> +			if (unbind && nlk->netlink_unbind)
> +				nlk->netlink_unbind(sock_net(sk), val);
> +			goto retry;
> +		}

[Severity: Low]
Can this call ->netlink_unbind() on a family whose ->netlink_bind() never
ran for this socket?

bound_gen is read after genl_bind() has dropped cb_lock. Consider this
order of events, all before the bound_gen read:

- family A is bound
- A is unregistered, and
  genl_unregister_mc_groups()->__netlink_clear_multicast_users() bumps
  mc_gen
- family B registers and gets the same group ID

In that case gen differs from the current value, but bound_gen matches
it.

genl_unbind() looks up the current owner of the group, so B->unbind() is
called. The retry then calls B->bind(). A later NETLINK_DROP_MEMBERSHIP or
close calls B->unbind() again, so B sees two unbinds for one bind.

netlink_bind() behaves the same way. The group is not set in gone, so it
gets unbound through:

	netlink_undo_bind(BITS_PER_TYPE(u32), groups & ~gone, sk);

A family that refcounts in its bind/unbind callbacks would see its count
go below zero.

The commit message documents this limitation: "if a family is
unregistered and another one registers just after ->netlink_bind()
returned, and before the second read, the new family gets an unbind
without a bind". In-tree, only thermal has these callbacks, and it is
never unregistered at runtime.

Is this acceptable for out-of-tree or future families with refcounting
bind/unbind callbacks?

[ ... ]

> @@ -2079,6 +2140,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
>  		nl_table[unit].registered = 1;
>  	} else {
>  		kfree(listeners);
> +		kfree(mc_gen);
>  		nl_table[unit].registered++;
>  	}

[Severity: Low]
Does nl_table[NETLINK_USERSOCK] end up with a NULL mc_gen?

netlink_add_usersock_entry() sets groups, listeners, module, flags and
registered = 1 for NETLINK_USERSOCK, but it doesn't allocate mc_gen.

A later kernel socket on NETLINK_USERSOCK takes this else branch, because
registered is already set. It frees its own mc_gen without installing it.

__netlink_change_ngroups() and __netlink_clear_multicast_users() then
dereference tbl->mc_gen without a check:

	memcpy(mc_gen, tbl->mc_gen, tbl->groups * sizeof(*mc_gen));

	tbl->mc_gen[group - 1]++;

In-tree, both helpers are only called from genetlink with
net->genl_sock. USERSOCK has no ->bind, so the new mc_gen reads in
netlink_bind() and netlink_setsockopt() are not reached for it either.

Should netlink_add_usersock_entry() also allocate mc_gen, so that every
registered table has one?

[ ... ]

> @@ -2113,6 +2176,17 @@ int __netlink_change_ngroups(struct sock *sk, unsigned int groups)
>  	if (groups < 32)
>  		groups = 32;
>  
> +	if (tbl->groups < groups) {
> +		unsigned int *mc_gen;
> +
> +		mc_gen = kcalloc(groups, sizeof(*mc_gen), GFP_ATOMIC);
> +		if (!mc_gen)
> +			return -ENOMEM;
> +		memcpy(mc_gen, tbl->mc_gen, tbl->groups * sizeof(*mc_gen));
> +		kfree(tbl->mc_gen);
> +		tbl->mc_gen = mc_gen;
> +	}

[Severity: Low]
Does this still work if the number of groups is reduced? The kernel-doc
of netlink_change_ngroups() says:

 * change the number of groups to below 32. Also note that it does
 * not implicitly clear listeners from groups that are removed when
 * the number of groups is reduced.

A shrink lowers tbl->groups and keeps the existing mc_gen. Existing
sockets keep their larger nlk->ngroups, because netlink_realloc_groups()
only grows.

For example, take a table with 64 groups and a socket with
ngroups = 64. If the table shrinks to 32 and then grows to 48, mc_gen
is a 48-entry array and entries 32..47 are reset to 0.

netlink_setsockopt() bounds val only by nlk->ngroups:

	if (!val || val - 1 >= nlk->ngroups)
		return -EINVAL;
	...
	gen = tbl->mc_gen[val - 1];

So NETLINK_ADD_MEMBERSHIP with val = 60 would read past the end of
tbl->mc_gen[]. The reset entries could also make a released group look
unchanged, which would skip the new check.

In-tree, genl_validate_assign_mc_groups() is the only caller, and it only
grows the count, so this can't be reached today.

Should mc_gen be sized from the largest group count ever used? The other
option would be to remove the mention of shrinking from the kernel-doc.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102155819.4100930-1-anilkaushikwireless%40gmail.com

Re: [PATCH net v4 2/2] netlink: Check a group again if it was released while joining it

[Anil Kaushik]

The deadlock is fixed by my separate patch:
https://lore.kernel.org/netdev/20261102120929.4077030-1-anilkaushikwireless@gmail.com/

Anil