* RE: [PATCH net v3] tipc: serialize udp bearer replicast list updates
2026-07-10 3:29 [PATCH net v3] tipc: serialize udp bearer replicast list updates Weiming Shi
@ 2026-07-10 15:39 ` Tung Quang Nguyen
0 siblings, 0 replies; 2+ messages in thread
From: Tung Quang Nguyen @ 2026-07-10 15:39 UTC (permalink / raw)
To: Weiming Shi
Cc: netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net,
linux-kernel@vger.kernel.org, xmei5@asu.edu, Jon Maloy,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman
>Subject: [PATCH net v3] tipc: serialize udp bearer replicast list updates
>
>tipc_udp_rcast_add() and cleanup_bearer() both update ub->rcast.list with
>list_add_rcu() / list_del_rcu(), but nothing serializes them. The add runs from
>the encap receive softirq (via tipc_udp_rcast_disc()) without rtnl_lock(), so it
>can race the cleanup delete and corrupt the list:
>
> list_del corruption. prev->next should be ffff8880298d7ab8,
> but was ffff88802449ad38. (prev=ffff888027e3ec98)
> kernel BUG at lib/list_debug.c:62!
> RIP: __list_del_entry_valid_or_report+0x17a/0x200
> Workqueue: events cleanup_bearer
> Call Trace:
> cleanup_bearer (net/tipc/udp_media.c:811)
> process_one_work (kernel/workqueue.c:3302)
> worker_thread (kernel/workqueue.c:3466)
>
>The bearer can be enabled from an unprivileged user namespace, as the
>TIPCv2 generic-netlink ops carry no GENL_ADMIN_PERM.
>
>Add a spinlock to struct udp_bearer and take it around the list_add_rcu() in
>tipc_udp_rcast_add() and the list_del_rcu() loop in cleanup_bearer() so the
>two writers can no longer corrupt the list.
>
>Check for a duplicate peer under the same lock before adding, and remove
>tipc_udp_is_known_peer(). The old lockless pre-check in
>tipc_udp_rcast_disc() was racy: two softirqs discovering the same peer could
>both find it absent and add it twice. Doing the check under rcast_lock in
>tipc_udp_rcast_add() makes it the single point of truth for both the discovery
>and the netlink add paths.
>
>Fixes: ef20cd4dd163 ("tipc: introduce UDP replicast")
>Reported-by: Xiang Mei <xmei5@asu.edu>
>Suggested-by: Tung Nguyen <tung.quang.nguyen@est.tech>
>Signed-off-by: Weiming Shi <bestswngs@gmail.com>
>Assisted-by: Claude:claude-opus-4-8
>---
>v3: (per Tung's review)
> - Check for a duplicate peer in tipc_udp_rcast_add() under rcast_lock
> before adding, and remove tipc_udp_is_known_peer().
> - This also drops the list_for_each_entry_rcu() change to
> tipc_udp_is_known_peer() from v2, since the helper is gone.
>v2:
> - Narrow the lock to the list mutation.
>
> net/tipc/udp_media.c | 45 ++++++++++++++++++--------------------------
> 1 file changed, 18 insertions(+), 27 deletions(-)
>
>diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index
>62ae7f5b5..3a7d9b15c 100644
>--- a/net/tipc/udp_media.c
>+++ b/net/tipc/udp_media.c
>@@ -94,6 +94,7 @@ struct udp_replicast {
> * @ifindex: local address scope
> * @work: used to schedule deferred work on a bearer
> * @rcast: associated udp_replicast container
>+ * @rcast_lock: serialize updates to @rcast.list against concurrent
>updaters
> */
> struct udp_bearer {
> struct tipc_bearer __rcu *bearer;
>@@ -101,6 +102,7 @@ struct udp_bearer {
> u32 ifindex;
> struct work_struct work;
> struct udp_replicast rcast;
>+ spinlock_t rcast_lock; /* protects rcast.list */
> };
>
> static int tipc_udp_is_mcast_addr(struct udp_media_addr *addr) @@ -278,30
>+280,10 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
> return err;
> }
>
>-static bool tipc_udp_is_known_peer(struct tipc_bearer *b,
>- struct udp_media_addr *addr)
>-{
>- struct udp_replicast *rcast, *tmp;
>- struct udp_bearer *ub;
>-
>- ub = rcu_dereference_rtnl(b->media_ptr);
>- if (!ub) {
>- pr_err_ratelimited("UDP bearer instance not found\n");
>- return false;
>- }
>-
>- list_for_each_entry_safe(rcast, tmp, &ub->rcast.list, list) {
>- if (!memcmp(&rcast->addr, addr, sizeof(struct
>udp_media_addr)))
>- return true;
>- }
>-
>- return false;
>-}
>-
> static int tipc_udp_rcast_add(struct tipc_bearer *b,
> struct udp_media_addr *addr)
> {
>- struct udp_replicast *rcast;
>+ struct udp_replicast *rcast, *tmp;
> struct udp_bearer *ub;
>
> ub = rcu_dereference_rtnl(b->media_ptr);
>@@ -326,7 +308,19 @@ static int tipc_udp_rcast_add(struct tipc_bearer *b,
> pr_info("New replicast peer: %pI6\n", &rcast->addr.ipv6);
>#endif
> b->bcast_addr.broadcast = TIPC_REPLICAST_SUPPORT;
>+
>+ /* serialize with other updaters and drop duplicates under the lock */
>+ spin_lock_bh(&ub->rcast_lock);
>+ list_for_each_entry(tmp, &ub->rcast.list, list) {
>+ if (!memcmp(&tmp->addr, addr, sizeof(*addr))) {
>+ spin_unlock_bh(&ub->rcast_lock);
>+ dst_cache_destroy(&rcast->dst_cache);
>+ kfree(rcast);
>+ return 0;
>+ }
>+ }
> list_add_rcu(&rcast->list, &ub->rcast.list);
>+ spin_unlock_bh(&ub->rcast_lock);
To avoid freeing allocated duplicate 'rcast' and avoid storm of malicious duplicate address attacks that could lead to running out of memory when allocating 'rcast', we should change like this:
@@ -308,16 +308,28 @@ static int tipc_udp_rcast_add(struct tipc_bearer *b,
if (!ub)
return -ENODEV;
+ spin_lock_bh(&ub->rcast_lock);
+ list_for_each_entry(rcast, &ub->rcast.list, list) {
+ if (!memcmp(&rcast->addr, addr, sizeof(*addr))) {
+ spin_unlock_bh(&ub->rcast_lock);
+ return 0;
+ }
+ }
rcast = kmalloc_obj(*rcast, GFP_ATOMIC);
- if (!rcast)
+ if (!rcast) {
+ spin_unlock_bh(&ub->rcast_lock);
return -ENOMEM;
+ }
if (dst_cache_init(&rcast->dst_cache, GFP_ATOMIC)) {
kfree(rcast);
+ spin_unlock_bh(&ub->rcast_lock);
return -ENOMEM;
}
memcpy(&rcast->addr, addr, sizeof(struct udp_media_addr));
+ list_add_rcu(&rcast->list, &ub->rcast.list);
+ spin_unlock_bh(&ub->rcast_lock);
if (ntohs(addr->proto) == ETH_P_IP)
pr_info("New replicast peer: %pI4\n", &rcast->addr.ipv4);
@@ -326,7 +338,6 @@ static int tipc_udp_rcast_add(struct tipc_bearer *b,
pr_info("New replicast peer: %pI6\n", &rcast->addr.ipv6); #endif
b->bcast_addr.broadcast = TIPC_REPLICAST_SUPPORT;
- list_add_rcu(&rcast->list, &ub->rcast.list);
return 0;
}
> return 0;
> }
>
>@@ -361,9 +355,6 @@ static int tipc_udp_rcast_disc(struct tipc_bearer *b,
>struct sk_buff *skb)
> return 0;
> }
>
>- if (likely(tipc_udp_is_known_peer(b, &src)))
>- return 0;
>-
> return tipc_udp_rcast_add(b, &src);
> }
>
>@@ -644,9 +635,6 @@ int tipc_udp_nl_bearer_add(struct tipc_bearer *b,
>struct nlattr *attr)
> return -EINVAL;
> }
>
>- if (tipc_udp_is_known_peer(b, &addr))
>- return 0;
>-
> return tipc_udp_rcast_add(b, &addr);
> }
>
>@@ -679,6 +667,7 @@ static int tipc_udp_enable(struct net *net, struct
>tipc_bearer *b,
> return -ENOMEM;
>
> INIT_LIST_HEAD(&ub->rcast.list);
>+ spin_lock_init(&ub->rcast_lock);
>
> if (!attrs[TIPC_NLA_BEARER_UDP_OPTS])
> goto err;
>@@ -819,10 +808,12 @@ static void cleanup_bearer(struct work_struct
>*work)
> struct udp_replicast *rcast, *tmp;
> struct tipc_net *tn;
>
>+ spin_lock_bh(&ub->rcast_lock);
> list_for_each_entry_safe(rcast, tmp, &ub->rcast.list, list) {
> list_del_rcu(&rcast->list);
> call_rcu_hurry(&rcast->rcu, rcast_free_rcu);
> }
>+ spin_unlock_bh(&ub->rcast_lock);
>
> tn = tipc_net(sock_net(ub->sk));
>
>--
>2.43.0
^ permalink raw reply [flat|nested] 2+ messages in thread