[PATCH net v2] tipc: fix several race conditions caused by tipc_node_write_unlock()

[PATCH net v2] tipc: fix several race conditions caused by tipc_node_write_unlock()

[Tung Nguyen]

tipc_node_write_unlock() releases the node lock after resetting n->action_flags.
This creates a window for several race conditions:

- A race between link-up and link-down events can occur when the
link-down thread is interrupted before removing a publication from
nt->cluster_scope, and the link-up event inserts a publication into
nt->cluster_scope.

- A race can occur between a tipc_rcv() thread that adds or removes
publications to or from node->publ_list and a node-down thread that
removes publications from node->publ_list. The latter thread traverses
node->publ_list without proper lock protection.

These race conditions can result in various use-after-free issues.

Fix these race conditions by:

1. Holding the node lock during link-up/link-down and node-up/node-down
   events.
2. Removing the node lookup and node lock from
   tipc_node_subscribe() and tipc_node_unsubscribe().
3. Holding the node lock before calling tipc_named_rcv() from
   tipc_node_bc_rcv() and tipc_rcv().
4. Moving tipc_node_broadcast() from tipc_nametbl_publish() and
   tipc_nametbl_withdraw() to tipc_node_write_unlock().
5. Moving tipc_node_xmit() from tipc_named_node_up() to
   tipc_node_write_unlock().

Fixes: 5405ff6e15f4 ("tipc: convert node lock to rwlock")
Reported-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Closes: https://lore.kernel.org/netdev/20261001182924.3928331-2-nicoyip.dev@gmail.com/
Closes: https://lore.kernel.org/netdev/20261001182924.3928331-3-nicoyip.dev@gmail.com/
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202610060355.bPzTfoXZ-lkp@intel.com/
Signed-off-by: Tung Nguyen <tung.quang.nguyen@est.tech>
---
v2: Address build warnings detected by kernel test robot and remove unused
variable in tipc_named_node_up() detected by sashiko.
v1: https://lore.kernel.org/netdev/20261005041053.22695-1-tung.quang.nguyen@est.tech/

 net/tipc/name_distr.c | 32 +++++++++--------
 net/tipc/name_distr.h |  7 ++--
 net/tipc/name_table.c | 25 ++++++-------
 net/tipc/name_table.h |  6 ++--
 net/tipc/net.c        |  8 +++--
 net/tipc/node.c       | 84 +++++++++++++++++++------------------------
 net/tipc/node.h       |  4 +--
 net/tipc/socket.c     | 24 +++++++++----
 8 files changed, 97 insertions(+), 93 deletions(-)

diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index ba4f4906e13b..dcb15de42a9e 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -202,15 +202,15 @@ static void named_distribute(struct net *net, struct sk_buff_head *list,
  * @net: the associated network namespace
  * @dnode: destination node
  * @capabilities: peer node's capabilities
+ * @xmitq: list of skbs need to be sent
  */
-void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
+void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities,
+			struct sk_buff_head *xmitq)
 {
 	struct name_table *nt = tipc_name_table(net);
 	struct tipc_net *tn = tipc_net(net);
-	struct sk_buff_head head;
 	u16 seqno;
 
-	__skb_queue_head_init(&head);
 	spin_lock_bh(&tn->nametbl_lock);
 	if (!(capabilities & TIPC_NAMED_BCAST))
 		nt->rc_dests++;
@@ -218,8 +218,7 @@ void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
 	spin_unlock_bh(&tn->nametbl_lock);
 
 	read_lock_bh(&nt->cluster_scope_lock);
-	named_distribute(net, &head, dnode, &nt->cluster_scope, seqno);
-	tipc_node_xmit(net, &head, dnode, 0);
+	named_distribute(net, xmitq, dnode, &nt->cluster_scope, seqno);
 	read_unlock_bh(&nt->cluster_scope_lock);
 }
 
@@ -227,12 +226,11 @@ void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
  * tipc_publ_purge - remove publication associated with a failed node
  * @net: the associated network namespace
  * @p: the publication to remove
- * @addr: failed node's address
  *
  * Invoked for each publication issued by a newly failed node.
  * Removes publication structure from name table & deletes it.
  */
