Replace sk_stop_timer_sync() (which wraps timer_delete_sync()) with
timer_shutdown_sync() in mptcp_pm_del_add_timer() and
mptcp_pm_free_anno_list() when freeing ADD_ADDR timer entries.

Issue #623 identified potential re-arming of ADD_ADDR timers
after cancellation. While the code is currently safe via RCU
protection and timer_delete_sync() retry loop, it relies on
implementation details rather than the documented timer API contract.

The kernel's timer API documentation states: "Callers must prevent
restarting of the timer, otherwise this function is meaningless."
timer_shutdown_sync() is the documented API for this exact scenario
- it sets timer->function = NULL, permanently preventing any
re-arm attempts.

Benefits of this change:
- Uses the proper, documented kernel API for shutdown scenarios
- Prevents re-arms at the core level (sets timer->function = NULL)
- Eliminates unsynchronized timer_done check in
  mptcp_pm_free_anno_list
- More robust against future kernel changes to timer internals
- Removes reliance on timer_delete_sync() retry loop mechanism

The change is safe: current protections (RCU, timer_delete_sync
retry loop) remain in place during the transition. This is a code
quality improvement, not a bug fix for a live crash.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/623
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
---
 net/mptcp/pm.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 3e770c7407e1..82785ae78c56 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -157,6 +157,9 @@ bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
 
 	entry = mptcp_pm_del_add_timer(msk, addr, false);
 	ret = entry;
+	/* timer_shutdown_sync() already called in mptcp_pm_del_add_timer.
+	 * Timer is permanently stopped and cannot re-arm. Safe to free.
+	 */
 	kfree_rcu(entry, rcu);
 
 	return ret;
@@ -404,6 +407,7 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 	struct mptcp_pm_add_entry *entry;
 	struct sock *sk = (struct sock *)msk;
 	bool stop_timer = false;
+	int timer_was_armed;
 
 	rcu_read_lock();
 
@@ -421,13 +425,24 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 	 * We hold rcu_read_lock() to ensure it is not freed under us.
 	 */
 	if (stop_timer) {
-		if (check_id)
+		/* For check_id=false (entry about to be freed): use
+		 * timer_shutdown_sync() to permanently prevent re-arming.
+		 * This matches the kernel's documented pattern for shutdown
+		 * scenarios and eliminates reliance on timer_delete_sync
+		 * catching in-callback re-arms.
+		 *
+		 * For check_id=true (entry kept alive): use async stop.
+		 */
+		if (check_id) {
 			sk_stop_timer(sk, &entry->add_timer);
-		else
-			sk_stop_timer_sync(sk, &entry->add_timer);
+		} else {
+			timer_was_armed = timer_shutdown_sync(&entry->add_timer);
+			if (timer_was_armed)
+				__sock_put(sk);
+		}
 	}
 
 	rcu_read_unlock();
+
 	return entry;
 }
 
@@ -474,6 +489,7 @@ static void mptcp_pm_free_anno_list(struct mptcp_sock *msk)
 	struct mptcp_pm_add_entry *entry, *tmp;
 	struct sock *sk = (struct sock *)msk;
 	LIST_HEAD(free_list);
+	int timer_was_armed;
 
 	pr_debug("msk=%p\n", msk);
 
@@ -482,8 +498,16 @@ static void mptcp_pm_free_anno_list(struct mptcp_sock *msk)
 	spin_unlock_bh(&msk->pm.lock);
 
 	list_for_each_entry_safe(entry, tmp, &free_list, list) {
-		if (!entry->timer_done)
-			sk_stop_timer_sync(sk, &entry->add_timer);
+		/* Always shutdown timer: no timer_done check needed.
+		 * timer_shutdown_sync() is idempotent and prevents future
+		 * re-arms. This eliminates the unsynchronized timer_done
+		 * read, making the shutdown intent explicit and matching
+		 * the kernel's documented pattern for cleanup scenarios.
+		 */
+		timer_was_armed = timer_shutdown_sync(&entry->add_timer);
+		if (timer_was_armed)
+			__sock_put(sk);
+
 		kfree_rcu(entry, rcu);
 	}
 }
-- 
2.43.0

Hi Kalpan,

On 03/06/2026 21:56, Kalpan Jani wrote:
> Replace sk_stop_timer_sync() (which wraps timer_delete_sync()) with
> timer_shutdown_sync() in mptcp_pm_del_add_timer() and
> mptcp_pm_free_anno_list() when freeing ADD_ADDR timer entries.
> 
> Issue #623 identified potential re-arming of ADD_ADDR timers
> after cancellation. While the code is currently safe via RCU
> protection and timer_delete_sync() retry loop, it relies on
> implementation details rather than the documented timer API contract.
> 
> The kernel's timer API documentation states: "Callers must prevent
> restarting of the timer, otherwise this function is meaningless."
> timer_shutdown_sync() is the documented API for this exact scenario
> - it sets timer->function = NULL, permanently preventing any
> re-arm attempts.
> 
> Benefits of this change:
> - Uses the proper, documented kernel API for shutdown scenarios
> - Prevents re-arms at the core level (sets timer->function = NULL)
> - Eliminates unsynchronized timer_done check in
>   mptcp_pm_free_anno_list
> - More robust against future kernel changes to timer internals
> - Removes reliance on timer_delete_sync() retry loop mechanism
> 
> The change is safe: current protections (RCU, timer_delete_sync
> retry loop) remain in place during the transition. This is a code
> quality improvement, not a bug fix for a live crash.

It looks like this patch is corrupted:

  error: corrupt patch at line 56

Plus, the CI tries to apply patches on top of the 'export' branch, it is
often easier if you are on top of this branch (or 'for-review').

Also, please sync with Li who is also trying to fix this issue:

  https://lore.kernel.org/20260526103647.732350-1-xiasong.lee@gmail.com

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.