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