-static void tipc_publ_purge(struct net *net, struct publication *p, u32 addr)
+static void tipc_publ_purge(struct net *net, struct publication *p)
 {
 	struct tipc_net *tn = tipc_net(net);
 	struct publication *_p;
@@ -243,14 +241,14 @@ static void tipc_publ_purge(struct net *net, struct publication *p, u32 addr)
 	spin_lock_bh(&tn->nametbl_lock);
 	_p = tipc_nametbl_remove_publ(net, &ua, &p->sk, p->key);
 	if (_p)
-		tipc_node_unsubscribe(net, &_p->binding_node, addr);
+		tipc_node_unsubscribe(&_p->binding_node);
 	spin_unlock_bh(&tn->nametbl_lock);
 	if (_p)
 		kfree_rcu(_p, rcu);
 }
 
 void tipc_publ_notify(struct net *net, struct list_head *nsub_list,
-		      u32 addr, u16 capabilities)
+		      u16 capabilities)
 {
 	struct name_table *nt = tipc_name_table(net);
 	struct tipc_net *tn = tipc_net(net);
@@ -258,7 +256,7 @@ void tipc_publ_notify(struct net *net, struct list_head *nsub_list,
 	struct publication *publ, *tmp;
 
 	list_for_each_entry_safe(publ, tmp, nsub_list, binding_node)
-		tipc_publ_purge(net, publ, addr);
+		tipc_publ_purge(net, publ);
 	spin_lock_bh(&tn->nametbl_lock);
 	if (!(capabilities & TIPC_NAMED_BCAST))
 		nt->rc_dests--;
@@ -272,12 +270,14 @@ void tipc_publ_notify(struct net *net, struct list_head *nsub_list,
  * @i: location of item in the message
  * @node: node address
  * @dtype: name distributor message type
+ * @publ_list: list of remote publications of a specific node
  *
  * tipc_nametbl_lock must be held.
  * Return: the publication item if successful, otherwise NULL.
  */
 static bool tipc_update_nametbl(struct net *net, struct distr_item *i,
-				u32 node, u32 dtype)
+				u32 node, u32 dtype,
+				struct list_head *publ_list)
 {
 	struct publication *p = NULL;
 	u32 lower = ntohl(i->lower);
@@ -301,13 +301,13 @@ static bool tipc_update_nametbl(struct net *net, struct distr_item *i,
 	if (dtype == PUBLICATION) {
 		p = tipc_nametbl_insert_publ(net, &ua, &sk, key);
 		if (p) {
-			tipc_node_subscribe(net, &p->binding_node, node);
+			tipc_node_subscribe(&p->binding_node, publ_list);
 			return true;
 		}
 	} else if (dtype == WITHDRAWAL) {
 		p = tipc_nametbl_remove_publ(net, &ua, &sk, key);
 		if (p) {
-			tipc_node_unsubscribe(net, &p->binding_node, node);
+			tipc_node_unsubscribe(&p->binding_node);
 			kfree_rcu(p, rcu);
 			return true;
 		}
@@ -367,11 +367,12 @@ static struct sk_buff *tipc_named_dequeue(struct sk_buff_head *namedq,
  * tipc_named_rcv - process name table update messages sent by another node
  * @net: the associated network namespace
  * @namedq: queue to receive from
+ * @publ_list: list of remote publications of a specific node
  * @rcv_nxt: store last received seqno here
  * @open: last bulk msg was received (FIXME)
  */
 void tipc_named_rcv(struct net *net, struct sk_buff_head *namedq,
-		    u16 *rcv_nxt, bool *open)
+		    struct list_head *publ_list, u16 *rcv_nxt, bool *open)
 {
 	struct tipc_net *tn = tipc_net(net);
 	struct distr_item *item;
@@ -386,7 +387,8 @@ void tipc_named_rcv(struct net *net, struct sk_buff_head *namedq,
 		item = (struct distr_item *)msg_data(hdr);
 		count = msg_data_sz(hdr) / ITEM_SIZE;
 		while (count--) {
-			tipc_update_nametbl(net, item, node, msg_type(hdr));
+			tipc_update_nametbl(net, item, node,
+					    msg_type(hdr), publ_list);
 			item++;
 		}
 		kfree_skb(skb);
diff --git a/net/tipc/name_distr.h b/net/tipc/name_distr.h
index c677f6f082df..14c008ab8644 100644
--- a/net/tipc/name_distr.h
+++ b/net/tipc/name_distr.h
@@ -69,11 +69,12 @@ struct distr_item {
 
 struct sk_buff *tipc_named_publish(struct net *net, struct publication *publ);
 struct sk_buff *tipc_named_withdraw(struct net *net, struct publication *publ);
-void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities);
+void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities,
+			struct sk_buff_head *xmitq);
 void tipc_named_rcv(struct net *net, struct sk_buff_head *namedq,
-		    u16 *rcv_nxt, bool *open);
+		    struct list_head *publ_list, u16 *rcv_nxt, bool *open);
 void tipc_named_reinit(struct net *net);
 void tipc_publ_notify(struct net *net, struct list_head *nsub_list,
-		      u32 addr, u16 capabilities);
+		      u16 capabilities);
 
 #endif
diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 6fda36ab1766..9ae3190ff6ea 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -760,15 +760,14 @@ void tipc_nametbl_build_group(struct net *net, struct tipc_group *grp,
 /* tipc_nametbl_publish - add service binding to name table
  */
 struct publication *tipc_nametbl_publish(struct net *net, struct tipc_uaddr *ua,
-					 struct tipc_socket_addr *sk, u32 key)
+					 struct tipc_socket_addr *sk, u32 key,
+					 struct sk_buff **skb, u32 *rc_dests)
 {
 	struct name_table *nt = tipc_name_table(net);
 	u32 max_user_pub = TIPC_MAX_PUBL - 1;
 	struct tipc_net *tn = tipc_net(net);
 	struct publication *p = NULL;
-	struct sk_buff *skb = NULL;
 	bool protocol_type = false;
-	u32 rc_dests;
 
 	if (ua->sr.type == TIPC_NODE_STATE || ua->sr.type == TIPC_LINK_STATE ||
 	    ua->sr.type == TIPC_TOP_SRV)
@@ -797,14 +796,12 @@ struct publication *tipc_nametbl_publish(struct net *net, struct tipc_uaddr *ua,
 		 */
 		if (!protocol_type)
 			nt->local_publ_count++;
-		skb = tipc_named_publish(net, p);
+		*skb = tipc_named_publish(net, p);
 	}
-	rc_dests = nt->rc_dests;
+	*rc_dests = nt->rc_dests;
 exit:
 	spin_unlock_bh(&tn->nametbl_lock);
 
-	if (skb)
-		tipc_node_broadcast(net, skb, rc_dests);
 	return p;
 
 }
@@ -815,15 +812,16 @@ struct publication *tipc_nametbl_publish(struct net *net, struct tipc_uaddr *ua,
  * @ua: service address/range being unbound
  * @sk: address of the socket being unbound from
  * @key: target publication key
+ * @skb: name distribution message needs to be sent
+ * @rc_dests: the number of replicast destinations
  */
 void tipc_nametbl_withdraw(struct net *net, struct tipc_uaddr *ua,
-			   struct tipc_socket_addr *sk, u32 key)
+			   struct tipc_socket_addr *sk, u32 key,
+			   struct sk_buff **skb, u32 *rc_dests)
 {
 	struct name_table *nt = tipc_name_table(net);
 	struct tipc_net *tn = tipc_net(net);
-	struct sk_buff *skb = NULL;
 	struct publication *p;
-	u32 rc_dests;
 
 	spin_lock_bh(&tn->nametbl_lock);
 
@@ -833,15 +831,12 @@ void tipc_nametbl_withdraw(struct net *net, struct tipc_uaddr *ua,
 		    p->sr.type != TIPC_LINK_STATE &&
 		    p->sr.type != TIPC_TOP_SRV)
 			nt->local_publ_count--;
-		skb = tipc_named_withdraw(net, p);
+		*skb = tipc_named_withdraw(net, p);
 		list_del_init(&p->binding_sock);
 		kfree_rcu(p, rcu);
 	}
-	rc_dests = nt->rc_dests;
+	*rc_dests = nt->rc_dests;
 	spin_unlock_bh(&tn->nametbl_lock);
-
-	if (skb)
-		tipc_node_broadcast(net, skb, rc_dests);
 }
 
 /**
diff --git a/net/tipc/name_table.h b/net/tipc/name_table.h
index 7ff6eeebaae6..6cbc9da17464 100644
--- a/net/tipc/name_table.h
+++ b/net/tipc/name_table.h
@@ -126,9 +126,11 @@ bool tipc_nametbl_lookup_group(struct net *net, struct tipc_uaddr *ua,
 void tipc_nametbl_build_group(struct net *net, struct tipc_group *grp,
 			      struct tipc_uaddr *ua);
 struct publication *tipc_nametbl_publish(struct net *net, struct tipc_uaddr *ua,
-					 struct tipc_socket_addr *sk, u32 key);
+					 struct tipc_socket_addr *sk, u32 key,
+					 struct sk_buff **skb, u32 *rc_dests);
 void tipc_nametbl_withdraw(struct net *net, struct tipc_uaddr *ua,
-			   struct tipc_socket_addr *sk, u32 key);
+			   struct tipc_socket_addr *sk, u32 key,
+			   struct sk_buff **skb, u32 *rc_dests);
 struct publication *tipc_nametbl_insert_publ(struct net *net,
 					     struct tipc_uaddr *ua,
 					     struct tipc_socket_addr *sk,
diff --git a/net/tipc/net.c b/net/tipc/net.c
index 7e65d0b0c4a8..1e445c5abc0f 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -125,9 +125,11 @@ int tipc_net_init(struct net *net, u8 *node_id, u32 addr)
 
 static void tipc_net_finalize(struct net *net, u32 addr)
 {
-	struct tipc_net *tn = tipc_net(net);
 	struct tipc_socket_addr sk = {0, addr};
+	struct tipc_net *tn = tipc_net(net);
+	struct sk_buff *skb = NULL;
 	struct tipc_uaddr ua;
+	u32 rc_dests;
 
 	tipc_uaddr(&ua, TIPC_SERVICE_RANGE, TIPC_CLUSTER_SCOPE,
 		   TIPC_NODE_STATE, addr, addr);
@@ -138,7 +140,9 @@ static void tipc_net_finalize(struct net *net, u32 addr)
 	tipc_named_reinit(net);
 	tipc_sk_reinit(net);
 	tipc_mon_reinit_self(net);
-	tipc_nametbl_publish(net, &ua, &sk, addr);
+	tipc_nametbl_publish(net, &ua, &sk, addr, &skb, &rc_dests);
+	if (skb)
+		tipc_node_broadcast(net, skb, rc_dests);
 }
 
 void tipc_net_finalize_work(struct work_struct *work)
diff --git a/net/tipc/node.c b/net/tipc/node.c
index d7cbfa786c13..182c6dbfa49d 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -397,44 +397,50 @@ static void tipc_node_write_unlock(struct tipc_node *n)
 	__releases(n->lock)
 {
 	struct tipc_socket_addr sk;
+	struct sk_buff *skb = NULL;
+	struct sk_buff_head xmitq;
 	struct net *net = n->net;
-	u32 flags = n->action_flags;
-	struct list_head *publ_list;
 	struct tipc_uaddr ua;
 	u32 bearer_id, node;
+	u32 rc_dests;
 
-	if (likely(!flags)) {
+	if (likely(!n->action_flags)) {
 		write_unlock_bh(&n->lock);
 		return;
 	}
 
+	__skb_queue_head_init(&xmitq);
 	tipc_uaddr(&ua, TIPC_SERVICE_RANGE, TIPC_NODE_SCOPE,
 		   TIPC_LINK_STATE, n->addr, n->addr);
 	sk.ref = n->link_id;
 	sk.node = tipc_own_addr(net);
 	node = n->addr;
 	bearer_id = n->link_id & 0xffff;
-	publ_list = &n->publ_list;
-
-	n->action_flags &= ~(TIPC_NOTIFY_NODE_DOWN | TIPC_NOTIFY_NODE_UP |
-			     TIPC_NOTIFY_LINK_DOWN | TIPC_NOTIFY_LINK_UP);
 
-	write_unlock_bh(&n->lock);
+	if (n->action_flags & TIPC_NOTIFY_NODE_DOWN)
+		tipc_publ_notify(net, &n->publ_list, n->capabilities);
 
-	if (flags & TIPC_NOTIFY_NODE_DOWN)
-		tipc_publ_notify(net, publ_list, node, n->capabilities);
+	if (n->action_flags & TIPC_NOTIFY_NODE_UP)
+		tipc_named_node_up(net, node, n->capabilities, &xmitq);
 
-	if (flags & TIPC_NOTIFY_NODE_UP)
-		tipc_named_node_up(net, node, n->capabilities);
-
-	if (flags & TIPC_NOTIFY_LINK_UP) {
+	if (n->action_flags & TIPC_NOTIFY_LINK_UP) {
 		tipc_mon_peer_up(net, node, bearer_id);
-		tipc_nametbl_publish(net, &ua, &sk, sk.ref);
+		tipc_nametbl_publish(net, &ua, &sk, sk.ref, &skb, &rc_dests);
 	}
-	if (flags & TIPC_NOTIFY_LINK_DOWN) {
+	if (n->action_flags & TIPC_NOTIFY_LINK_DOWN) {
 		tipc_mon_peer_down(net, node, bearer_id);
-		tipc_nametbl_withdraw(net, &ua, &sk, sk.ref);
+		tipc_nametbl_withdraw(net, &ua, &sk, sk.ref, &skb, &rc_dests);
 	}
+
+	n->action_flags &= ~(TIPC_NOTIFY_NODE_DOWN | TIPC_NOTIFY_NODE_UP |
+			     TIPC_NOTIFY_LINK_DOWN | TIPC_NOTIFY_LINK_UP);
+	write_unlock_bh(&n->lock);
+
+	if (!skb_queue_empty(&xmitq))
+		tipc_node_xmit(net, &xmitq, node, 0);
+
+	if (skb)
+		tipc_node_broadcast(net, skb, rc_dests);
 }
 
 static void tipc_node_assign_peer_net(struct tipc_node *n, u32 hash_mixes)
@@ -653,40 +659,14 @@ void tipc_node_stop(struct net *net)
 	spin_unlock_bh(&tn->node_list_lock);
 }
 
-void tipc_node_subscribe(struct net *net, struct list_head *subscr, u32 addr)
+void tipc_node_subscribe(struct list_head *subscr, struct list_head *publ_list)
 {
-	struct tipc_node *n;
-
-	if (in_own_node(net, addr))
-		return;
-
-	n = tipc_node_find(net, addr);
-	if (!n) {
-		pr_warn("Node subscribe rejected, unknown node 0x%x\n", addr);
-		return;
-	}
-	tipc_node_write_lock(n);
-	list_add_tail(subscr, &n->publ_list);
-	tipc_node_write_unlock_fast(n);
-	tipc_node_put(n);
+	list_add_tail(subscr, publ_list);
 }
 
-void tipc_node_unsubscribe(struct net *net, struct list_head *subscr, u32 addr)
+void tipc_node_unsubscribe(struct list_head *subscr)
 {
-	struct tipc_node *n;
-
-	if (in_own_node(net, addr))
-		return;
-
-	n = tipc_node_find(net, addr);
-	if (!n) {
-		pr_warn("Node unsubscribe rejected, unknown node 0x%x\n", addr);
-		return;
-	}
-	tipc_node_write_lock(n);
 	list_del_init(subscr);
-	tipc_node_write_unlock_fast(n);
-	tipc_node_put(n);
 }
 
 int tipc_node_add_conn(struct net *net, u32 dnode, u32 port, u32 peer_port)
@@ -1917,10 +1897,14 @@ static void tipc_node_bc_rcv(struct net *net, struct sk_buff *skb, int bearer_id
 		tipc_node_mcast_rcv(n);
 
 	/* Handle NAME_DISTRIBUTOR messages sent from 1.7 nodes */
-	if (!skb_queue_empty(&n->bc_entry.namedq))
+	if (!skb_queue_empty(&n->bc_entry.namedq)) {
+		tipc_node_write_lock(n);
 		tipc_named_rcv(net, &n->bc_entry.namedq,
+			       &n->publ_list,
 			       &n->bc_entry.named_rcv_nxt,
 			       &n->bc_entry.named_open);
+		tipc_node_write_unlock_fast(n);
+	}
 
 	/* If reassembly or retransmission failure => reset all links to peer */
 	if (rc & TIPC_LINK_DOWN_EVT)
@@ -2198,10 +2182,14 @@ void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
 	if (unlikely(rc & TIPC_LINK_DOWN_EVT))
 		tipc_node_link_down(n, bearer_id, false);
 
-	if (unlikely(!skb_queue_empty(&n->bc_entry.namedq)))
+	if (unlikely(!skb_queue_empty(&n->bc_entry.namedq))) {
+		tipc_node_write_lock(n);
 		tipc_named_rcv(net, &n->bc_entry.namedq,
+			       &n->publ_list,
 			       &n->bc_entry.named_rcv_nxt,
 			       &n->bc_entry.named_open);
+		tipc_node_write_unlock_fast(n);
+	}
 
 	if (unlikely(!skb_queue_empty(&n->bc_entry.inputq1)))
 		tipc_node_mcast_rcv(n);
diff --git a/net/tipc/node.h b/net/tipc/node.h
index 154a5bbb0d29..3599e48457bb 100644
--- a/net/tipc/node.h
+++ b/net/tipc/node.h
@@ -103,8 +103,8 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head *list, u32 dnode,
 int tipc_node_distr_xmit(struct net *net, struct sk_buff_head *list);
 int tipc_node_xmit_skb(struct net *net, struct sk_buff *skb, u32 dest,
 		       u32 selector);
-void tipc_node_subscribe(struct net *net, struct list_head *subscr, u32 addr);
-void tipc_node_unsubscribe(struct net *net, struct list_head *subscr, u32 addr);
+void tipc_node_subscribe(struct list_head *subscr, struct list_head *publ_list);
+void tipc_node_unsubscribe(struct list_head *subscr);
 void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests);
 int tipc_node_add_conn(struct net *net, u32 dnode, u32 port, u32 peer_port);
 void tipc_node_remove_conn(struct net *net, u32 dnode, u32 port);
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index d5d70eb230b5..dda247db0e60 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2906,23 +2906,27 @@ static void tipc_sk_timeout(struct timer_list *t)
 
 static int tipc_sk_publish(struct tipc_sock *tsk, struct tipc_uaddr *ua)
 {
-	struct sock *sk = &tsk->sk;
-	struct net *net = sock_net(sk);
+	struct net *net = sock_net(&tsk->sk);
 	struct tipc_socket_addr skaddr;
+	struct sk_buff *skb = NULL;
 	struct publication *p;
+	u32 rc_dests;
 	u32 key;
 
-	if (tipc_sk_connected(sk))
+	if (tipc_sk_connected(&tsk->sk))
 		return -EINVAL;
 	key = tsk->portid + tsk->pub_count + 1;
 	if (key == tsk->portid)
 		return -EADDRINUSE;
 	skaddr.ref = tsk->portid;
 	skaddr.node = tipc_own_addr(net);
-	p = tipc_nametbl_publish(net, ua, &skaddr, key);
+	p = tipc_nametbl_publish(net, ua, &skaddr, key, &skb, &rc_dests);
 	if (unlikely(!p))
 		return -EINVAL;
 
+	if (skb)
+		tipc_node_broadcast(net, skb, rc_dests);
+
 	list_add(&p->binding_sock, &tsk->publications);
 	tsk->pub_count++;
 	tsk->published = true;
@@ -2934,13 +2938,19 @@ static int tipc_sk_withdraw(struct tipc_sock *tsk, struct tipc_uaddr *ua)
 	struct net *net = sock_net(&tsk->sk);
 	struct publication *safe, *p;
 	struct tipc_uaddr _ua;
+	struct sk_buff *skb;
 	int rc = -EINVAL;
+	u32 rc_dests;
 
 	list_for_each_entry_safe(p, safe, &tsk->publications, binding_sock) {
+		skb = NULL;
 		if (!ua) {
 			tipc_uaddr(&_ua, TIPC_SERVICE_RANGE, p->scope,
 				   p->sr.type, p->sr.lower, p->sr.upper);
-			tipc_nametbl_withdraw(net, &_ua, &p->sk, p->key);
+			tipc_nametbl_withdraw(net, &_ua, &p->sk, p->key,
+					      &skb, &rc_dests);
+			if (skb)
+				tipc_node_broadcast(net, skb, rc_dests);
 			continue;
 		}
 		/* Unbind specific publication */
@@ -2952,7 +2962,9 @@ static int tipc_sk_withdraw(struct tipc_sock *tsk, struct tipc_uaddr *ua)
 			continue;
 		if (p->sr.upper != ua->sr.upper)
 			break;
-		tipc_nametbl_withdraw(net, ua, &p->sk, p->key);
+		tipc_nametbl_withdraw(net, ua, &p->sk, p->key, &skb, &rc_dests);
+		if (skb)
+			tipc_node_broadcast(net, skb, rc_dests);
 		rc = 0;
 		break;
 	}
-- 
2.43.0

Re: [PATCH net v2] tipc: fix several race conditions caused by tipc_node_write_unlock()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2] tipc: fix several race conditions caused by tipc_node_write_unlock()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 (1) · High: 1 (1) · Medium: 1 · Low: 2

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 patch changes which node's publ_list a remote publication
  goes on, but does not check that the message's orignode matches that…
- [Medium] Moving tipc_node_xmit() out of tipc_named_node_up() means the
  bulk snapshot for a new peer is no longer sent atomically with respect…
- [Low] The first race in the commit message does not match the code.
- [Low] The patch edits the kernel-doc for tipc_update_nametbl() and
  tipc_named_rcv() to add @publ_list but leaves the locking contract…

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but the patch
  rewrites the affected call and changes how it fails.
- [High] named_distribute() dereferences NULL when nt->cluster_scope is
  empty.

--

tipc: fix several race conditions caused by tipc_node_write_unlock()

This commit keeps the node write lock held while tipc_node_write_unlock()
handles link and node up/down events. It also moves publ_list locking out
of tipc_node_subscribe()/tipc_node_unsubscribe() and into the callers of
tipc_named_rcv(). Name distribution messages built under the lock are now
sent after the lock is dropped.

Closes: https://lore.kernel.org/netdev/20261001182924.3928331-2-nicoyip.dev@gmail.com/
Closes: https://lore.kernel.org/netdev/20261001182924.3928331-3-nicoyip.dev@gmail.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202610060355.bPzTfoXZ-lkp@intel.com/

> - A race between link-up and link-down events can occur when the
> link-down thread is interrupted before removing a publication from
> nt->cluster_scope, and the link-up event inserts a publication into
> nt->cluster_scope.

[Severity: Low]
Is this description accurate? tipc_node_write_unlock() builds the
link-state publication with node scope:

	tipc_uaddr(&ua, TIPC_SERVICE_RANGE, TIPC_NODE_SCOPE,
		   TIPC_LINK_STATE, n->addr, n->addr);

tipc_named_publish() puts node scope publications on nt->node_scope, not
on nt->cluster_scope:

	if (p->scope == TIPC_NODE_SCOPE) {
		list_add_tail_rcu(&p->binding_node, &nt->node_scope);
		return NULL;
	}

The insert in tipc_nametbl_publish() and the removal in
tipc_nametbl_withdraw() also both run under tn->nametbl_lock.

Releasing the node lock early could reorder link-up and link-down
handling and leave a stale link-state entry in node_scope. That looks
different from the cluster_scope use-after-free described here.

Could the commit message describe the race that change 1 actually
addresses?

> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
> index ba4f4906e13b7..dcb15de42a9ee 100644
> --- a/net/tipc/name_distr.c
> +++ b/net/tipc/name_distr.c

[ ... ]

> @@ -218,8 +218,7 @@ void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
>  	spin_unlock_bh(&tn->nametbl_lock);
>  
>  	read_lock_bh(&nt->cluster_scope_lock);
> -	named_distribute(net, &head, dnode, &nt->cluster_scope, seqno);
> -	tipc_node_xmit(net, &head, dnode, 0);
> +	named_distribute(net, xmitq, dnode, &nt->cluster_scope, seqno);
>  	read_unlock_bh(&nt->cluster_scope_lock);
>  }

[Severity: Medium]
Can a withdrawal now overtake the bulk that is sent to a newly up peer?

Before this change, cluster_scope_lock was held for reading across both
named_distribute() and tipc_node_xmit(). tipc_named_withdraw() needs the
write side of that lock for list_del(&p->binding_node). So a withdrawal
either happened before the snapshot, or its WITHDRAWAL was built after
the bulk was already queued on the link.

Now the lock is dropped right after named_distribute(), and
tipc_node_write_unlock() sends the bulk only after the node lock is
released:

	write_unlock_bh(&n->lock);

	if (!skb_queue_empty(&xmitq))
		tipc_node_xmit(net, &xmitq, node, 0);

One possible ordering:

CPU0 (link up)
tipc_node_write_unlock()
  tipc_named_node_up()
    named_distribute()          <- publication P copied into bulk B
    read_unlock_bh(&nt->cluster_scope_lock)

CPU1 (unbind or close)
tipc_sk_withdraw()
  tipc_nametbl_withdraw()
    tipc_named_withdraw()       <- P removed, WITHDRAWAL W built
  tipc_node_broadcast()
    tipc_node_xmit_skb()
      tipc_node_xmit()          <- waits in tipc_node_read_lock(n)

CPU0
  write_unlock_bh(&n->lock)     <- CPU1 now enqueues W
  tipc_node_xmit(&xmitq)        <- B enqueued after W

In replicast mode, tipc_node_broadcast() already sends W to the new peer,
because node_is_up(n) is already true. Replicast mode means rc_dests > 0,
which tipc_named_node_up() itself sets for a peer without
TIPC_NAMED_BCAST, or BCLINK_MODE_RCAST. Both messages use selector 0.

The peer would treat W as a no-op and then install P from B. That leaves
a stale binding to a port that no longer exists until this node goes
down. A peer with TIPC_NAMED_BCAST could be affected the same way when
its bc_entry.named_open is stale.

[ ... ]

> @@ -272,12 +270,14 @@ void tipc_publ_notify(struct net *net, struct list_head *nsub_list,
>   * @i: location of item in the message
>   * @node: node address
>   * @dtype: name distributor message type
> + * @publ_list: list of remote publications of a specific node
>   *
>   * tipc_nametbl_lock must be held.
>   * Return: the publication item if successful, otherwise NULL.
>   */
>  static bool tipc_update_nametbl(struct net *net, struct distr_item *i,
> -				u32 node, u32 dtype)
> +				u32 node, u32 dtype,
> +				struct list_head *publ_list)

[Severity: Low]
This isn't a bug, but tipc_node_subscribe() and tipc_node_unsubscribe()
are now just list_add_tail() and list_del_init(). The only thing
protecting publ_list is the caller holding the owning node's write lock.

Should this comment, and the new @publ_list text for tipc_named_rcv(),
state that requirement as well as tipc_nametbl_lock?

The Return: line in this block also describes a publication pointer,
but tipc_update_nametbl() is declared bool.

>  {
>  	struct publication *p = NULL;
>  	u32 lower = ntohl(i->lower);
> @@ -301,13 +301,13 @@ static bool tipc_update_nametbl(struct net *net, struct distr_item *i,
>  	if (dtype == PUBLICATION) {
>  		p = tipc_nametbl_insert_publ(net, &ua, &sk, key);
>  		if (p) {
> -			tipc_node_subscribe(net, &p->binding_node, node);
> +			tipc_node_subscribe(&p->binding_node, publ_list);
>  			return true;
>  		}
>  	} else if (dtype == WITHDRAWAL) {
>  		p = tipc_nametbl_remove_publ(net, &ua, &sk, key);
>  		if (p) {
> -			tipc_node_unsubscribe(net, &p->binding_node, node);
> +			tipc_node_unsubscribe(&p->binding_node);
>  			kfree_rcu(p, rcu);
>  			return true;
>  		}

[Severity: High]
Is publ_list guaranteed to belong to the node that the item names?

tipc_named_rcv() takes node from the message header:

	node = msg_orignode(hdr);

But publ_list is &n->publ_list of the node that tipc_rcv() or
tipc_node_bc_rcv() found via msg_prevnode() (or destnode). That is also
the node whose lock is held:

	tipc_node_write_lock(n);
	tipc_named_rcv(net, &n->bc_entry.namedq,
		       &n->publ_list,
		       ...

Nothing compares orignode with n->addr. tipc_data_input() also queues
NAME_DISTRIBUTOR messages to namedq without checking it.

Before this patch, tipc_node_subscribe() and tipc_node_unsubscribe() looked
up the node by orignode and took that node's lock. A publication with
sk.node == Y was therefore always on Y->publ_list and only changed under
Y->lock.

Now a peer N can send a WITHDRAWAL with orignode == Y. It can learn the
type, range, port and key of Y's publications from normal name
distribution. tipc_nametbl_remove_publ() then matches Y's publication,
and list_del_init() unlinks it from Y->publ_list while only N->lock and
nametbl_lock are held.

Can that race with a node-down walk of Y->publ_list?

CPU0
tipc_node_write_unlock(Y)          <- holds Y->lock, NODE_DOWN set
  tipc_publ_notify(&Y->publ_list)
    list_for_each_entry_safe(publ, tmp, ...)
      tipc_publ_purge(publ)        <- nametbl_lock per entry only

CPU1
tipc_rcv(N)                        <- holds N->lock only
  tipc_named_rcv(&N->publ_list)
    tipc_update_nametbl(WITHDRAWAL, node = Y)
      list_del_init(&tmp->binding_node)

If the unlinked entry is CPU0's saved tmp, it now points to itself. Then
tipc_publ_purge(tmp) finds nothing and the loop never ends, with BH
disabled and Y->lock held for write.

This looks like the same publ_list race that the commit message says is
fixed.

With in_own_node() removed, a PUBLICATION whose orignode is the local
address or 0 is also linked onto the peer's publ_list now.

Could n->addr be passed down so that items whose orignode does not match
it are dropped?

[ ... ]

> diff --git a/net/tipc/net.c b/net/tipc/net.c
> index 7e65d0b0c4a8d..1e445c5abc0f5 100644
> --- a/net/tipc/net.c
> +++ b/net/tipc/net.c

[ ... ]

> @@ -138,7 +140,9 @@ static void tipc_net_finalize(struct net *net, u32 addr)
>  	tipc_named_reinit(net);
>  	tipc_sk_reinit(net);
>  	tipc_mon_reinit_self(net);
> -	tipc_nametbl_publish(net, &ua, &sk, addr);
> +	tipc_nametbl_publish(net, &ua, &sk, addr, &skb, &rc_dests);
> +	if (skb)
> +		tipc_node_broadcast(net, skb, rc_dests);
>  }

[Severity: High]
This is a pre-existing issue, but what happens if tipc_nametbl_publish()
returns NULL here? That can happen when the GFP_ATOMIC allocation in
tipc_publ_create() fails.

At that point the node address has already been set by cmpxchg() and
tipc_set_node_addr(). The cluster scope TIPC_NODE_STATE binding is what
normally keeps nt->cluster_scope non-empty.

If there are no user cluster scope bindings, named_distribute() then
runs on an empty list when the first peer comes up:

tipc_node_write_unlock()
  tipc_named_node_up()
    named_distribute()

It still does this unconditionally:

	hdr = buf_msg(skb_peek_tail(list));

skb_peek_tail() returns NULL for an empty queue, so buf_msg() would
dereference NULL. The new skb_queue_empty(&xmitq) check in
tipc_node_write_unlock() runs after this point and does not help.

> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index d7cbfa786c132..182c6dbfa49d2 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c

[ ... ]

> @@ -653,40 +659,14 @@ void tipc_node_stop(struct net *net)

[ ... ]

> -void tipc_node_unsubscribe(struct net *net, struct list_head *subscr, u32 addr)
> +void tipc_node_unsubscribe(struct list_head *subscr)
>  {
> -	struct tipc_node *n;
> -
> -	if (in_own_node(net, addr))
> -		return;
> -
> -	n = tipc_node_find(net, addr);
> -	if (!n) {
> -		pr_warn("Node unsubscribe rejected, unknown node 0x%x\n", addr);
> -		return;
> -	}
> -	tipc_node_write_lock(n);
>  	list_del_init(subscr);
> -	tipc_node_write_unlock_fast(n);
> -	tipc_node_put(n);
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but removing in_own_node()
changes how it plays out. Can a peer free a local publication by sending
a WITHDRAWAL with orignode set to 0 or to our own address?

tipc_service_remove_publ() treats node 0 as a wildcard and does not check
scope:

	if (p->key != key || p->sk.ref != sk->ref ||
	    (node && node != p->sk.node))
		continue;

The type, range, port and key of cluster scope publications are known
from normal name distribution. So tipc_update_nametbl() can kfree_rcu() a
publication owned by a local socket. Its p->binding_sock stays on
tsk->publications, and nt->local_publ_count is not decremented.

After the grace period, tipc_sk_withdraw() on unbind or close walks
tsk->publications and reads the freed p->scope and p->sr. In addition,
list_add(&p->binding_sock, &tsk->publications) in tipc_sk_publish()
writes into the freed neighbour.

Before this patch, in_own_node() made tipc_node_unsubscribe() return
early, so p->binding_node stayed on nt->cluster_scope after the free.
Without that check, list_del_init() now removes it from nt->cluster_scope
(or from the RCU list nt->node_scope) without cluster_scope_lock and
without list_del_rcu(). That races with named_distribute() and with
tipc_named_publish()/tipc_named_withdraw().

Should orignode be validated before these items are applied to the name
table?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006052312.30382-1-tung.quang.nguyen%40est.tech