quoted

Subject: [PATCH net v4] 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.

Reject a duplicate peer under the same lock before allocating, 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(), before the allocation, makes it the single point of truth
for both the discovery and the netlink add paths and keeps a flood of the same
address from churning the allocator.

Fixes: ef20cd4dd163 ("tipc: introduce UDP replicast")
Reported-by: Xiang Mei <redacted>
Suggested-by: Tung Nguyen <redacted>
Signed-off-by: Weiming Shi <redacted>
---
v4: (per Tung's review)
- Reject a duplicate under rcast_lock before allocating rcast, so a
  flood of the same address can't churn the allocator.
v3:
- Check for a duplicate peer in tipc_udp_rcast_add() under rcast_lock
  and remove tipc_udp_is_known_peer().
v2:
- Narrow the lock to the list mutation.

net/tipc/udp_media.c | 53 ++++++++++++++++++++------------------------
1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index

62ae7f5b5..3f0d75d72 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,26
+280,6 @@ 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)
{

@@ -308,16 +290,34 @@ static int tipc_udp_rcast_add(struct tipc_bearer *b,
      if (!ub)
              return -ENODEV;

+      /* Serialize with other updaters and reject a duplicate before
+       * allocating, so a flood of the same address can't churn the
+       * allocator.
+       */
+      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)) {
+              spin_unlock_bh(&ub->rcast_lock);
              kfree(rcast);
              return -ENOMEM;
      }

      memcpy(&rcast->addr, addr, sizeof(struct udp_media_addr));
+      list_add_rcu(&rcast->list, &ub->rcast.list);
+      b->bcast_addr.broadcast = TIPC_REPLICAST_SUPPORT;
+      spin_unlock_bh(&ub->rcast_lock);

sashiko reports potential memory leak when tipc_udp_nl_bearer_add() adds an element to the list right after cleanup_bearer() frees all element in the list.
I think we need to add more code to handle this case as below:

--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c

@@ -95,6 +95,7 @@ struct udp_replicast {
  * @work:      used to schedule deferred work on a bearer
  * @rcast:     associated udp_replicast container
  * @rcast_lock:        serialize updates to @rcast.list against concurrent updaters
+ * @bearer_disabled: flag to not add rcast to rcast.list if bearer was disabled
  */
 struct udp_bearer {
        struct tipc_bearer __rcu *bearer;

@@ -103,6 +104,7 @@ struct udp_bearer {
        struct work_struct work;
        struct udp_replicast rcast;
        spinlock_t rcast_lock; /* protects rcast.list */
+       bool bearer_disabled;
 };

 static int tipc_udp_is_mcast_addr(struct udp_media_addr *addr)

@@ -295,6 +297,10 @@ static int tipc_udp_rcast_add(struct tipc_bearer *b,
         * allocator.
         */
        spin_lock_bh(&ub->rcast_lock);
+       if (ub->bearer_disabled) {
+               spin_unlock_bh(&ub->rcast_lock);
+               return 0;
+       }
        list_for_each_entry(rcast, &ub->rcast.list, list) {
                if (!memcmp(&rcast->addr, addr, sizeof(*addr))) {
                        spin_unlock_bh(&ub->rcast_lock);

@@ -817,6 +823,7 @@ static void cleanup_bearer(struct work_struct *work)
                list_del_rcu(&rcast->list);
                call_rcu_hurry(&rcast->rcu, rcast_free_rcu);
        }
+       ub->bearer_disabled = true;
        spin_unlock_bh(&ub->rcast_lock);

        tn = tipc_net(sock_net(ub->sk));

quoted

      if (ntohs(addr->proto) == ETH_P_IP)
              pr_info("New replicast peer: %pI4\n", &rcast->addr.ipv4); @@
-325,8 +325,6 @@ static int tipc_udp_rcast_add(struct tipc_bearer *b,
      else if (ntohs(addr->proto) == ETH_P_IPV6)
              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;
}

@@ -361,9 +359,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 +639,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 +671,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 +812,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