[PATCH net] tipc: serialize publication purging with name table updates

[PATCH net] tipc: serialize publication purging with name table updates

[Chengfeng Ye]

tipc_publ_notify() walks a failed node's publication list after the node
lock has been released. Its list iterator is not protected by the name
table lock, which is only acquired inside tipc_publ_purge().

CPU A can save the next publication before entering tipc_publ_purge().
CPU B then takes nametbl_lock in tipc_named_rcv(), processes a WITHDRAWAL
for that publication, unlinks it with list_del_init(), and queues it for
freeing with kfree_rcu(). CPU A advances to the removed publication and
loops on its self-linked binding_node, stalling the CPU.

The kernel reported:

  rcu: INFO: rcu_sched self-detected stall on CPU
  Call Trace:
   tipc_publ_notify+0x3b5/0x650
   tipc_node_write_unlock+0x49d/0x5d0
   tipc_node_link_down+0x15c/0x4a0
   tipc_node_delete_links+0xfc/0x190
   bearer_disable+0x111/0x270
   __tipc_nl_bearer_disable+0x1db/0x2f0
   tipc_nl_bearer_disable+0x1c/0x30

Locking the entire traversal would also prevent the race, but would hold
nametbl_lock with bottom halves disabled while purging every publication.
A node with many publications could therefore cause excessive lock hold
times and delay other name-table operations.

Move the failed node's publications to a private list under nametbl_lock,
then select and unlink its first entry under the same lock before purging
it. Concurrent withdrawals can still unlink entries from this private
list, while publications arriving after node recovery stay on the node's
live list. No publication pointer is carried across an unlocked interval.

Unlink before the table lookup to make progress even if the lookup fails,
and acquire and release the lock for each publication. Check the private
list under the lock on every iteration, since concurrent withdrawals can
still remove entries. Keep withdrawal notifications and the final
rc_dests update in their existing order.

Remove the failed-node address argument from tipc_publ_notify() and its
purge helper since unlinking no longer needs a node lookup.

Fixes: 9db9fdd1983e ("tipc: avoid to asynchronously notify subscriptions")
Cc: stable@vger.kernel.org
Assisted-by: GPT-6-Astra
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/tipc/name_distr.c | 34 +++++++++++++++++++++++-----------
 net/tipc/name_distr.h |  2 +-
 net/tipc/node.c       |  2 +-
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index ba4f4906e13b..34867b69044c 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -227,38 +227,50 @@ 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.
+ * The caller must hold nametbl_lock and unlink the node subscription.
  */
-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;
 	struct tipc_uaddr ua;
 
 	tipc_uaddr(&ua, TIPC_SERVICE_RANGE, p->scope, p->sr.type,
 		   p->sr.lower, p->sr.upper);
-	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);
-	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);
 
-	struct publication *publ, *tmp;
+	struct publication *publ;
+	LIST_HEAD(purge_list);
+
+	spin_lock_bh(&tn->nametbl_lock);
+	/* Leave new publications on the node's list during the purge. */
+	list_splice_init(nsub_list, &purge_list);
+	spin_unlock_bh(&tn->nametbl_lock);
+
+	for (;;) {
+		spin_lock_bh(&tn->nametbl_lock);
+		if (list_empty(&purge_list)) {
+			spin_unlock_bh(&tn->nametbl_lock);
+			break;
+		}
+		publ = list_first_entry(&purge_list, struct publication,
+					binding_node);
+		list_del_init(&publ->binding_node);
+		tipc_publ_purge(net, publ);
+		spin_unlock_bh(&tn->nametbl_lock);
+	}
 
-	list_for_each_entry_safe(publ, tmp, nsub_list, binding_node)
-		tipc_publ_purge(net, publ, addr);
 	spin_lock_bh(&tn->nametbl_lock);
 	if (!(capabilities & TIPC_NAMED_BCAST))
 		nt->rc_dests--;
