[PATCH net] tipc: hold a reference to nodes found by link name

[PATCH net] tipc: hold a reference to nodes found by link name

[Chengfeng Ye]

tipc_node_find_by_name() returns a node after dropping its RCU read lock
without taking a reference. The LINK_SET, LINK_GET and LINK_RESET_STATS
handlers then lock and access the node, racing with timer-driven cleanup
of a down peer. Generic netlink serialization does not exclude the node
timer.

The following interleaving can leave a handler using a freed node:

  CPU 0: find the node under RCU and release the node read lock
  CPU 1: tipc_node_timeout() clears the links and unlinks the down node
  CPU 1: drop the list and timer references, queuing tipc_node_free()
  CPU 0: leave the RCU read-side critical section
  CPU 1: complete the grace period and free the node
  CPU 0: acquire the node lock through the stale pointer

LINK_SET also uses the node's media address after releasing the node
lock, when passing queued packets to tipc_bearer_xmit().

KASAN reported:

  BUG: KASAN: slab-use-after-free in _raw_read_lock_bh+0x1d/0x40
  Write of size 4 at addr ffff888112723808 by task poc/87
  Call Trace:
   _raw_read_lock_bh+0x1d/0x40
   tipc_nl_node_set_link+0x30e/0x680
   genl_family_rcv_msg_doit+0x1e0/0x2c0
   genl_rcv_msg+0x419/0x6d0
   netlink_rcv_skb+0x11f/0x350
  Allocated by task 28:
   tipc_node_create+0x9c1/0x1fa0
   tipc_node_check_dest+0x121/0x11e0
   tipc_disc_rcv+0xdbf/0x1430
  Freed by task 87:
   kfree+0x149/0x330
   rcu_core+0x50a/0x1850
  Last potentially related work creation:
   __call_rcu_common.constprop.0+0x71/0xa10
   tipc_node_timeout+0xb1b/0xe70

Acquire a reference to the selected node with kref_get_unless_zero()
before leaving RCU, returning NULL if the node has already been released.
Release that reference on every caller exit after the last node access,
including transmission in LINK_SET. Keep the existing link lookup order
and locking so concurrent link removal still takes the existing error
paths.

Fixes: 6a939f365bdb ("tipc: Auto removal of peer down node instance")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/tipc/node.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index bd91378b7540..2726bee3bb40 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -2424,6 +2424,8 @@ static struct tipc_node *tipc_node_find_by_name(struct net *net,
 		if (found_node)
 			break;
 	}
+	if (found_node && !kref_get_unless_zero(&found_node->kref))
+		found_node = NULL;
 	rcu_read_unlock();
 
 	return found_node;
@@ -2507,6 +2509,7 @@ int tipc_nl_node_set_link(struct sk_buff *skb, struct genl_info *info)
 	tipc_node_read_unlock(node);
 	tipc_bearer_xmit(net, bearer_id, &xmitq, &node->links[bearer_id].maddr,
 			 NULL);
+	tipc_node_put(node);
 	return res;
 }
 
@@ -2558,12 +2561,14 @@ int tipc_nl_node_get_link(struct sk_buff *skb, struct genl_info *info)
 		link = node->links[bearer_id].link;
 		if (!link) {
 			tipc_node_read_unlock(node);
+			tipc_node_put(node);
 			err = -EINVAL;
 			goto err_free;
 		}
 
 		err = __tipc_nl_add_link(net, &msg, link, 0);
 		tipc_node_read_unlock(node);
+		tipc_node_put(node);
 		if (err)
 			goto err_free;
 	}
@@ -2634,11 +2639,13 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info)
 	if (!link) {
 		spin_unlock_bh(&le->lock);
 		tipc_node_read_unlock(node);
+		tipc_node_put(node);
 		return -EINVAL;
 	}
 	tipc_link_reset_stats(link);
 	spin_unlock_bh(&le->lock);
 	tipc_node_read_unlock(node);
