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

[PATCH net v3 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.

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   v3
  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

The test family counts its bind and unbind callbacks: they stay
balanced with v3. Normal joins, rtnetlink bind() and lockdep 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 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.

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 | 61 +++++++++++++++++++++++++++++++++++++++-
 net/netlink/af_netlink.h |  1 +
 net/netlink/genetlink.c  |  3 +-
 3 files changed, 63 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH net v3 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

[PATCH net v3 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 ->netlink_bind() and again once the table is
grabbed. If it changed, the group was released, so check it again
against its current owner. ->netlink_unbind() is not called for it: the
family that ->netlink_bind() checked it against is gone.

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

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 | 61 +++++++++++++++++++++++++++++++++++++++-
 net/netlink/af_netlink.h |  1 +
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index fec2879c497d..e1af3ac4bf6d 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,8 @@ 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 gen[BITS_PER_TYPE(u32)];
 	int err = 0;
 	unsigned long groups;
 	bool bound;
@@ -1023,9 +1027,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))
@@ -1057,6 +1066,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr_unsized *addr,
 	netlink_unlock_table();
 
 	netlink_table_grab();
+	if (nlk->netlink_bind && groups) {
+		unsigned long released = 0;
+		int group;
+
+		for_each_set_bit(group, &groups, BITS_PER_TYPE(u32))
+			if (gen[group] != tbl->mc_gen[group])
+				__set_bit(group, &released);
+		/* see netlink_setsockopt(), undo the others and retry all */
+		if (released) {
+			netlink_table_ungrab();
+			netlink_undo_bind(BITS_PER_TYPE(u32), groups & ~released,
+					  sk);
+			bound = true;
+			goto retry;
+		}
+	}
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(groups) -
 					 hweight32(nlk->groups[0]));
@@ -1686,6 +1711,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;
 		int err;
 
 		if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV))
@@ -1695,12 +1722,25 @@ 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_table_grab();
+		/* The group was released after ->netlink_bind() checked it, and
+		 * may have a new owner now. The old one is gone, so check the
+		 * group again without ->netlink_unbind().
+		 */
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind &&
+		    gen != tbl->mc_gen[val - 1]) {
+			netlink_table_ungrab();
+			goto retry;
+		}
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
@@ -2031,6 +2071,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 +2093,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 +2111,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 +2122,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 +2130,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 +2158,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 +2213,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 v3 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 · 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] Check-then-act race (TOCTOU) between genl_bind() and adding
  the socket to the group.

--

genetlink: Reject joining multicast groups no family owns

This commit makes genl_bind() return -ENOENT when no registered family
owns the requested multicast group ID. Before this change, any user could
join such an ID, and a family that got the ID later would inherit the
membership without any GENL_MCAST_CAP_* check.

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;
>  
>  	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;

[Severity: Medium]
Can a socket still end up as a member of an unowned group ID through a
race with genl_unregister_family()?

genl_bind() checks ownership and capabilities under down_read(&cb_lock),
then drops cb_lock and returns. The caller only sets the membership bit
later, under a different lock:

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() has the same gap. Its per-group loop calls
nlk->netlink_bind(net, group + 1), and the subscription update happens
afterwards under netlink_table_grab().

If genl_unregister_family() runs in that window, it only clears sockets
that are already members, and then frees the ID:

net/netlink/genetlink.c:genl_unregister_mc_groups() {
    ...
	netlink_table_grab();
	rcu_read_lock();
	for_each_net_rcu(net) {
		for (i = 0; i < family->n_mcgrps; i++)
			__netlink_clear_multicast_users(
				net->genl_sock, family->mcgrp_offset + i);
	}
	...
		if (grp_id != 1)
			clear_bit(grp_id, mc_groups);
    ...
}

The interleaving would look like this:

CPU1 (unprivileged)
netlink_setsockopt(NETLINK_ADD_MEMBERSHIP)
  genl_bind()       /* owner found, no caps needed, returns 0 */

CPU2
genl_unregister_family()
  genl_unregister_mc_groups()
    __netlink_clear_multicast_users()   /* CPU1 is not a member yet */
    clear_bit(grp_id, mc_groups)

CPU1
  netlink_table_grab()
  netlink_update_socket_mc()            /* now a member of a free ID */