diff --git a/net/tipc/name_distr.h b/net/tipc/name_distr.h
index c677f6f082df..8debe23469b2 100644
--- a/net/tipc/name_distr.h
+++ b/net/tipc/name_distr.h
@@ -74,6 +74,6 @@ void tipc_named_rcv(struct net *net, struct sk_buff_head *namedq,
 		    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/node.c b/net/tipc/node.c
index bd91378b7540..9a218d137c45 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -422,7 +422,7 @@ static void tipc_node_write_unlock(struct tipc_node *n)
 	write_unlock_bh(&n->lock);
 
 	if (flags & TIPC_NOTIFY_NODE_DOWN)
-		tipc_publ_notify(net, publ_list, node, n->capabilities);
+		tipc_publ_notify(net, publ_list, n->capabilities);
 
 	if (flags & TIPC_NOTIFY_NODE_UP)
 		tipc_named_node_up(net, node, n->capabilities);
-- 
2.43.0

RE: [PATCH net] tipc: serialize publication purging with name table updates

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: serialize publication purging with name table updates
>
>tipc_publ_notify() walks a failed node's publication list after the node lock has
>been released. Its list iterator is not protected by the name table lock, which is
>only acquired inside tipc_publ_purge().
>
>CPU A can save the next publication before entering tipc_publ_purge().
>CPU B then takes nametbl_lock in tipc_named_rcv(), processes a
>WITHDRAWAL for that publication, unlinks it with list_del_init(), and queues it
>for freeing with kfree_rcu(). CPU A advances to the removed publication and
>loops on its self-linked binding_node, stalling the CPU.
>
>The kernel reported:
>
>  rcu: INFO: rcu_sched self-detected stall on CPU
>  Call Trace:
>   tipc_publ_notify+0x3b5/0x650
>   tipc_node_write_unlock+0x49d/0x5d0
>   tipc_node_link_down+0x15c/0x4a0
>   tipc_node_delete_links+0xfc/0x190
>   bearer_disable+0x111/0x270
>   __tipc_nl_bearer_disable+0x1db/0x2f0
>   tipc_nl_bearer_disable+0x1c/0x30
>

Please update your changelog with decoded stack trace.

>Locking the entire traversal would also prevent the race, but would hold
>nametbl_lock with bottom halves disabled while purging every publication.
>A node with many publications could therefore cause excessive lock hold times
>and delay other name-table operations.
>
>Move the failed node's publications to a private list under nametbl_lock, then
>select and unlink its first entry under the same lock before purging it.
>Concurrent withdrawals can still unlink entries from this private list, while
>publications arriving after node recovery stay on the node's live list. No
>publication pointer is carried across an unlocked interval.
>
>Unlink before the table lookup to make progress even if the lookup fails, and
>acquire and release the lock for each publication. Check the private list under
>the lock on every iteration, since concurrent withdrawals can still remove
>entries. Keep withdrawal notifications and the final rc_dests update in their
>existing order.
>
>Remove the failed-node address argument from tipc_publ_notify() and its
>purge helper since unlinking no longer needs a node lookup.
>
>Fixes: 9db9fdd1983e ("tipc: avoid to asynchronously notify subscriptions")
>Cc: stable@vger.kernel.org
>Assisted-by: GPT-6-Astra
>Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
>---
> net/tipc/name_distr.c | 34 +++++++++++++++++++++++-----------
> net/tipc/name_distr.h |  2 +-
> net/tipc/node.c       |  2 +-
> 3 files changed, 25 insertions(+), 13 deletions(-)
>
>diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c index
>ba4f4906e13b..34867b69044c 100644
>--- a/net/tipc/name_distr.c
>+++ b/net/tipc/name_distr.c
>@@ -227,38 +227,50 @@ 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.
>+ * The caller must hold nametbl_lock and unlink the node subscription.
>  */
>-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;
> 	struct tipc_uaddr ua;
>
> 	tipc_uaddr(&ua, TIPC_SERVICE_RANGE, p->scope, p->sr.type,
> 		   p->sr.lower, p->sr.upper);
>-	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);
>-	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);
>
>-	struct publication *publ, *tmp;
>+	struct publication *publ;
>+	LIST_HEAD(purge_list);
>+
>+	spin_lock_bh(&tn->nametbl_lock);
>+	/* Leave new publications on the node's list during the purge. */
>+	list_splice_init(nsub_list, &purge_list);
>+	spin_unlock_bh(&tn->nametbl_lock);

After this lock is released, new publications can be inserted into node->publ_list. Does this defeat the purpose of current publication release ?

>+
>+	for (;;) {
>+		spin_lock_bh(&tn->nametbl_lock);
>+		if (list_empty(&purge_list)) {
>+			spin_unlock_bh(&tn->nametbl_lock);
>+			break;
>+		}
>+		publ = list_first_entry(&purge_list, struct publication,
>+					binding_node);
>+		list_del_init(&publ->binding_node);
>+		tipc_publ_purge(net, publ);
>+		spin_unlock_bh(&tn->nametbl_lock);
>+	}
>
>-	list_for_each_entry_safe(publ, tmp, nsub_list, binding_node)
>-		tipc_publ_purge(net, publ, addr);
> 	spin_lock_bh(&tn->nametbl_lock);
> 	if (!(capabilities & TIPC_NAMED_BCAST))
> 		nt->rc_dests--;
>diff --git a/net/tipc/name_distr.h b/net/tipc/name_distr.h index
>c677f6f082df..8debe23469b2 100644
>--- a/net/tipc/name_distr.h
>+++ b/net/tipc/name_distr.h
>@@ -74,6 +74,6 @@ void tipc_named_rcv(struct net *net, struct sk_buff_head
>*namedq,
> 		    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/node.c b/net/tipc/node.c index
>bd91378b7540..9a218d137c45 100644
>--- a/net/tipc/node.c
>+++ b/net/tipc/node.c
>@@ -422,7 +422,7 @@ static void tipc_node_write_unlock(struct tipc_node
>*n)
> 	write_unlock_bh(&n->lock);
>
> 	if (flags & TIPC_NOTIFY_NODE_DOWN)
>-		tipc_publ_notify(net, publ_list, node, n->capabilities);
>+		tipc_publ_notify(net, publ_list, n->capabilities);
>
> 	if (flags & TIPC_NOTIFY_NODE_UP)
> 		tipc_named_node_up(net, node, n->capabilities);
>--
>2.43.0

Re: [PATCH net] tipc: serialize publication purging with name table updates

[netdev-bot+sashiko]

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

Critical: 1 · High: 0 · Medium: 0 · Low: 0

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).

- [Critical] In tipc_update_nametbl() (net/tipc/name_distr.c), the
  WITHDRAWAL branch frees a publication with kfree_rcu(p, rcu) whether…

--

tipc: serialize publication purging with name table updates

This patch changes tipc_publ_notify() so that it moves a failed node's
publications onto a private on-stack list while holding nametbl_lock. It
then unlinks and purges them one at a time, taking and releasing the lock
for each entry. The addr argument is dropped from tipc_publ_notify() and
tipc_publ_purge() because the unlink no longer needs a node lookup.

> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
> index ba4f4906e13b7..34867b69044c0 100644
> --- a/net/tipc/name_distr.c
> +++ b/net/tipc/name_distr.c
> @@ -227,38 +227,50 @@ void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)

[ ... ]

>  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);
>  
> -	struct publication *publ, *tmp;
> +	struct publication *publ;
> +	LIST_HEAD(purge_list);
> +
> +	spin_lock_bh(&tn->nametbl_lock);
> +	/* Leave new publications on the node's list during the purge. */
> +	list_splice_init(nsub_list, &purge_list);
> +	spin_unlock_bh(&tn->nametbl_lock);
> +
> +	for (;;) {
> +		spin_lock_bh(&tn->nametbl_lock);
> +		if (list_empty(&purge_list)) {
> +			spin_unlock_bh(&tn->nametbl_lock);
> +			break;
> +		}
> +		publ = list_first_entry(&purge_list, struct publication,
> +					binding_node);
> +		list_del_init(&publ->binding_node);
> +		tipc_publ_purge(net, publ);
> +		spin_unlock_bh(&tn->nametbl_lock);
> +	}