+	tipc_node_put(node);
 	return 0;
 }
 
-- 
2.43.0

RE: [PATCH net] tipc: hold a reference to nodes found by link name

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: hold a reference to nodes found by link name
>
>tipc_node_find_by_name() returns a node after dropping its RCU read lock
>without taking a reference. The LINK_SET, LINK_GET and LINK_RESET_STATS
>handlers then lock and access the node, racing with timer-driven cleanup of a
>down peer. Generic netlink serialization does not exclude the node timer.
>
>The following interleaving can leave a handler using a freed node:
>
>  CPU 0: find the node under RCU and release the node read lock
>  CPU 1: tipc_node_timeout() clears the links and unlinks the down node
>  CPU 1: drop the list and timer references, queuing tipc_node_free()
>  CPU 0: leave the RCU read-side critical section
>  CPU 1: complete the grace period and free the node
>  CPU 0: acquire the node lock through the stale pointer
>
>LINK_SET also uses the node's media address after releasing the node lock,
>when passing queued packets to tipc_bearer_xmit().
>
>KASAN reported:
>
>  BUG: KASAN: slab-use-after-free in _raw_read_lock_bh+0x1d/0x40
>  Write of size 4 at addr ffff888112723808 by task poc/87
>  Call Trace:
>   _raw_read_lock_bh+0x1d/0x40
>   tipc_nl_node_set_link+0x30e/0x680
>   genl_family_rcv_msg_doit+0x1e0/0x2c0
>   genl_rcv_msg+0x419/0x6d0
>   netlink_rcv_skb+0x11f/0x350
>  Allocated by task 28:
>   tipc_node_create+0x9c1/0x1fa0
>   tipc_node_check_dest+0x121/0x11e0
>   tipc_disc_rcv+0xdbf/0x1430
>  Freed by task 87:
>   kfree+0x149/0x330
>   rcu_core+0x50a/0x1850
>  Last potentially related work creation:
>   __call_rcu_common.constprop.0+0x71/0xa10
>   tipc_node_timeout+0xb1b/0xe70
>

Can you update your changelog with decoded stack trace ?
With decoded stack trace, It helps me understand how your reproducer triggers the issue.

>Acquire a reference to the selected node with kref_get_unless_zero() before
>leaving RCU, returning NULL if the node has already been released.
>Release that reference on every caller exit after the last node access, including
>transmission in LINK_SET. Keep the existing link lookup order and locking so
>concurrent link removal still takes the existing error paths.
>
>Fixes: 6a939f365bdb ("tipc: Auto removal of peer down node instance")
>Cc: stable@vger.kernel.org
>Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
>---
> net/tipc/node.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/net/tipc/node.c b/net/tipc/node.c index
>bd91378b7540..2726bee3bb40 100644
>--- a/net/tipc/node.c
>+++ b/net/tipc/node.c
>@@ -2424,6 +2424,8 @@ static struct tipc_node
>*tipc_node_find_by_name(struct net *net,
> 		if (found_node)
> 			break;
> 	}
>+	if (found_node && !kref_get_unless_zero(&found_node->kref))
>+		found_node = NULL;

This checking is not optimal.
Try this:

diff --git a/net/tipc/node.c b/net/tipc/node.c
index bd91378b7540..3e61e9106dd7 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -2421,8 +2421,11 @@ static struct tipc_node *tipc_node_find_by_name(struct net *net,
                        }
                }
                tipc_node_read_unlock(n);
-               if (found_node)
+               if (found_node) {
+                       if (!kref_get_unless_zero(&found_node->kref))
+                               found_node = NULL;
                        break;
+               }
        }
        rcu_read_unlock();