A family registered later, for example one with GENL_MCAST_CAP_NET_ADMIN
or GENL_MCAST_CAP_SYS_ADMIN loaded via request_module(), could then get
that ID. The socket would receive its notifications without a capability
check. At this commit that is the same problem the patch is meant to fix,
although the window needs a family to be unregistered concurrently
(module unload, or module init failing after genl_register_family()).

A later patch in this series, "netlink: Check a group again if it was
released while joining it", appears to close this window. It adds a
per-group generation counter, tbl->mc_gen[], which
__netlink_clear_multicast_users() increments. netlink_setsockopt() and
netlink_bind() check the counter again under netlink_table_grab() and
retry genl_bind() if it changed.

Would it make sense to order that patch before this one, or fold the two
together, so that this commit does not leave the window open on its own?

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

Re: [PATCH net v3 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 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 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] The new retry path assumes that if a group's generation
  changed, the earlier ->netlink_bind() call was checked against the old…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a high-
  severity deadlock in the function the patch changes.

--

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

This adds a per-group generation counter, mc_gen, to the netlink table.
__netlink_clear_multicast_users() bumps the counter. netlink_bind() and
netlink_setsockopt(NETLINK_ADD_MEMBERSHIP) read it before
->netlink_bind() and compare it again once the table is grabbed. If the
counter changed, the group is re-checked against its current owner
without calling ->netlink_unbind().

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..08f89503f4073 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c

[ ... ]

> @@ -1057,6 +1066,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr_unsized *addr,
>  	netlink_unlock_table();
>  
>  	netlink_table_grab();
> +	if (nlk->netlink_bind && groups) {
> +		unsigned long released = 0;
> +		int group;
> +
> +		for_each_set_bit(group, &groups, BITS_PER_TYPE(u32))
> +			if (gen[group] != tbl->mc_gen[group])
> +				__set_bit(group, &released);
> +		/* see netlink_setsockopt(), undo the others and retry all */
> +		if (released) {
> +			netlink_table_ungrab();
> +			netlink_undo_bind(BITS_PER_TYPE(u32), groups & ~released,
> +					  sk);
> +			bound = true;
> +			goto retry;
> +		}
> +	}

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. The new
retry path calls netlink_table_ungrab() before netlink_undo_bind().
However, the older insert-failure path earlier in netlink_bind() still
calls netlink_undo_bind() while netlink_lock_table() is held:

    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;
        }
    }

On NETLINK_GENERIC sockets, netlink_undo_bind() ends up in genl_unbind().
genl_unbind() does down_read(&cb_lock) while nl_table_users is still
raised.

Can this deadlock against genl family unregistration?

CPU0 in netlink_bind():
  netlink_lock_table()                /* nl_table_users++ */
  netlink_insert() returns -EADDRINUSE
  netlink_undo_bind()
    genl_unbind()
      down_read(&cb_lock)             /* waits for CPU1 */

CPU1 in genl_unregister_family():
  genl_lock_all()                     /* down_write(&cb_lock) */
  genl_unregister_mc_groups()
    netlink_table_grab()              /* waits for nl_table_users == 0 */

genl_register_family() has the same shape. It calls netlink_table_grab()
from genl_validate_assign_mc_groups(), or goes through
netlink_change_ngroups(), while holding cb_lock for write.

genl_pernet_init() sets NL_CFG_F_NONROOT_RECV, so an unprivileged bind()
can reach CPU0's side. It needs nl_groups != 0 and an nl_pid that
another socket already owns. lockdep won't flag this, because
nl_table_users is a plain atomic counter.

The comment in netlink_release() already warns about this ordering:

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

[ ... ]

> @@ -1695,12 +1722,25 @@ 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_table_grab();
> +		/* The group was released after ->netlink_bind() checked it, and
> +		 * may have a new owner now. The old one is gone, so check the
> +		 * group again without ->netlink_unbind().
> +		 */
> +		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind &&
> +		    gen != tbl->mc_gen[val - 1]) {
> +			netlink_table_ungrab();
> +			goto retry;
> +		}

[Severity: Medium]
Is the assumption in this comment always true? gen is read and the table
lock is dropped before ->netlink_bind() runs. genl_bind() picks the
owning family only later, under down_read(&cb_lock).

Both of these can complete in that gap:

  genl_unregister_family(old)
    genl_unregister_mc_groups()
      __netlink_clear_multicast_users()   /* mc_gen[val - 1]++ */
      clear_bit(grp_id, mc_groups)
  genl_register_family(new)
    genl_allocate_reserve_groups()        /* hands out the same grp_id */