[Severity: Critical]

The commit message says "Concurrent withdrawals can still unlink entries
from this private list". Is that always the case?

The WITHDRAWAL branch in tipc_update_nametbl() frees the publication even
when binding_node was never unlinked:

net/tipc/name_distr.c:tipc_update_nametbl() {
    ...
	} else if (dtype == WITHDRAWAL) {
		p = tipc_nametbl_remove_publ(net, &ua, &sk, key);
		if (p) {
			tipc_node_unsubscribe(net, &p->binding_node, node);
			kfree_rcu(p, rcu);
			return true;
		}
    ...
}

tipc_node_unsubscribe() returns without calling list_del_init() in two
cases:

net/tipc/node.c:tipc_node_unsubscribe() {
	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;
	}
    ...
}

In both cases the freed publication stays linked on n->publ_list. This
loop then splices it into purge_list, writes to it with list_del_init(),
and reads p->scope, p->sr, p->sk and p->key in tipc_publ_purge().

The lock is dropped and BH is re-enabled between iterations, so a grace
period can end in the middle of the loop. Can a later iteration then touch
a publication that kfree_rcu() has already freed?

One way to make tipc_node_find() fail while the purge is running:

CPU B: tipc_named_rcv() holds nametbl_lock and has already dequeued a
       WITHDRAWAL from node N (node_lost_contact() only purges what is
       still left in bc_entry.namedq)
CPU A: tipc_node_write_unlock()->tipc_publ_notify() waits on
       nametbl_lock
CPU C: tipc_nl_peer_rm()->tipc_node_delete() removes N from the hash
       once N is SELF_DOWN_PEER_LEAVING or SELF_DOWN_PEER_DOWN
CPU B: tipc_update_nametbl()->tipc_node_unsubscribe() fails the lookup,
       then kfree_rcu(p) runs with p still on N->publ_list
CPU A: list_splice_init() moves p onto purge_list, and a later iteration
       accesses p

A second path does not depend on the purge race. tipc_named_rcv() takes
the node from the header the peer sends, and nothing validates it:

		node = msg_orignode(hdr);

tipc_service_remove_publ() treats node == 0 as a wildcard:

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

A peer with a link could send a WITHDRAWAL with orignode 0 that matches a
publication from node X. That publication is removed and freed. Because
in_own_node(net, 0) is true, tipc_node_unsubscribe() skips the unlink, and
the freed entry stays on X->publ_list.

>From then on, list_add_tail() in tipc_node_subscribe(), list_del_init()
on neighbouring entries, and the splice and walk here when X goes down
would all operate on freed memory. The spoofing itself predates this
patch, but it produces the same state that this loop assumes cannot
happen.

Could the WITHDRAWAL branch always do list_del_init(&p->binding_node)
under nametbl_lock? Every writer of binding_node for remote publications
already holds that lock.

Separately, could tipc_named_rcv() reject an orignode of 0 or our own
address, or require it to match the owner of the removed publication?
Without that check, a spoofed withdrawal can also free local publications
that are still on the socket and cluster_scope lists.

The kernel-doc for struct publication in net/tipc/name_table.h also still
says:

 * - Remote publications: in node->publ_list;

While a purge is running, binding_node is on the on-stack purge_list.
Should that comment be updated too?

>  
> -	list_for_each_entry_safe(publ, tmp, nsub_list, binding_node)
> -		tipc_publ_purge(net, publ, addr);
>  	spin_lock_bh(&tn->nametbl_lock);
>  	if (!(capabilities & TIPC_NAMED_BCAST))
>  		nt->rc_dests--;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927180806.1315902-1-nicoyip.dev%40gmail.com

[PATCH net v2 0/2] tipc: fix publication lifetime races

[Chengfeng Ye]

Two races can leave the publication lists referring to objects with an
invalid lifetime.

First, tipc_node_unsubscribe() looks up the publishing node before
unlinking a publication. If the node has already been removed from the
hash, the lookup fails and the caller frees the publication while its
binding_node remains linked.

Second, tipc_publ_notify() retains the next publication from a failed
node's list across an unlocked interval. A concurrent withdrawal can
unlink and schedule that publication for freeing before the purge
iterator advances to it.

Patch 1 unlinks successfully removed remote publications directly under
nametbl_lock. Patch 2 moves the failed node's publications to a private
list and selects each publication under the same lock, so no publication
pointer is retained across an unlocked interval.

Changes in v2:
- Split the original fix into two patches.
- Add patch 1/2 to address the unlink-before-free issue reported by
  Sashiko and remove the now-unused tipc_node_unsubscribe() helper.
- Add the decoded causal call trace requested by Tung Quang Nguyen.
- Explain the ordering of name-table updates around the node-down
  publication snapshot.

v1:
https://lore.kernel.org/netdev/20260927180806.1315902-1-nicoyip.dev@gmail.com/

Chengfeng Ye (2):
  tipc: unlink publications without a node lookup
  tipc: serialize publication purging with name table updates

 net/tipc/name_distr.c | 36 ++++++++++++++++++++++++------------
 net/tipc/name_distr.h |  2 +-
 net/tipc/node.c       | 20 +-------------------
 net/tipc/node.h       |  1 -
 4 files changed, 26 insertions(+), 33 deletions(-)

-- 
2.43.0

[PATCH net v2 1/2] tipc: unlink publications without a node lookup

[Chengfeng Ye]

The two remote-publication removal paths remove a publication from the
name table and call tipc_node_unsubscribe() before scheduling it for
freeing.

