Re: [PATCH net] genetlink: Reject joining multicast groups no family owns

[Anil Kaushik]

On Sat, Oct 31, 2026 at 4:10 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · 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).
>
> - [High] The fix is incomplete because of a check-then-use race.
>
> --
>
Yes I agree. genl_bind() drops cb_lock before the membership bit is set, so an
unregister in that window leaves the socket in a group ID no family owns.
v2 will add a patch that clears the members of the group IDs when a
family is given them, as suggested.
pw-bot: cr

> genetlink: Reject joining multicast groups no family owns
>
> This patch makes genl_bind() return -ENOENT when no registered family
> owns the requested multicast group ID. Before the patch such joins
> succeeded. If a family with GENL_MCAST_CAP_* flags was later given that
> ID, the socket stayed a member.
>
> Link: https://lore.kernel.org/netdev/179324692107.3134.14443689892607921337@xxxxxxxxxx/
>
> > 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: High]
> Does this fully close the case described in the commit message?
>
> genl_bind() checks ownership and the GENL_MCAST_CAP_* flags under
> down_read(&cb_lock). It drops the lock before it returns. The caller
> sets the membership bit only afterwards:
>
> 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_table_ungrab();
>     ...
> }
>
> netlink_bind() follows the same pattern. It updates nlk->groups[0] under
> netlink_table_grab() after genl_bind() has returned.
>
> The owning family can be unregistered inside that window, for example by
> a module unload or by a module init that fails after
> genl_register_family(). Can the following sequence happen?
>
> CPU1 (unprivileged)                  CPU2
> netlink_setsockopt(ADD_MEMBERSHIP, G)
>   genl_bind()
>     down_read(&cb_lock)
>     family F owns G, no caps, ret = 0
>     up_read(&cb_lock)
>                                      genl_unregister_family(F)
>                                        down_write(&cb_lock)
>                                        genl_unregister_mc_groups()
>                                          __netlink_clear_multicast_users()
>                                            (CPU1 not a member yet)
>                                          clear_bit(G, mc_groups)
>                                        idr_remove()
>   netlink_table_grab()
>   netlink_update_socket_mc(nlk, G, 1)
>
> After this sequence the socket is a member of a group ID that no family
> owns.
>
> Later, genl_register_family()->genl_validate_assign_mc_groups()->
> genl_allocate_reserve_groups() can give G to a family whose group has
> GENL_MCAST_CAP_NET_ADMIN or GENL_MCAST_CAP_SYS_ADMIN, such as psample,
> mptcp pm, ioam6 or cifs. Nothing clears the existing members of G at
> that point. do_one_broadcast() delivers based only on the membership
> bit. The CTRL_CMD_GETFAMILY->request_module() autoload path from the
> commit message could still trigger that registration.
>
> That is the end state the commit message says is fixed:
>
>   "A family registered later can get that ID, and the socket stays a
>   member: nothing checks it again."
>
> Would it be more robust to clear existing members of newly assigned
> group IDs in genl_validate_assign_mc_groups()? That could run under
> netlink_table_grab(), the same way genl_unregister_mc_groups() clears
> them on removal. It would cover both the original case and this race,
> instead of relying on the check in genl_bind().
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029062214.3407223-1-anilkaushikwireless%40gmail.com