> 	rcu_read_unlock();
>
> 	return found_node;
>@@ -2507,6 +2509,7 @@ int tipc_nl_node_set_link(struct sk_buff *skb, struct
>genl_info *info)
> 	tipc_node_read_unlock(node);
> 	tipc_bearer_xmit(net, bearer_id, &xmitq, &node-
>>links[bearer_id].maddr,
> 			 NULL);
>+	tipc_node_put(node);
> 	return res;
> }
>
>@@ -2558,12 +2561,14 @@ int tipc_nl_node_get_link(struct sk_buff *skb,
>struct genl_info *info)
> 		link = node->links[bearer_id].link;
> 		if (!link) {
> 			tipc_node_read_unlock(node);
>+			tipc_node_put(node);
> 			err = -EINVAL;
> 			goto err_free;
> 		}
>
> 		err = __tipc_nl_add_link(net, &msg, link, 0);
> 		tipc_node_read_unlock(node);
>+		tipc_node_put(node);
> 		if (err)
> 			goto err_free;
> 	}
>@@ -2634,11 +2639,13 @@ int tipc_nl_node_reset_link_stats(struct sk_buff
>*skb, struct genl_info *info)
> 	if (!link) {
> 		spin_unlock_bh(&le->lock);
> 		tipc_node_read_unlock(node);
>+		tipc_node_put(node);
> 		return -EINVAL;
> 	}
> 	tipc_link_reset_stats(link);
> 	spin_unlock_bh(&le->lock);
> 	tipc_node_read_unlock(node);
>+	tipc_node_put(node);
> 	return 0;
> }
>
>--
>2.43.0

Re: [PATCH net] tipc: hold a reference to nodes found by link name

[Chengfeng Ye]

On Mon, Sep 28, 2026 at 8:17 PM Tung Quang Nguyen
<tung.quang.nguyen@est.tech> wrote:
>
> >Subject: [PATCH net] tipc: hold a reference to nodes found by link name
> >
> >tipc_node_find_by_name() returns a node after dropping its RCU read lock
> >without taking a reference. The LINK_SET, LINK_GET and LINK_RESET_STATS
> >handlers then lock and access the node, racing with timer-driven cleanup of a
> >down peer. Generic netlink serialization does not exclude the node timer.
> >
> >The following interleaving can leave a handler using a freed node:
> >
> >  CPU 0: find the node under RCU and release the node read lock
> >  CPU 1: tipc_node_timeout() clears the links and unlinks the down node
> >  CPU 1: drop the list and timer references, queuing tipc_node_free()
> >  CPU 0: leave the RCU read-side critical section
> >  CPU 1: complete the grace period and free the node
> >  CPU 0: acquire the node lock through the stale pointer
> >
> >LINK_SET also uses the node's media address after releasing the node lock,
> >when passing queued packets to tipc_bearer_xmit().
> >
> >KASAN reported:
> >
> >  BUG: KASAN: slab-use-after-free in _raw_read_lock_bh+0x1d/0x40
> >  Write of size 4 at addr ffff888112723808 by task poc/87
> >  Call Trace:
> >   _raw_read_lock_bh+0x1d/0x40
> >   tipc_nl_node_set_link+0x30e/0x680
> >   genl_family_rcv_msg_doit+0x1e0/0x2c0
> >   genl_rcv_msg+0x419/0x6d0
> >   netlink_rcv_skb+0x11f/0x350
> >  Allocated by task 28:
> >   tipc_node_create+0x9c1/0x1fa0
> >   tipc_node_check_dest+0x121/0x11e0
> >   tipc_disc_rcv+0xdbf/0x1430
> >  Freed by task 87:
> >   kfree+0x149/0x330
> >   rcu_core+0x50a/0x1850
> >  Last potentially related work creation:
> >   __call_rcu_common.constprop.0+0x71/0xa10
> >   tipc_node_timeout+0xb1b/0xe70
> >
>
> Can you update your changelog with decoded stack trace ?
> With decoded stack trace, It helps me understand how your reproducer triggers the issue.

The partial decoded stack trace is attached on the changelog on v2.
https://lore.kernel.org/netdev/179061216640.31693.424352671155791136@kernel.org/T/#t
I will send you the full KASAN report as well as the reproduction
method in a separate private email.