tipc_node_unsubscribe() looks up the publishing node by address and returns
without unlinking when the node has already disappeared from the hash. The
publication is then freed while its binding_node remains linked, so a later
publication-list traversal can access freed memory.

Both paths already run under nametbl_lock. Unlink binding_node directly
under that lock instead of performing another node lookup. For a valid
remote publication, binding_node is either on the node publication list or
is initialized as an empty list when subscription failed. Remove the
now-unused helper and address arguments.

Fixes: a8f48af587b0 ("tipc: remove node subscription infrastructure")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/tipc/name_distr.c | 11 +++++------
 net/tipc/name_distr.h |  2 +-
 net/tipc/node.c       | 20 +-------------------
 net/tipc/node.h       |  1 -
 4 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index ba4f4906e13b..acf96562608b 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -227,12 +227,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 +242,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);
+		list_del_init(&_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 +257,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--;
@@ -307,7 +306,7 @@ static bool tipc_update_nametbl(struct net *net, struct distr_item *i,
 	} else if (dtype == WITHDRAWAL) {
 		p = tipc_nametbl_remove_publ(net, &ua, &sk, key);
 		if (p) {
-			tipc_node_unsubscribe(net, &p->binding_node, node);
+			list_del_init(&p->binding_node);
 			kfree_rcu(p, rcu);
 			return true;
 		}
diff --git a/net/tipc/name_distr.h b/net/tipc/name_distr.h
index c677f6f082df..8debe23469b2 100644
--- a/net/tipc/name_distr.h
+++ b/net/tipc/name_distr.h
@@ -74,6 +74,6 @@ void tipc_named_rcv(struct net *net, struct sk_buff_head *namedq,
 		    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/node.c b/net/tipc/node.c
index bd91378b7540..0e333f952c4f 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -422,7 +422,7 @@ static void tipc_node_write_unlock(struct tipc_node *n)
 	write_unlock_bh(&n->lock);
 
 	if (flags & TIPC_NOTIFY_NODE_DOWN)
-		tipc_publ_notify(net, publ_list, node, n->capabilities);
+		tipc_publ_notify(net, publ_list, n->capabilities);
 
 	if (flags & TIPC_NOTIFY_NODE_UP)
 		tipc_named_node_up(net, node, n->capabilities);
@@ -671,24 +671,6 @@ void tipc_node_subscribe(struct net *net, struct list_head *subscr, u32 addr)
 	tipc_node_put(n);
 }
 
-void tipc_node_unsubscribe(struct net *net, struct list_head *subscr, u32 addr)
-{
-	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)
 {
 	struct tipc_node *node;
diff --git a/net/tipc/node.h b/net/tipc/node.h
index 154a5bbb0d29..a5f060b622a7 100644
--- a/net/tipc/node.h
+++ b/net/tipc/node.h
@@ -104,7 +104,6 @@ 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_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);
-- 
2.43.0

[PATCH net v2 2/2] tipc: serialize publication purging with name table updates

[Chengfeng Ye]

tipc_publ_notify() walks a failed node publication list after the node lock
has been released. Its safe iterator is not protected by nametbl_lock,
which is acquired only inside tipc_publ_purge().

A concurrent withdrawal can unlink and schedule the saved next publication
for freeing. The purge iterator then advances to that removed publication.
It may access freed memory after the RCU grace period, or repeatedly follow
the self-linked binding_node before then.

The decoded causal stack is:

  tipc_nametbl_remove_publ net/tipc/name_table.c:543
  tipc_publ_purge          net/tipc/name_distr.c:244
  tipc_publ_notify         net/tipc/name_distr.c:261
  tipc_node_write_unlock   net/tipc/node.c:425
  tipc_node_link_down      net/tipc/node.c:1094
  tipc_node_delete_links   net/tipc/node.c:1325
  bearer_disable           net/tipc/bearer.c:414
  __tipc_nl_bearer_disable net/tipc/bearer.c:992
  tipc_nl_bearer_disable   net/tipc/bearer.c:1002

Move the failed node publications to a private list under nametbl_lock.
Select, unlink and purge one publication during each lock acquisition, so
no publication pointer is retained across an unlocked interval. Concurrent
withdrawals can remove entries from the private list under the same lock.

Holding the lock for the whole purge would keep bottom halves disabled
while removing every publication. Releasing it after each entry avoids
an excessive lock hold for nodes with many publications.

node_lost_contact() purges queued name-table updates before scheduling the
node-down notification. An update already dequeued by tipc_named_rcv()
holds nametbl_lock until it updates the publication list, so it completes
before the snapshot and is included. A publication accepted after the
snapshot remains on the live node list for a later contact.

Fixes: 9db9fdd1983e ("tipc: avoid to asynchronously notify subscriptions")
Cc: stable@vger.kernel.org

Link: https://lore.kernel.org/netdev/20260927180806.1315902-1-nicoyip.dev@gmail.com/
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/tipc/name_distr.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index acf96562608b..9a400a1fa4d7 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -230,20 +230,16 @@ void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
  *
  * Invoked for each publication issued by a newly failed node.
  * Removes publication structure from name table & deletes it.
+ * The caller must hold nametbl_lock and unlink the node subscription.
  */
 static void tipc_publ_purge(struct net *net, struct publication *p)
 {
-	struct tipc_net *tn = tipc_net(net);
 	struct publication *_p;
 	struct tipc_uaddr ua;
 
 	tipc_uaddr(&ua, TIPC_SERVICE_RANGE, p->scope, p->sr.type,
 		   p->sr.lower, p->sr.upper);
-	spin_lock_bh(&tn->nametbl_lock);
 	_p = tipc_nametbl_remove_publ(net, &ua, &p->sk, p->key);
-	if (_p)
-		list_del_init(&_p->binding_node);
-	spin_unlock_bh(&tn->nametbl_lock);
 	if (_p)
 		kfree_rcu(_p, rcu);
 }
@@ -254,10 +250,27 @@ void tipc_publ_notify(struct net *net, struct list_head *nsub_list,
 	struct name_table *nt = tipc_name_table(net);
 	struct tipc_net *tn = tipc_net(net);
 
-	struct publication *publ, *tmp;
+	struct publication *publ;
+	LIST_HEAD(purge_list);
 
-	list_for_each_entry_safe(publ, tmp, nsub_list, binding_node)
+	spin_lock_bh(&tn->nametbl_lock);
+	/* Preserve publications learned after this node-down snapshot. */
+	list_splice_init(nsub_list, &purge_list);
+	spin_unlock_bh(&tn->nametbl_lock);
+
+	for (;;) {
+		spin_lock_bh(&tn->nametbl_lock);
+		if (list_empty(&purge_list)) {
+			spin_unlock_bh(&tn->nametbl_lock);
+			break;
+		}
+		publ = list_first_entry(&purge_list, struct publication,
+					binding_node);
+		list_del_init(&publ->binding_node);
 		tipc_publ_purge(net, publ);
+		spin_unlock_bh(&tn->nametbl_lock);
+	}
+
 	spin_lock_bh(&tn->nametbl_lock);
 	if (!(capabilities & TIPC_NAMED_BCAST))
 		nt->rc_dests--;
-- 
2.43.0

Re: [PATCH net v2 0/2] tipc: fix publication lifetime races

[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] tipc: serialize publication purging with name table updates

[Chengfeng Ye]

On Tue, Sep 29, 2026 at 7:14 PM Tung Quang Nguyen
<tung.quang.nguyen@est.tech> wrote:
>
> >Subject: [PATCH net] tipc: serialize publication purging with name table updates
> >
> >tipc_publ_notify() walks a failed node's publication list after the node lock has
> >been released. Its list iterator is not protected by the name table lock, which is
> >only acquired inside tipc_publ_purge().
> >
> >CPU A can save the next publication before entering tipc_publ_purge().
> >CPU B then takes nametbl_lock in tipc_named_rcv(), processes a
> >WITHDRAWAL for that publication, unlinks it with list_del_init(), and queues it
> >for freeing with kfree_rcu(). CPU A advances to the removed publication and
> >loops on its self-linked binding_node, stalling the CPU.
> >
> >The kernel reported:
> >
> >  rcu: INFO: rcu_sched self-detected stall on CPU
> >  Call Trace:
> >   tipc_publ_notify+0x3b5/0x650
> >   tipc_node_write_unlock+0x49d/0x5d0
> >   tipc_node_link_down+0x15c/0x4a0
> >   tipc_node_delete_links+0xfc/0x190
> >   bearer_disable+0x111/0x270
> >   __tipc_nl_bearer_disable+0x1db/0x2f0
> >   tipc_nl_bearer_disable+0x1c/0x30
> >
>
> Please update your changelog with decoded stack trace.

Updated in v2.

> >Locking the entire traversal would also prevent the race, but would hold
> >nametbl_lock with bottom halves disabled while purging every publication.
> >A node with many publications could therefore cause excessive lock hold times
> >and delay other name-table operations.
> >
> >Move the failed node's publications to a private list under nametbl_lock, then
> >select and unlink its first entry under the same lock before purging it.
> >Concurrent withdrawals can still unlink entries from this private list, while
> >publications arriving after node recovery stay on the node's live list. No
> >publication pointer is carried across an unlocked interval.
> >
> >Unlink before the table lookup to make progress even if the lookup fails, and
> >acquire and release the lock for each publication. Check the private list under
> >the lock on every iteration, since concurrent withdrawals can still remove
> >entries. Keep withdrawal notifications and the final rc_dests update in their
> >existing order.
> >
> >Remove the failed-node address argument from tipc_publ_notify() and its
> >purge helper since unlinking no longer needs a node lookup.
> >
> >Fixes: 9db9fdd1983e ("tipc: avoid to asynchronously notify subscriptions")
> >Cc: stable@vger.kernel.org
> >Assisted-by: GPT-6-Astra
> >Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> >---
> > net/tipc/name_distr.c | 34 +++++++++++++++++++++++-----------
> > net/tipc/name_distr.h |  2 +-
> > net/tipc/node.c       |  2 +-
> > 3 files changed, 25 insertions(+), 13 deletions(-)
> >
> >diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c index
> >ba4f4906e13b..34867b69044c 100644
> >--- a/net/tipc/name_distr.c
> >+++ b/net/tipc/name_distr.c
> >@@ -227,38 +227,50 @@ 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.
> >+ * The caller must hold nametbl_lock and unlink the node subscription.
> >  */
> >-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;
> >       struct tipc_uaddr ua;
> >
> >       tipc_uaddr(&ua, TIPC_SERVICE_RANGE, p->scope, p->sr.type,
> >                  p->sr.lower, p->sr.upper);
> >-      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);
> >-      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);
> >
> >-      struct publication *publ, *tmp;
> >+      struct publication *publ;
> >+      LIST_HEAD(purge_list);
> >+
> >+      spin_lock_bh(&tn->nametbl_lock);
> >+      /* Leave new publications on the node's list during the purge. */
> >+      list_splice_init(nsub_list, &purge_list);
> >+      spin_unlock_bh(&tn->nametbl_lock);
>
> After this lock is released, new publications can be inserted into node->publ_list. Does this defeat the purpose of current publication release ?

I think probably no. I understand that tipc_publ_notify() as to remove
the publications associated with the contact that has just failed. The
original traversal did not provide deterministic behavior for such
concurrent insertions. Since publications are appended with
list_add_tail(), list_for_each_entry_safe() could either reach a newly
inserted publication or miss it, depending on when the iterator saved
its next entry. The private list makes the node-down cleanup boundary
explicit.

> >+
> >+      for (;;) {
> >+              spin_lock_bh(&tn->nametbl_lock);
> >+              if (list_empty(&purge_list)) {
> >+                      spin_unlock_bh(&tn->nametbl_lock);
> >+                      break;
> >+              }
> >+              publ = list_first_entry(&purge_list, struct publication,
> >+                                      binding_node);
> >+              list_del_init(&publ->binding_node);
> >+              tipc_publ_purge(net, publ);
> >+              spin_unlock_bh(&tn->nametbl_lock);
> >+      }
> >
> >-      list_for_each_entry_safe(publ, tmp, nsub_list, binding_node)
> >-              tipc_publ_purge(net, publ, addr);
> >       spin_lock_bh(&tn->nametbl_lock);
> >       if (!(capabilities & TIPC_NAMED_BCAST))
> >               nt->rc_dests--;
> >diff --git a/net/tipc/name_distr.h b/net/tipc/name_distr.h index
> >c677f6f082df..8debe23469b2 100644
> >--- a/net/tipc/name_distr.h
> >+++ b/net/tipc/name_distr.h
> >@@ -74,6 +74,6 @@ void tipc_named_rcv(struct net *net, struct sk_buff_head
> >*namedq,
> >                   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/node.c b/net/tipc/node.c index
> >bd91378b7540..9a218d137c45 100644
> >--- a/net/tipc/node.c
> >+++ b/net/tipc/node.c
> >@@ -422,7 +422,7 @@ static void tipc_node_write_unlock(struct tipc_node
> >*n)
> >       write_unlock_bh(&n->lock);
> >
> >       if (flags & TIPC_NOTIFY_NODE_DOWN)
> >-              tipc_publ_notify(net, publ_list, node, n->capabilities);
> >+              tipc_publ_notify(net, publ_list, n->capabilities);
> >
> >       if (flags & TIPC_NOTIFY_NODE_UP)
> >               tipc_named_node_up(net, node, n->capabilities);
> >--
> >2.43.0
>

The node-lookup failure reported by sashiko is pre-existing and was
not introduced by the patch, but the patch relies on the affected
list-membership invariant, and the original publication release
implementation is also affected.

Since the race addressed in this patch and reported by sashiko are of
different root cause, I address both cases in v2 with a two-patch
series. Patch 1 replaces tipc_node_unsubscribe() with
list_del_init(&p->binding_node) while nametbl_lock is held and before
kfree_rcu(). Consequently, a failed node lookup can no longer cause a
remote publication to be freed while it remains on either
node->publ_list or the private purge list. Patch 2 fixes the problem
addressed by this v1 patch.

https://lore.kernel.org/netdev/20261001182924.3928331-1-nicoyip.dev@gmail.com/

Best regards,
Chengfeng

RE: [PATCH net v2 1/2] tipc: unlink publications without a node lookup

[Tung Quang Nguyen]

>Subject: [PATCH net v2 1/2] tipc: unlink publications without a node lookup
>
>The two remote-publication removal paths remove a publication from the
>name table and call tipc_node_unsubscribe() before scheduling it for freeing.
>
>tipc_node_unsubscribe() looks up the publishing node by address and returns
>without unlinking when the node has already disappeared from the hash. The
>publication is then freed while its binding_node remains linked, so a later
>publication-list traversal can access freed memory.
>
>Both paths already run under nametbl_lock. Unlink binding_node directly
>under that lock instead of performing another node lookup. For a valid remote
>publication, binding_node is either on the node publication list or is initialized
>as an empty list when subscription failed. Remove the now-unused helper and
>address arguments.
>

This does not fix the root cause of this issue: Not holding proper lock in tipc_node_write_unlock().
I will post fix for this issue. Thanks for your report.

RE: [PATCH net v2 2/2] tipc: serialize publication purging with name table updates

[Tung Quang Nguyen]

>Subject: [PATCH net v2 2/2] tipc: serialize publication purging with name table
>updates
>
>tipc_publ_notify() walks a failed node publication list after the node lock has
>been released. Its safe iterator is not protected by nametbl_lock, which is
>acquired only inside tipc_publ_purge().
>
>A concurrent withdrawal can unlink and schedule the saved next publication
>for freeing. The purge iterator then advances to that removed publication.
>It may access freed memory after the RCU grace period, or repeatedly follow
>the self-linked binding_node before then.
>
>The decoded causal stack is:
>
>  tipc_nametbl_remove_publ net/tipc/name_table.c:543
>  tipc_publ_purge          net/tipc/name_distr.c:244
>  tipc_publ_notify         net/tipc/name_distr.c:261
>  tipc_node_write_unlock   net/tipc/node.c:425
>  tipc_node_link_down      net/tipc/node.c:1094
>  tipc_node_delete_links   net/tipc/node.c:1325
>  bearer_disable           net/tipc/bearer.c:414
>  __tipc_nl_bearer_disable net/tipc/bearer.c:992
>  tipc_nl_bearer_disable   net/tipc/bearer.c:1002
>
>Move the failed node publications to a private list under nametbl_lock.
>Select, unlink and purge one publication during each lock acquisition, so no
>publication pointer is retained across an unlocked interval. Concurrent
>withdrawals can remove entries from the private list under the same lock.
>
>Holding the lock for the whole purge would keep bottom halves disabled while
>removing every publication. Releasing it after each entry avoids an excessive
>lock hold for nodes with many publications.
>
>node_lost_contact() purges queued name-table updates before scheduling the
>node-down notification. An update already dequeued by tipc_named_rcv()
>holds nametbl_lock until it updates the publication list, so it completes before
>the snapshot and is included. A publication accepted after the snapshot
>remains on the live node list for a later contact.
>

This does not fix the root cause of this issue: Not holding proper lock in tipc_node_write_unlock().
I will post fix for this issue. Thanks for your report.

Re: [PATCH net v2 1/2] tipc: unlink publications without a node lookup

[Chengfeng Ye]

On Mon, Oct 5, 2026 at 12:08 PM Tung Quang Nguyen
<tung.quang.nguyen@est.tech> wrote:
>
> >Subject: [PATCH net v2 1/2] tipc: unlink publications without a node lookup
> >
> >The two remote-publication removal paths remove a publication from the
> >name table and call tipc_node_unsubscribe() before scheduling it for freeing.
> >
> >tipc_node_unsubscribe() looks up the publishing node by address and returns
> >without unlinking when the node has already disappeared from the hash. The
> >publication is then freed while its binding_node remains linked, so a later
> >publication-list traversal can access freed memory.
> >
> >Both paths already run under nametbl_lock. Unlink binding_node directly
> >under that lock instead of performing another node lookup. For a valid remote
> >publication, binding_node is either on the node publication list or is initialized
> >as an empty list when subscription failed. Remove the now-unused helper and
> >address arguments.
> >
>
> This does not fix the root cause of this issue: Not holding proper lock in tipc_node_write_unlock().
> I will post fix for this issue. Thanks for your report.

Thanks for your help on it.

Best regards,
Chengfeng

Re: [PATCH net v2 1/2] tipc: unlink publications without a node lookup

[netdev-bot+sashiko]

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

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

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).

- [Critical] The patch replaces tipc_node_unsubscribe() with an
  unconditional list_del_init(&p->binding_node) in tipc_update_nametbl()…
- [High] The WITHDRAWAL branch of tipc_update_nametbl() now calls
  list_del_init(&p->binding_node) on whatever publication…

Pre-existing issues:
- [High] At this commit, tipc_publ_notify() still walks n->publ_list with
  list_for_each_entry_safe() while holding no lock.
- [Low] The kernel-doc of tipc_update_nametbl() says "Return: the
  publication item if successful, otherwise NULL.", but the function is…

--

tipc: unlink publications without a node lookup

This commit replaces tipc_node_unsubscribe() in tipc_publ_purge() and in
the WITHDRAWAL path of tipc_update_nametbl() with a direct
list_del_init(&p->binding_node) done under nametbl_lock. The node lookup
helper and the now-unused address arguments are removed.

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

[ ... ]

> @@ -243,14 +242,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);
> +		list_del_init(&_p->binding_node);
>  	spin_unlock_bh(&tn->nametbl_lock);
>  	if (_p)
>  		kfree_rcu(_p, rcu);
>  }

