Re: [PATCH net 1/2] tipc: fix self-deadlock in tipc_named_node_up

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 2 (2) · 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).

- [Medium] At the target commit, the -ENOBUFS recovery for bulk name
  distribution can reset a link other than the one that failed.
- [Medium] tipc: leak of cloned skbs in xmitq on -ENOBUFS in
  tipc_node_distribute()
- [Low] The commit message leaves out several behaviour changes and
  misdescribes the new helper.
- [Low] The new kernel-doc for tipc_node_distribute() lists the return
  value as `-ENOBUF` (net/tipc/node.c:1798).

Pre-existing issues:
- [High] named_distribute() runs `hdr = buf_msg(skb_peek_tail(list));`
  (name_distr.c:191) whether or not anything was queued.
- [Medium] At the target commit named_distribute() is still `static
  void`.
- [Medium] This problem wasn't introduced by this patch, but it is copied
  into the new code.

--

tipc: fix self-deadlock in tipc_named_node_up

This patch removes nt->cluster_scope_lock and uses tn->nametbl_lock to
protect the cluster_scope list. That breaks a self-deadlock in which
tipc_named_node_up() held the read lock across tipc_node_xmit() ->
tipc_node_link_down() -> tipc_nametbl_withdraw().

It adds tipc_node_distribute(), which transmits like tipc_node_xmit() but
does not reset the link, plus a shared helper, tipc_node_build_list().
tipc_named_node_up() now returns an error code, and
tipc_node_write_unlock() resets the link when that code is -ENOBUFS.

> Fix this issue by:
> - Removing cluster_scope_lock, which protects the name table's cluster_scope
>   list. Use nametbl_lock instead to protect the list against races when multiple
>   threads concurrently call tipc_named_publish(), tipc_named_withdraw(),
>   and tipc_named_node_up().
>
> - Introducing a new function, tipc_node_distribute(), which is similar to
>   tipc_node_xmit() but does not call tipc_node_link_down(). Both functions call
>   the newly introduced tipc_node_build_list(), which builds a list of messages
>   in sequence.

[Severity: Low]
Could the commit message cover a few more of the behaviour changes in this
patch?

The link reset on bulk failure is still there. It moved into
tipc_node_write_unlock():

	if (rc == -ENOBUFS)
		tipc_node_link_down(n, bearer_id, false);

tipc_named_node_up() now also maps -EMSGSIZE to -ENOBUFS, so an oversized
bulk now resets the link. Before this patch, tipc_node_xmit() returned
-EMSGSIZE and the void tipc_named_node_up() ignored it. The -ELINKCONG
mapping has a comment, but this one does not.

The message says nametbl_lock now protects the cluster_scope list. In fact
the whole transmit now runs under spin_lock_bh(&tn->nametbl_lock):
tipc_node_find(), the node read lock, le->lock, tipc_link_xmit(), and
tipc_bearer_xmit() all the way down to the media send_msg().

Also, tipc_node_build_list() does more than build a list of messages. It
either delivers to the peer namespace through tipc_lxc_xmit() or enqueues
on the link through tipc_link_xmit().

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

[ ... ]

> @@ -202,25 +198,39 @@ 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
> + *
> + * Return:
> + * * 0               - Success
> + * * -ENOBUFS        - No buffer space is available
> + * * -EHOSTUNREACH   - Destination node does not exist or cannot be reached
> + *
>   */
> -void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
> +int tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
>  {
>  	struct name_table *nt = tipc_name_table(net);
>  	struct tipc_net *tn = tipc_net(net);
>  	struct sk_buff_head head;
> -	u16 seqno;
> +	int bearer_id = 0;
> +	int rc = 0;
>  
>  	__skb_queue_head_init(&head);
>  	spin_lock_bh(&tn->nametbl_lock);
>  	if (!(capabilities & TIPC_NAMED_BCAST))
>  		nt->rc_dests++;
> -	seqno = nt->snd_nxt;
> +	named_distribute(net, &head, dnode, &nt->cluster_scope, nt->snd_nxt);
> +	if (!skb_queue_empty(&head))
> +		rc = tipc_node_distribute(net, &head, dnode, &bearer_id, 0);

[Severity: High]
This is a pre-existing issue, but can named_distribute() dereference NULL
here when nt->cluster_scope is empty?

named_distribute() runs this whether or not it queued anything:

	hdr = buf_msg(skb_peek_tail(list));
	msg_set_last_bulk(hdr);

The list can be empty here. That happens if the TIPC_NOTIFY_NODE_UP event
is handled before tipc_net_finalize() has published the self
TIPC_NODE_STATE name, or if that publish failed. skb_peek_tail() then
returns NULL, and buf_msg(NULL) is dereferenced in softirq context.

A remote peer bringing up a link can trigger this. A syzbot report shows
the path tipc_rcv() -> tipc_node_write_unlock() -> tipc_named_node_up().

The new skb_queue_empty() check only runs after the dereference.

The follow-up patch "tipc: fix NULL pointer dereference in
named_distribute()" appears to fix this. It routes the call through
__tipc_named_distribute(), which makes sure the self publication is on
nt->cluster_scope, or defers the bulk, before named_distribute() runs.

[Severity: Medium]
This isn't a bug introduced by this patch, but the new kernel-doc says
tipc_named_node_up() returns -ENOBUFS when no buffer space is available.
Can that happen for allocation failures?

named_distribute() is still void. When named_prepare_buf() fails, it
returns without purging the partial list and without setting last_bulk or
the seqno:

			if (!skb) {
				pr_warn("Bulk publication failure\n");
				return;
			}

rc is only set from tipc_node_distribute(), so this case returns 0. The
new -ENOBUFS recovery in tipc_node_write_unlock() never runs. The peer's
tipc_named_dequeue() never sees last_bulk, so the peer's name table stays
stale.

The follow-up patch "tipc: fix NULL pointer dereference in
named_distribute()" seems to fix this. There, named_distribute() purges
the list and returns -ENOBUFS, and tipc_named_node_up() passes that
error up.

[ ... ]

> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 53b0993c2c233..b19d6652dc731 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c

[ ... ]

> @@ -435,6 +436,9 @@ static void tipc_node_write_unlock(struct tipc_node *n)
>  		tipc_mon_peer_down(net, node, bearer_id);
>  		tipc_nametbl_withdraw(net, &ua, &sk, sk.ref);
>  	}
> +
> +	if (rc == -ENOBUFS)
> +		tipc_node_link_down(n, bearer_id, false);

