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

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

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/

quoted

- 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?

OK. I will change it.

quoted

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

[ ... ]

quoted

@@ -218,8 +218,7 @@ void tipc_named_node_up(struct net *net, u32

dnode, u16 capabilities)

quoted

 	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.

[ ... ]

Yes. Additional queue would be needed to address this. I will make the change in next version.

quoted

@@ -272,12 +270,14 @@ void tipc_publ_notify(struct net *net, struct

list_head *nsub_list,

quoted

  * @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.

Yes. This should be rewritten.

quoted

 {
 	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,

quoted

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

quoted

+			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?

Yes. I will address it in new version.

[ ... ]

quoted

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

[ ... ]

quoted

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

This is known issue. I will address via another patch.

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.

quoted

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

[ ... ]

quoted

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

[ ... ]

quoted

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

quoted

-		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?

No, the peer cannot send 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

--
pw-bot: cr