[ ... ]

> @@ -258,7 +257,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);

[Severity: High]
This is a pre-existing issue, but tipc_publ_notify() still walks
n->publ_list here without holding any lock. nametbl_lock is only taken
inside tipc_publ_purge(), one entry at a time.

Can a concurrent WITHDRAWAL unlink and free the entry the iterator has
cached in tmp?

tipc_named_rcv()
  tipc_update_nametbl()
    list_del_init(&p->binding_node);
    kfree_rcu(p, rcu);

tipc_publ_purge() would then read that entry's fields before it takes
nametbl_lock, which is a use-after-free once the grace period has passed.
Before then, the self-linked binding_node means the loop never gets back
to the list head.

The old unlink ran under n->lock, and this traversal didn't hold that lock
either, so the race existed before this patch. It appears to be fixed
later in the series by "tipc: serialize publication purging with name
table updates". That commit splices nsub_list onto a private list under
nametbl_lock and then pops one entry per lock acquisition.

[ ... ]

> @@ -307,7 +306,7 @@ static bool tipc_update_nametbl(struct net *net, struct distr_item *i,
>  	} else if (dtype == WITHDRAWAL) {
>  		p = tipc_nametbl_remove_publ(net, &ua, &sk, key);
>  		if (p) {
> -			tipc_node_unsubscribe(net, &p->binding_node, node);
> +			list_del_init(&p->binding_node);
>  			kfree_rcu(p, rcu);
>  			return true;
>  		}

[Severity: Critical]
Can this list_del_init() write into a freed struct tipc_node?

The new unlink assumes that the list head binding_node is linked to is
still alive. Node teardown doesn't appear to drain n->publ_list first:

tipc_node_timeout()
  tipc_node_cleanup()
    tipc_node_delete_from_list()
      list_del_rcu(&node->list);
      hlist_del_rcu(&node->hash);
      tipc_node_put(node);

tipc_node_kref_release()
  kfree(n->bc_entry.link);
  call_rcu(&n->rcu, tipc_node_free);

A publication can also be linked onto a node that is down but still in
the hash. tipc_named_rcv() takes node = msg_orignode(hdr) straight from
the peer's header. tipc_node_subscribe() only checks that the node is in
the hash and does not look at its state:

	tipc_node_write_lock(n);
	list_add_tail(subscr, &n->publ_list);

That allows this sequence:

1. Peer C sends a PUBLICATION with orignode set to a down node B. The
   publication goes onto B->publ_list, and no later NODE_DOWN purge will
   remove it.

2. After NODE_CLEANUP_AFTER, tipc_node_cleanup() deletes B, and B is freed
   after a grace period. The stale publication's binding_node still points
   at &B->publ_list.

3. C sends a WITHDRAWAL with orignode B. tipc_nametbl_remove_publ()
   returns the publication, and list_del_init() writes next->prev and
   prev->next into the freed (or reallocated) tipc_node.

tipc_publ_purge() above uses the same unconditional unlink.

Before this patch, tipc_node_find(B) failed in tipc_node_unsubscribe()
and nothing was written. The commit message describes this exact case:

    tipc_node_unsubscribe() looks up the publishing node by address and returns
    without unlinking when the node has already disappeared from the hash.

The later patch "tipc: serialize publication purging with name table
updates" only reworks tipc_publ_notify(), so this seems to still be
present at the end of the series.

Would it be safer to detach or purge every entry on n->publ_list under
nametbl_lock before the node can be freed?

[Severity: High]
Is p guaranteed to be a remote publication here? The removed
tipc_node_unsubscribe() returned early via in_own_node(net, addr), which
is true for both the own address and 0. The new code has no equivalent
check.

The peer controls node through msg_orignode(), and
tipc_service_remove_publ() treats node == 0 as a wildcard:

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

A WITHDRAWAL with orignode 0 or the local address could therefore match
a local publication. The peer already has the type, lower, upper, port
and key values it needs from the PUBLICATION items it received.

For a local publication, binding_node is on nt->cluster_scope or
nt->node_scope. tipc_named_publish() and tipc_named_withdraw() modify that
list under cluster_scope_lock. named_distribute() walks it holding only
read_lock_bh(&nt->cluster_scope_lock), called from tipc_named_node_up().

Can this list_del_init(), done under nametbl_lock only, race with that
walk? A walker on this entry would follow the self-linked next pointer
and never reach the head. It would keep packing the same item with BH
disabled.

kfree_rcu() then runs on the publication while it is still linked on the
owning socket's tsk->publications list through binding_sock.
tipc_sk_withdraw() later walks that list on unbind or close and reads
p->scope, p->sr and p->sk from freed memory.

A remote withdrawal could already free a local publication before this
patch. The unlocked change to cluster_scope is new, though, and the commit
message's safety argument only covers "a valid remote publication".

[Severity: Low]
This is a pre-existing issue, but the kernel-doc for tipc_update_nametbl()
says:

 * Return: the publication item if successful, otherwise NULL.

The function is declared static bool and returns only true or false.
Since this patch already touches the function body, could the comment be
updated as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001182924.3928331-1-nicoyip.dev%40gmail.com

Re: [PATCH net v2 2/2] tipc: serialize publication purging with name table updates

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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).