genl_bind() then finds the new family in genl_fam_idr and calls
family->bind(i) for it. Back under netlink_table_grab(),
gen != tbl->mc_gen[val - 1] is true. The code jumps to retry without
calling ->netlink_unbind(), and genl_bind() calls the new family's
->bind(i) a second time.

Only one membership bit gets set. A later NETLINK_DROP_MEMBERSHIP or
netlink_release() therefore calls genl_unbind() -> family->unbind(i)
only once. Doesn't that leave the new owner with two bind calls and one
unbind?

netlink_bind() does the same thing. It does memcpy(gen, tbl->mc_gen,
sizeof(gen)) before the bind loop. Groups in released are then left out
of netlink_undo_bind(..., groups & ~released, sk) and bound again on
retry.

A stateful .bind would stay unbalanced. For example, intel_hfi.c does:

    if (++hfi_clients_nr == 1)
        func = hfi_enable_instance;

and only disables the instance when the count drops back to 0.

In-tree, thermal_genl_family is the only genl family with a .bind
callback. It is registered from __init thermal_netlink_init() and never
unregistered, so this looks latent today. Still, "The old one is gone"
does not hold when the release finishes before genl_bind() takes
cb_lock.

Would it work to read the generation again right after ->netlink_bind()
returns? ->netlink_unbind() could then be called before retrying when
that post-bind value matches the current one.

[ ... ]

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

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

[Anil Kaushik]

The Medium is identified correctly, v4 will fix it.

The deadlock already exists without this patch, I have sent a separate patch.
https://lore.kernel.org/netdev/20261102120929.4077030-1-anilkaushikwireless@gmail.com/

On Sun, Nov 1, 2026 at 3:44 PM Anil Kaushik
<anilkaushikwireless@gmail.com> wrote:
>
> 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 ->netlink_bind() and again once the table is
> grabbed. If it changed, the group was released, so check it again
> against its current owner. ->netlink_unbind() is not called for it: the
> family that ->netlink_bind() checked it against is gone.
>
> bind() can then fail after it gave the socket its port ID. This only
> happens when a family is unregistered meanwhile.
>
> 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 | 61 +++++++++++++++++++++++++++++++++++++++-
>  net/netlink/af_netlink.h |  1 +
>  2 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index fec2879c497d..e1af3ac4bf6d 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,8 @@ 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 gen[BITS_PER_TYPE(u32)];
>         int err = 0;
>         unsigned long groups;
>         bool bound;
> @@ -1023,9 +1027,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))
> @@ -1057,6 +1066,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr_unsized *addr,
>         netlink_unlock_table();
>
>         netlink_table_grab();
> +       if (nlk->netlink_bind && groups) {
> +               unsigned long released = 0;
> +               int group;
> +
> +               for_each_set_bit(group, &groups, BITS_PER_TYPE(u32))
> +                       if (gen[group] != tbl->mc_gen[group])
> +                               __set_bit(group, &released);
> +               /* see netlink_setsockopt(), undo the others and retry all */
> +               if (released) {
> +                       netlink_table_ungrab();
> +                       netlink_undo_bind(BITS_PER_TYPE(u32), groups & ~released,
> +                                         sk);
> +                       bound = true;
> +                       goto retry;
> +               }
> +       }
>         netlink_update_subscriptions(sk, nlk->subscriptions +
>                                          hweight32(groups) -
>                                          hweight32(nlk->groups[0]));
> @@ -1686,6 +1711,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;
>                 int err;
>
>                 if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV))
> @@ -1695,12 +1722,25 @@ 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_table_grab();
> +               /* The group was released after ->netlink_bind() checked it, and
> +                * may have a new owner now. The old one is gone, so check the
> +                * group again without ->netlink_unbind().
> +                */
> +               if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind &&
> +                   gen != tbl->mc_gen[val - 1]) {
> +                       netlink_table_ungrab();
> +                       goto retry;
> +               }
>                 netlink_update_socket_mc(nlk, val,
>                                          optname == NETLINK_ADD_MEMBERSHIP);
>                 netlink_table_ungrab();
> @@ -2031,6 +2071,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 +2093,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 +2111,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 +2122,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 +2130,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 +2158,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 +2213,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
>

pw-bot: cr