netlink: do not free nlk->groups while lockless readers can use it
Subsystems (2)
PATCH 1/1 netlink: do not free nlk->groups while lockless readers can use it
netlink_realloc_groups() uses krealloc() under netlink_table_grab(). Whenever NLGRPSZ(groups) lands in a different kmalloc bucket, the old bitmap is freed immediately.
Two readers of nlk->groups / nlk->ngroups do not hold the netlink table lock:
1) sk_diag_dump_groups(). Hashed (bound) sockets are dumped from the rhashtable walk in __netlink_diag_dump(), which only holds RCU. Only the mc_list part of the dump takes nl_table_lock.
2) netlink_native_seq_show() (/proc/net/netlink), whose walk has been lockless since commit 21e4902aea80 ("netlink: Lockless lookup with RCU grace period in socket release").
Both can read a freed buffer, and sk_diag_dump_groups() can also read past the end of the old (smaller) buffer if it happens to load the old @groups pointer together with the new @ngroups value, copying the result into a NETLINK_DIAG_GROUPS attribute.
This is the same class of bug that commit f773608026ee ("netlink:
access nlk groups safely in netlink bind and getname") fixed for bind() and getname(); these two readers were missed. Simply grabbing the table lock in sk_diag_dump_groups() is not an option, because it is also called with nl_table_lock already held from the mc_list section of the dump.
Make the lockless readers safe instead:
- Allocate a new bitmap and free the old one after an RCU grace period, instead of relying on the implicit kfree() done by krealloc().
- Publish @groups before @ngroups, both with release semantics, and have the lockless readers load @ngroups first. A reader can then never pair the new (bigger) size with the old (smaller) buffer, and a reader picking up the new pointer while still seeing the old size is guaranteed to see the initialized bitmap.
netlink_realloc_groups() is called from process context (bind() and setsockopt()), so kfree_rcu_mightsleep() can be used, once the table has been released.
--- net/netlink/af_netlink.c | 42 ++++++++++++++++++++++++++++++++-------- net/netlink/diag.c | 20 +++++++++++++++---- 2 files changed, 50 insertions(+), 12 deletions(-)
net/netlink/af_netlink.c +34 -8
@@ -922,9 +922,9 @@ netlink_update_subscriptions(struct sock *sk, unsigned int subscriptions)static int netlink_realloc_groups (struct sock *sk) { +unsigned long *new_groups, *old_groups =NULL ;struct netlink_sock *nlk =nlk_sk (sk);unsigned int groups; -unsigned long *new_groups;int err =0 ;netlink_table_grab ();
@@ -938,18 +938,37 @@ static int netlink_realloc_groups(struct sock *sk)if (nlk->ngroups >= groups) goto out_unlock; - new_groups =krealloc (nlk->groups,NLGRPSZ (groups),GFP_ATOMIC ); -if (new_groups ==NULL ) { +/* Can not use krealloc(), because the old buffer might be freed +* immediately, while lockless readers (netlink diag dump and +* /proc/net/netlink) can still be looking at it. +*/ + new_groups =kzalloc (NLGRPSZ (groups),GFP_ATOMIC ); +if (!new_groups) { err = -ENOMEM ; goto out_unlock; } -memset ((char *)new_groups +NLGRPSZ (nlk->ngroups),0 , -NLGRPSZ (groups) -NLGRPSZ (nlk->ngroups)); + old_groups = nlk->groups; +if (old_groups) +memcpy (new_groups, old_groups,NLGRPSZ (nlk->ngroups)); + +/* Publish the new bitmap and its content: pairs with the address +* dependency in lockless readers, which can pick up the new pointer +* while still seeing the old (smaller) nlk->ngroups. +*/ +smp_store_release (&nlk->groups, new_groups); + +/* Then publish the new size: pairs with smp_load_acquire() from +* lockless readers, so that they can not read NLGRPSZ(new ngroups) +* bytes from the old buffer. +*/ +smp_store_release (&nlk->ngroups, groups); - nlk->groups = new_groups; - nlk->ngroups = groups; out_unlock:netlink_table_ungrab (); + +if (old_groups) +kfree_rcu_mightsleep (old_groups); +return err; }
@@ -2705,12 +2724,19 @@ static int netlink_native_seq_show(struct seq_file *seq, void *v) }else {struct sock *s = v;struct netlink_sock *nlk =nlk_sk (s); +const unsigned long *groups; + +/* Lockless read : netlink_realloc_groups() can change +* nlk->groups under us. The old buffer is freed after an +* RCU grace period, and this walk is RCU protected. +*/ + groups =READ_ONCE (nlk->groups); seq_printf(seq, "%pK %-3d %-10u %08x %-8d %-8d %-5d %-8d %-8u %-8llu\n", s, s->sk_protocol, nlk->portid, - nlk->groups ? (u32)nlk->groups[0] : 0, + groups ? (u32)groups[0] : 0, sk_rmem_alloc_get(s), sk_wmem_alloc_get(s), READ_ONCE(nlk->cb_running),
net/netlink/diag.c +16 -4
@@ -12,12 +12,24 @@static int sk_diag_dump_groups (struct sock *sk,struct sk_buff *nlskb) {struct netlink_sock *nlk =nlk_sk (sk); - -if (nlk->groups ==NULL ) +unsigned long *groups; +unsigned int ngroups; + +/* Hashed sockets are dumped from the rhashtable walk, which only +* holds rcu_read_lock(), while netlink_realloc_groups() can replace +* nlk->groups and nlk->ngroups at any time. +* +* Read nlk->ngroups first : this pairs with smp_store_release() +* from netlink_realloc_groups(), so that we can not use the new +* (bigger) size with the old (smaller) buffer. The old buffer is +* freed after an RCU grace period. +*/ + ngroups =smp_load_acquire (&nlk->ngroups); + groups =READ_ONCE (nlk->groups); +if (!groups)return 0 ; -return nla_put (nlskb,NETLINK_DIAG_GROUPS ,NLGRPSZ (nlk->ngroups), - nlk->groups); +return nla_put (nlskb,NETLINK_DIAG_GROUPS ,NLGRPSZ (ngroups), groups); }static int sk_diag_put_flags (struct sock *sk,struct sk_buff *skb)
Signature · 1 line
2.55.0.1007.g17ff1f9808-goog
Jakub Kicinski
maintainernet/netlink/af_netlink.c static int netlink_realloc_groups(struct sock *sk) · line 957
Quoted 4 lines · On Fri, 11 Sep 2026 16:08:04 +0000 Eric Dumazet wrote:
net/netlink/af_netlink.c +4
-0+/* Publish the new bitmap and its content: pairs with the address +* dependency in lockless readers, which can pick up the new pointer +* while still seeing the old (smaller) nlk->ngroups. +*/
What do you think about the value of these comments, Eric?
Quoted 1 line
net/netlink/af_netlink.c +1
-0+smp_store_release (&nlk->groups, new_groups);
Quoted 8 lines · Jakub Kicinski
net/netlink/af_netlink.c static int netlink_realloc_groups(struct sock *sk) · line 957
Quoted 4 lines · On Fri, 11 Sep 2026 16:08:04 +0000 Eric Dumazet wrote:
net/netlink/af_netlink.c +4
-0+/* Publish the new bitmap and its content: pairs with the address +* dependency in lockless readers, which can pick up the new pointer +* while still seeing the old (smaller) nlk->ngroups. +*/ What do you think about the value of these comments, Eric?
I think they might prevent Sashiko and AI other agents from asking for a full RCU conversion,
but I do not know.
Note that I had a first verstion of the patch with a missing smp_store_release(),
so even LLM can get this wrong.
This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
Quoted 8 lines · On Fri, 11 Sep 2026 16:08:04 +0000 you wrote:
netlink_realloc_groups()useskrealloc()undernetlink_table_grab(). Whenever NLGRPSZ(groups) lands in a different kmalloc bucket, the old bitmap is freed immediately.Two readers of
nlk->groups/nlk->ngroupsdo not hold the netlink table lock:[...]
Here is the summary with links:
- [net] netlink: do not free nlk->groups while lockless readers can use it
https://git.kernel.org/netdev/net/c/ceac0de741bf
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html