- [Medium] tipc_publ_notify() (net/tipc/name_distr.c) now calls
  list_del_init(&publ->binding_node) on every selected publication…

--

tipc: serialize publication purging with name table updates

This patch moves a failed node's publications onto a private list while
holding nametbl_lock. It then unlinks and purges one publication per lock
acquisition, so no publication pointer is kept across an unlocked
interval.

Link: https://lore.kernel.org/netdev/20260927180806.1315902-1-nicoyip.dev@gmail.com/

> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
> index acf96562608b9..9a400a1fa4d72 100644
> --- a/net/tipc/name_distr.c
> +++ b/net/tipc/name_distr.c
> @@ -230,20 +230,16 @@ void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)

[ ... ]

>  	tipc_uaddr(&ua, TIPC_SERVICE_RANGE, p->scope, p->sr.type,
>  		   p->sr.lower, p->sr.upper);
> -	spin_lock_bh(&tn->nametbl_lock);
>  	_p = tipc_nametbl_remove_publ(net, &ua, &p->sk, p->key);
> -	if (_p)
> -		list_del_init(&_p->binding_node);
> -	spin_unlock_bh(&tn->nametbl_lock);
>  	if (_p)
>  		kfree_rcu(_p, rcu);
>  }
> @@ -254,10 +250,27 @@ void tipc_publ_notify(struct net *net, struct list_head *nsub_list,

[ ... ]

> +	for (;;) {
> +		spin_lock_bh(&tn->nametbl_lock);
> +		if (list_empty(&purge_list)) {
> +			spin_unlock_bh(&tn->nametbl_lock);
> +			break;
> +		}
> +		publ = list_first_entry(&purge_list, struct publication,
> +					binding_node);
> +		list_del_init(&publ->binding_node);
>  		tipc_publ_purge(net, publ);

[Severity: Medium]
What happens to publ here if tipc_nametbl_remove_publ() returns NULL
inside tipc_publ_purge()?

The old code only unlinked binding_node when removal succeeded:

	if (_p)
		list_del_init(&_p->binding_node);

A failed removal left the publication on n->publ_list, so a later
node-down purge would try it again. With this change, list_del_init() runs
before the removal is attempted. tipc_publ_purge() also drops a NULL
result silently.

So a publication that fails removal stays in the name table but is no
longer on any node publication list. Can it then be orphaned for good?

A failed removal looks reachable through an existing problem with
duplicate ranges. This part predates the patch. In
tipc_service_create_range(), equal lower bounds always go left:

	if (lower <= sr->lower)
		n = &parent->rb_left;
	else
		n = &parent->rb_right;

After a rotation, an exact match in a right subtree is missed. For one
service type, with every range published by the same remote node:

  1. Insert R1=[10,20] (pubA), then R2=[10,30] (pubB). R2 becomes the
     left child of R1.
  2. Insert R3=[10,40] (pubC). After the rebalance, R2 is the root, R3 is
     on its left and R1 is on its right.
  3. Insert pubD=[10,20]. The walk goes R2 -> R3 -> NULL and never
     reaches R1. A duplicate [10,20] range R4 is created as the left
     child of R3.
  4. n->publ_list holds pubA, pubB, pubC, pubD in that order, because
     tipc_node_subscribe() uses list_add_tail().

On node-down, the new loop picks pubA first and unlinks it. Then
tipc_service_find_range() returns the leftmost exact match, R4.
tipc_service_remove_publ(R4, skA, keyA) returns NULL because R4 only
holds pubD.

The loop goes on to purge pubB, pubC and pubD, and R4 is erased. pubA is
left in R1 and is no longer on any node list.

If the peer reconnects, tipc_service_insert_publ() rejects the
re-published pubA as a duplicate, so pubA never goes back on the node
list. If the peer then goes down without withdrawing it, pubA stays in
the name table until tipc_nametbl_stop().

Until then, lookups can still pick the dead node/port, and topology
subscribers never get TIPC_WITHDRAWN for it. The remote peer controls the
order through its bind() calls.

Could tipc_publ_purge() return whether the removal succeeded, so the
caller can keep or re-queue the publication when it fails?

> +		spin_unlock_bh(&tn->nametbl_lock);
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001182924.3928331-1-nicoyip.dev%40gmail.com