> >Acquire a reference to the selected node with kref_get_unless_zero() before
> >leaving RCU, returning NULL if the node has already been released.
> >Release that reference on every caller exit after the last node access, including
> >transmission in LINK_SET. Keep the existing link lookup order and locking so
> >concurrent link removal still takes the existing error paths.
> >
> >Fixes: 6a939f365bdb ("tipc: Auto removal of peer down node instance")
> >Cc: stable@vger.kernel.org
> >Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> >---
> > net/tipc/node.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> >diff --git a/net/tipc/node.c b/net/tipc/node.c index
> >bd91378b7540..2726bee3bb40 100644
> >--- a/net/tipc/node.c
> >+++ b/net/tipc/node.c
> >@@ -2424,6 +2424,8 @@ static struct tipc_node
> >*tipc_node_find_by_name(struct net *net,
> >               if (found_node)
> >                       break;
> >       }
> >+      if (found_node && !kref_get_unless_zero(&found_node->kref))
> >+              found_node = NULL;
>

> This checking is not optimal.
> Try this:
>
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index bd91378b7540..3e61e9106dd7 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -2421,8 +2421,11 @@ static struct tipc_node *tipc_node_find_by_name(struct net *net,
>                         }
>                 }
>                 tipc_node_read_unlock(n);
> -               if (found_node)
> +               if (found_node) {
> +                       if (!kref_get_unless_zero(&found_node->kref))
> +                               found_node = NULL;
>                         break;
> +               }
>         }
>         rcu_read_unlock();
>
> >       rcu_read_unlock();
> >
> >       return found_node;
> >@@ -2507,6 +2509,7 @@ int tipc_nl_node_set_link(struct sk_buff *skb, struct
> >genl_info *info)
> >       tipc_node_read_unlock(node);
> >       tipc_bearer_xmit(net, bearer_id, &xmitq, &node-
> >>links[bearer_id].maddr,
> >                        NULL);
> >+      tipc_node_put(node);
> >       return res;
> > }
> >
> >@@ -2558,12 +2561,14 @@ int tipc_nl_node_get_link(struct sk_buff *skb,
> >struct genl_info *info)
> >               link = node->links[bearer_id].link;
> >               if (!link) {
> >                       tipc_node_read_unlock(node);
> >+                      tipc_node_put(node);
> >                       err = -EINVAL;
> >                       goto err_free;
> >               }
> >
> >               err = __tipc_nl_add_link(net, &msg, link, 0);
> >               tipc_node_read_unlock(node);
> >+              tipc_node_put(node);
> >               if (err)
> >                       goto err_free;
> >       }
> >@@ -2634,11 +2639,13 @@ int tipc_nl_node_reset_link_stats(struct sk_buff
> >*skb, struct genl_info *info)
> >       if (!link) {
> >               spin_unlock_bh(&le->lock);
> >               tipc_node_read_unlock(node);
> >+              tipc_node_put(node);
> >               return -EINVAL;
> >       }
> >       tipc_link_reset_stats(link);
> >       spin_unlock_bh(&le->lock);
> >       tipc_node_read_unlock(node);
> >+      tipc_node_put(node);
> >       return 0;
> > }
> >
> >--
> >2.43.0
>

The adjustment has been adapted in v2.

Re: [PATCH net] tipc: hold a reference to nodes found by link name

[netdev-bot+sashiko]

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

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

- [High] The fix is incomplete.
- [Low] The kernel-doc for tipc_node_find_by_name() in net/tipc/node.c no
  longer matches the function.

--

tipc: hold a reference to nodes found by link name

The patch makes tipc_node_find_by_name() take a node reference with
kref_get_unless_zero() before it leaves the RCU read-side section. The
LINK_SET, LINK_GET and LINK_RESET_STATS handlers now drop that reference
with tipc_node_put() after their last access to the node. This closes a
use-after-free against timer-driven cleanup of a down peer.

> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index bd91378b75407..2726bee3bb40d 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -2424,6 +2424,8 @@ static struct tipc_node *tipc_node_find_by_name(struct net *net,
>  		if (found_node)
>  			break;
>  	}
> +	if (found_node && !kref_get_unless_zero(&found_node->kref))
> +		found_node = NULL;
>  	rcu_read_unlock();
>  
>  	return found_node;

[Severity: Low]
This isn't a bug, but should the kernel-doc above tipc_node_find_by_name()
be updated to match the new contract? It still says:

 * Returns pointer to node owning the link, or 0 if no matching link is found.

After this change, a non-NULL return carries a reference that the caller
must drop with tipc_node_put(). NULL can also now mean that a link matched
but the node's refcount had already reached zero.

The comment also documents the parameter as @name, while the real
parameter is link_name. That mismatch was already there before this patch.

[ ... ]

> @@ -2634,11 +2639,13 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info)
>  	if (!link) {
>  		spin_unlock_bh(&le->lock);
>  		tipc_node_read_unlock(node);
> +		tipc_node_put(node);
>  		return -EINVAL;
>  	}
>  	tipc_link_reset_stats(link);
>  	spin_unlock_bh(&le->lock);
>  	tipc_node_read_unlock(node);
> +	tipc_node_put(node);
>  	return 0;
>  }

[Severity: High]
Is the fix complete for the handlers named in the commit message? The
broadcast branch earlier in tipc_nl_node_reset_link_stats() never calls
tipc_node_find_by_name(), so it takes no node reference:

	} else if (strstr(link_name, tipc_bclink_name)) {
		rcu_read_lock();
		list_for_each_entry_rcu(node, &tn->node_list, list) {
			tipc_node_read_lock(node);
			link = node->bc_entry.link;
			if (link && !strcmp(link_name, tipc_link_name(link))) {
				err = tipc_bclink_reset_stats(net, link);
	...

Only the node struct waits for an RCU grace period. The broadcast link is
freed right away, and the pointer is never cleared:

net/tipc/node.c:tipc_node_kref_release() {
	...
	kfree(n->bc_entry.link);
	call_rcu(&n->rcu, tipc_node_free);
}

Could this interleaving still happen?

CPU0 (reset_link_stats, broadcast branch)
  rcu_read_lock()
  list_for_each_entry_rcu() reaches node N

CPU1
  tipc_node_timeout()
    tipc_node_cleanup()
      tipc_node_clear_links(), tipc_node_delete_from_list()
    tipc_node_put(n)
      tipc_node_kref_release()
        kfree(n->bc_entry.link)

CPU0
  tipc_node_read_lock(N)          /* N still valid under RCU */
  link = N->bc_entry.link         /* stale, non-NULL */
  tipc_link_name(link)            /* reads freed memory */
  tipc_bclink_reset_stats(net, link)
    tipc_link_reset_stats()       /* memset of l->stats in freed memory */

The LINK_GET dump path looks exposed in the same way. When
TIPC_NLA_LINK_BROADCAST is set, it runs:

tipc_nl_node_dump_link()->__tipc_nl_add_node_links()->tipc_nl_add_bc_link()

	if (bc_link) {
		*prev_link = i;
		err = tipc_nl_add_bc_link(net, msg, node->bc_entry.link);

This reads bcl->name, bcl->max_win and bcl->stats from the possibly freed
link and copies them into the netlink reply. It also runs without a node
reference.

TIPC_NL_LINK_RESET_STATS only needs GENL_UNS_ADMIN_PERM. The LINK_GET
dumpit has no permission flag.

Two possible fixes: take a node reference with kref_get_unless_zero()
during the RCU walk before touching bc_entry.link, as this patch already
does in tipc_node_find_by_name(). Or move the kfree(n->bc_entry.link) into
the RCU callback tipc_node_free().

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