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@xxxxxxxxx/

On Sun, Nov 1, 2026 at 3:44 PM Anil Kaushik
<anilkaushikwireless@xxxxxxxxx> 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@xxxxxxxxxx/
> Fixes: 4d54cc32112d ("mptcp: avoid lock_fast usage in accept path")
> Signed-off-by: Anil Kaushik <anilkaushikwireless@xxxxxxxxx>
> ---
>  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