Re: [PATCH net v2] tipc: serialize udp bearer replicast list updates

[Weiming Shi]

Tung Quang Nguyen <tung.quang.nguyen@xxxxxxxx> 于2026年7月9日周四 14:42写道：
>
> >Subject: [PATCH net v2] 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.
> >
> >While here, switch the read-only walk in tipc_udp_is_known_peer() to
> >list_for_each_entry_rcu(); it never deletes, so list_for_each_entry_safe() was
> >misleading.
> >
> >Fixes: ef20cd4dd163 ("tipc: introduce UDP replicast")
> >Reported-by: Xiang Mei <xmei5@xxxxxxx>
> >Suggested-by: Tung Nguyen <tung.quang.nguyen@xxxxxxxx>
> >Assisted-by: Claude:claude-opus-4-8
> >Signed-off-by: Weiming Shi <bestswngs@xxxxxxxxx>
> >---
> >v2: (per Tung's review)
> > - Narrow the lock to just list_add_rcu().
> > - Drop the under-lock dup re-check; serializing the writers is enough.
> > - Use list_for_each_entry_rcu() in tipc_udp_is_known_peer().
> >
> > net/tipc/udp_media.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> >diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index
> >62ae7f5b5840..c6aa8c3c54ce 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:       serializes updates to @rcast.list
> >  */
> > 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) @@ -281,7
> >+283,7 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
> >static bool tipc_udp_is_known_peer(struct tipc_bearer *b,
> >                                  struct udp_media_addr *addr)
> > {
> >-      struct udp_replicast *rcast, *tmp;
> >+      struct udp_replicast *rcast;
> >       struct udp_bearer *ub;
> >
> >       ub = rcu_dereference_rtnl(b->media_ptr);
> >@@ -290,7 +292,7 @@ static bool tipc_udp_is_known_peer(struct tipc_bearer
> >*b,
> >               return false;
> >       }
> >
> >-      list_for_each_entry_safe(rcast, tmp, &ub->rcast.list, list) {
> >+      list_for_each_entry_rcu(rcast, &ub->rcast.list, list) {
> >               if (!memcmp(&rcast->addr, addr, sizeof(struct
> >udp_media_addr)))
> >                       return true;
> >       }
>
> sashiko reports:
>
> ...
> Is there a possibility of triggering a lockdep warning here?
> Since this function is called from tipc_udp_nl_bearer_add() during Netlink
> configuration, rtnl_lock() is held but rcu_read_lock() is not.
> Without an explicit lockdep condition like lockdep_rtnl_is_held() passed to
> the list traversal macro, lockdep will complain about the RCU list being
> traversed outside a reader section.
> >                 if (!memcmp(&rcast->addr, addr, sizeof(struct udp_media_addr)))
> >                         return true;
> >         }
> > @@ -326,7 +328,9 @@ 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;
> > +       spin_lock_bh(&ub->rcast_lock);
> >         list_add_rcu(&rcast->list, &ub->rcast.list);
> > +       spin_unlock_bh(&ub->rcast_lock);
> >         return 0;
> >  }
> Could this allow duplicate peers to be added to the list?
> The under-lock duplicate check was removed in this patch iteration. Since
> tipc_udp_is_known_peer() is checked locklessly earlier in
> tipc_udp_rcast_disc(), two concurrent packets from the same unknown peer
> could both see the peer as missing.
> They would then both acquire rcast_lock sequentially and add duplicate
> entries, leading to memory leaks and traffic amplification when broadcasting.
> [ ... ]
> ...
>
> I think we have to check duplicate address in tipc_udp_rcast_add() before adding and remove tipc_udp_is_known_peer().
>
> >@@ -326,7 +328,9 @@ 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;
> >+      spin_lock_bh(&ub->rcast_lock);
> >       list_add_rcu(&rcast->list, &ub->rcast.list);
> >+      spin_unlock_bh(&ub->rcast_lock);
> >       return 0;
> > }
> >
> >@@ -679,6 +683,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 +824,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
>

Thanks for the review. v3 has been sent.