[PATCH net] tipc: prevent node timer rearm after peer removal

[PATCH net] tipc: prevent node timer rearm after peer removal

[David Lee]

TIPC node deletion removes the node from lookup tables, calls
timer_delete_sync(), and drops the timer reference.  This stops an already
running timer callback, but it does not prevent another racing path from
rearming the same timer after deletion has started.

Synthetic UDP discovery can race TIPC_NL_PEER_REMOVE and link property
updates.  When discovery recreates the first link, tipc_node_check_dest()
calls mod_timer() and retakes the timer reference.  If that happens after
tipc_node_delete() has deleted the timer, the orphaned timer can later run
after network namespace teardown has freed the per-net TIPC state and
broadcast link.

Use timer_shutdown_sync() when deleting a node.  Shutdown has the same
synchronization effect as timer_delete_sync(), and also prevents future
rearming through mod_timer().

Fixes: b34040227be7 ("tipc: add peer removal functionality")
Signed-off-by: David Lee <david.lee@trailofbits.com>
Assisted-by: Codex:gpt-5.5
---
Trail of Bits has a reproducer for this bug demonstrating Kernel Panic which can be shared further if needed.

 net/tipc/node.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 262b39ecf5f8..5b3ee44347be 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -638,7 +638,7 @@ static void tipc_node_delete(struct tipc_node *node)
 	trace_tipc_node_delete(node, true, " ");
 	tipc_node_delete_from_list(node);
 
-	timer_delete_sync(&node->timer);
+	timer_shutdown_sync(&node->timer);
 	tipc_node_put(node);
 }
 
-- 
2.43.0

RE: [PATCH net] tipc: prevent node timer rearm after peer removal

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: prevent node timer rearm after peer removal
>
>TIPC node deletion removes the node from lookup tables, calls
>timer_delete_sync(), and drops the timer reference.  This stops an already
>running timer callback, but it does not prevent another racing path from
>rearming the same timer after deletion has started.
>
>Synthetic UDP discovery can race TIPC_NL_PEER_REMOVE and link property
>updates.  When discovery recreates the first link, tipc_node_check_dest() calls
>mod_timer() and retakes the timer reference.  If that happens after
>tipc_node_delete() has deleted the timer, the orphaned timer can later run
>after network namespace teardown has freed the per-net TIPC state and
>broadcast link.
>
>Use timer_shutdown_sync() when deleting a node.  Shutdown has the same
>synchronization effect as timer_delete_sync(), and also prevents future
>rearming through mod_timer().
>
>Fixes: b34040227be7 ("tipc: add peer removal functionality")
>Signed-off-by: David Lee <david.lee@trailofbits.com>
>Assisted-by: Codex:gpt-5.5
>---
>Trail of Bits has a reproducer for this bug demonstrating Kernel Panic which can
>be shared further if needed.
>
> net/tipc/node.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/tipc/node.c b/net/tipc/node.c index
>262b39ecf5f8..5b3ee44347be 100644
>--- a/net/tipc/node.c
>+++ b/net/tipc/node.c
>@@ -638,7 +638,7 @@ static void tipc_node_delete(struct tipc_node *node)
> 	trace_tipc_node_delete(node, true, " ");
> 	tipc_node_delete_from_list(node);
>
>-	timer_delete_sync(&node->timer);
>+	timer_shutdown_sync(&node->timer);
 
sashiko reports:

Could this regression introduce a permanent memory leak of the tipc_node 
structure if it races with tipc_node_check_dest()?
If thread A deletes a node and shuts down the timer, and thread B 
concurrently processes a UDP discovery packet and calls 
tipc_node_check_dest(), thread B might execute this sequence:
    if (!mod_timer(&n->timer, intv))
        tipc_node_get(n);
Since the timer is shut down, mod_timer() silently discards the operation,
does not enqueue the timer, and returns 0. Thread B then calls 
tipc_node_get() expecting the timer callback to eventually execute and drop
the reference.
Because the timer is shut down and will never fire, tipc_node_timeout() 
will never execute to drop this reference, leaking the unlinked node 
permanently.

> 	tipc_node_put(node);
> }
>
>--
>2.43.0
>