[Severity: Medium]
Can this reset a different link from the one the bulk failed on?

Here bearer_id comes from n->link_id. It is saved before n->lock is
dropped:

	bearer_id = n->link_id & 0xffff;

The bulk is sent on whichever bearer tipc_node_build_list() picks later,
in a separate lock section:

	*bearer_id = n->active_links[selector & 1];

tipc_named_node_up() receives that bearer from tipc_node_distribute() in
its local bearer_id, then discards it and returns only rc.

Suppose a higher priority second link comes up in that window.
__tipc_node_link_up() puts the new bearer in both active slots, and the
bulk then fails on the new link. The old, healthy link is the one that
gets reset, and the bulk is never resent. The old tipc_node_xmit() path
reset the bearer it had actually used.

In the follow-up patch "tipc: fix NULL pointer dereference in
named_distribute()" this call becomes tipc_node_down(n), which brings
down every bearer. So the problem seems to be gone by the end of the
series.

[ ... ]

> @@ -1715,42 +1763,66 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head *list,

[ ... ]

> +/**
> + * tipc_node_distribute() - distribute list of messages to specific destination
> + * @net: the applicable net namespace
> + * @list: chain of buffers containing message
> + * @dnode: address of destination node
> + * @bearer_id: id of the bearer
> + * @selector: a number used for deterministic link selection
> + *
> + * Return:
> + * * 0              - Success
> + * * -ELINKCONG     - Link is congested
> + * * -EHOSTUNREACH  - Host is not reachable
> + * * -EMSGSIZE      - Message's size is too big
> + * * -ENOBUF        - No buffer space is available

[Severity: Low]
This isn't a bug, but -ENOBUF should be -ENOBUFS. The typo seems to have
been copied from the tipc_node_xmit() comment, and it is still there at
the end of the series.

> + *
> + */
> +int tipc_node_distribute(struct net *net, struct sk_buff_head *list,
> +			 u32 dnode, int *bearer_id, int selector)
> +{

[ ... ]

>  	__skb_queue_head_init(&xmitq);
> -	le = &n->links[bearer_id];
> -	spin_lock_bh(&le->lock);
> -	rc = tipc_link_xmit(le->link, list, &xmitq);
> -	spin_unlock_bh(&le->lock);
> -	tipc_node_read_unlock(n);
> +	rc = tipc_node_build_list(n, list, &xmitq, selector, &maddr,
> +				  bearer_id, &lxc);
> +	if (lxc) {
> +		tipc_node_put(n);
> +		return 0;
> +	}
>  
> -	if (unlikely(rc == -ENOBUFS))
> -		tipc_node_link_down(n, bearer_id, false);
> -	else
> -		tipc_bearer_xmit(net, bearer_id, &xmitq, &le->maddr, n);
> +	if (likely(rc != -ENOBUFS))
> +		tipc_bearer_xmit(net, *bearer_id, &xmitq, &maddr, n);

[Severity: Medium]
This is a pre-existing issue, but can the clones already queued on
xmitq leak when tipc_link_xmit() returns -ENOBUFS here?

tipc_link_xmit() takes each skb off list, puts it on l->transmq and
puts a skb_clone() of it on the caller's xmitq. Suppose skb_clone()
fails on a later skb in the list. It frees that skb, purges list and
returns -ENOBUFS, but the clones from the earlier iterations stay on
xmitq.

On -ENOBUFS, tipc_node_distribute() skips tipc_bearer_xmit() and
returns. Nothing ever purges the stack-local xmitq, so those clones
are lost.

The refactored tipc_node_xmit() has the same problem. It calls
tipc_node_link_down() instead. That only cleans up the link's own
queues, not the caller's xmitq.

This needs a multi-buffer list, for example a bulk from
named_distribute() when nt->cluster_scope spans more than one MTU. It
also needs a GFP_ATOMIC failure in softirq context, so it is most
likely under memory pressure.

Both callers now go through tipc_node_build_list(). Would it make
sense to purge xmitq there on -ENOBUFS? Or should tipc_link_xmit()
clean up the clones it queued before returning the error?

[Severity: Medium]
This is a pre-existing issue, but are the skb clones on xmitq leaked when
rc is -ENOBUFS?

tipc_link_xmit() clones each skb onto xmitq. If skb_clone() fails on a
later skb, it returns -ENOBUFS and the earlier clones stay queued:

			_skb = skb_clone(skb, GFP_ATOMIC);
			if (!_skb) {
				kfree_skb(skb);
				__skb_queue_purge(list);
				return -ENOBUFS;
			}

On -ENOBUFS this function skips tipc_bearer_xmit() and never purges the
stack-local xmitq. The refactored tipc_node_xmit() does the same, as did
the original version.

Would a __skb_queue_purge(xmitq) on -ENOBUFS in tipc_node_build_list()
cover both callers? The leak is still present at the end of the series.

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