[PATCH mptcp-net 0/2] mptcp: prevent add_addr timer rearm during teardown

[PATCH mptcp-net 0/2] mptcp: prevent add_addr timer rearm during teardown

[Li Xiasong]

From: Li Xiasong <lixiasong1@huawei.com>

This series fixes a potential use-after-free in MPTCP ADD_ADDR timer
teardown.

The ADD_ADDR retransmission timer callback may re-arm itself on the
sock_owned_by_user() path. During final teardown, this can race with
timer cancellation and leave a timer pending after the entry is removed
and freed.

Using sk_stop_timer_sync() is not sufficient for this case because it
waits for a running callback but does not permanently disable the timer.

This series does two things:

  1. Add sk_shutdown_timer_sync() to socket timer helpers.
     It wraps timer_shutdown_sync() and preserves sk reference accounting
     semantics by dropping a socket reference when a pending timer is
     removed.

  2. Switch MPTCP ADD_ADDR final teardown paths to
     sk_shutdown_timer_sync(), so the timer cannot be re-armed once
     teardown starts.

Reported scenario:
  Link: https://github.com/multipath-tcp/mptcp_net-next/issues/623

Li Xiasong (2):
  net: add sk_shutdown_timer_sync() helper
  mptcp: use sk_shutdown_timer_sync() for add_addr timer teardown

 include/net/sock.h | 5 +++++
 net/core/sock.c    | 7 +++++++
 net/mptcp/pm.c     | 4 ++--
 3 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH mptcp-net 1/2] net: add sk_shutdown_timer_sync() helper

[Li Xiasong]

From: Li Xiasong <lixiasong1@huawei.com>

Add sk_shutdown_timer_sync(), a socket-timer helper wrapping
timer_shutdown_sync() with the same sk reference accounting used by
sk_stop_timer() and sk_stop_timer_sync().

When a pending timer is removed, the helper drops the corresponding sk
reference via __sock_put(). This provides a safe shutdown primitive for
final teardown paths where timer re-arming must be prevented.

Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---
 include/net/sock.h | 5 +++++
 net/core/sock.c    | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/include/net/sock.h b/include/net/sock.h
index dccd3738c368..2de205e88271 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2496,6 +2496,11 @@ void sk_stop_timer(struct sock *sk, struct timer_list *timer);
 
 void sk_stop_timer_sync(struct sock *sk, struct timer_list *timer);
 
+/* Synchronously stop and permanently disable a socket timer.
+ * Drop @sk ref if a pending timer was removed.
+ */
+void sk_shutdown_timer_sync(struct sock *sk, struct timer_list *timer);
+
 int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue,
 			struct sk_buff *skb, unsigned int flags,
 			void (*destructor)(struct sock *sk,
diff --git a/net/core/sock.c b/net/core/sock.c
index b37b664b6eb9..4670bc3e6a41 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3696,6 +3696,13 @@ void sk_stop_timer_sync(struct sock *sk, struct timer_list *timer)
 }
 EXPORT_SYMBOL(sk_stop_timer_sync);
 
+void sk_shutdown_timer_sync(struct sock *sk, struct timer_list *timer)
+{
+	if (timer_shutdown_sync(timer))
+		__sock_put(sk);
+}
+EXPORT_SYMBOL(sk_shutdown_timer_sync);
+
 void sock_init_data_uid(struct socket *sock, struct sock *sk, kuid_t uid)
 {
 	sk_init_common(sk);
-- 
2.43.0

[PATCH mptcp-net 2/2] mptcp: use sk_shutdown_timer_sync() for add_addr timer teardown

[Li Xiasong]

From: Li Xiasong <lixiasong1@huawei.com>

Sashiko reported a possible use-after-free in add_addr timer teardown.

The add_addr timer callback can re-arm itself on the sock_owned_by_user()
path. In final teardown paths, sk_stop_timer_sync() only waits for a
running callback to finish, but does not prevent a concurrent callback
from re-arming the timer.

Use sk_shutdown_timer_sync() for add_addr timer teardown so the timer
cannot be re-armed after teardown starts, preventing a possible
use-after-free on a freed add entry.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/623
Fixes: 5cd6e0ad79d2 ("mptcp: pm: ADD_ADDR rtx: fix potential data-race")
Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---
 net/mptcp/pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 3e770c7407e1..cda3efb2c206 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -424,7 +424,7 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 		if (check_id)
 			sk_stop_timer(sk, &entry->add_timer);
 		else
-			sk_stop_timer_sync(sk, &entry->add_timer);
+			sk_shutdown_timer_sync(sk, &entry->add_timer);
 	}
 
 	rcu_read_unlock();
@@ -483,7 +483,7 @@ static void mptcp_pm_free_anno_list(struct mptcp_sock *msk)
 
 	list_for_each_entry_safe(entry, tmp, &free_list, list) {
 		if (!entry->timer_done)
-			sk_stop_timer_sync(sk, &entry->add_timer);
+			sk_shutdown_timer_sync(sk, &entry->add_timer);
 		kfree_rcu(entry, rcu);
 	}
 }
-- 
2.43.0

Re: [PATCH mptcp-net 2/2] mptcp: use sk_shutdown_timer_sync() for add_addr timer teardown

[Matthieu Baerts]

Hi Li,

On 26/05/2026 20:36, Li Xiasong wrote:
> From: Li Xiasong <lixiasong1@huawei.com>
> 
> Sashiko reported a possible use-after-free in add_addr timer teardown.
> 
> The add_addr timer callback can re-arm itself on the sock_owned_by_user()
> path. In final teardown paths, sk_stop_timer_sync() only waits for a
> running callback to finish, but does not prevent a concurrent callback
> from re-arming the timer.
> 
> Use sk_shutdown_timer_sync() for add_addr timer teardown so the timer
> cannot be re-armed after teardown starts, preventing a possible
> use-after-free on a freed add entry.

Sashiko is saying that if sk_reset_timer() is called after
sk_shutdown_timer_sync(), it will not re-arm the timer, but in this
case, mod_timer() will also return 0, and the socket refcount will be
incremented.

https://sashiko.dev/#/patchset/20260526103647.732350-1-xiasong.lee%40gmail.com

It is not clear to me how to solve that without checking whether
timer->function is set or not after having called mod_timer(), because
that seems quite hackish...

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

Re: [PATCH mptcp-net 2/2] mptcp: use sk_shutdown_timer_sync() for add_addr timer teardown

[Li Xiasong]

Hi Matt,

> Sashiko is saying that if sk_reset_timer() is called after
> sk_shutdown_timer_sync(), it will not re-arm the timer, but in this
> case, mod_timer() will also return 0, and the socket refcount will be
> incremented.

Thanks for the review and for pointing this out.

You are right, and sashiko's concern is valid: with current semantics,
mod_timer() returns 0 both when an inactive timer is armed and when a
shutdown timer rejects re-arming. As a result, sk_reset_timer() cannot
reliably distinguish these two cases, which can break socket refcount
accounting.

So if we keep using shutdown on a socket timer, there is no simple fix
in MPTCP alone. A proper fix likely needs timer-core support (e.g. a
shutdown-aware API, or equivalent semantics).

> It is not clear to me how to solve that without checking whether
> timer->function is set or not after having called mod_timer(), because
> that seems quite hackish...

I also re-checked timer core behavior: timer_delete_sync() does more
than waiting for a running callback. If a callback re-arms the timer
before returning, timer_delete_sync() can still remove it before
sk_stop_timer_sync() returns.

I need to revisit the design with this in mind and do a deeper analysis
before sending v2.

Thanks again for the feedback.

Li Xiasong

Re: [PATCH mptcp-net 0/2] mptcp: prevent add_addr timer rearm during teardown

[MPTCP CI]

Hi Li,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_diag ⚠️ 
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/26490120648

Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/f75fedf58a6b
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1100863


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-net] mptcp: prevent add_addr timer rearm during teardown

[Kalpan Jani]

Hi Li,
 
Great work on this patch series! Your approach of creating sk_shutdown_timer_sync()
helper is the right pattern.
 
I noticed one additional issue: in mptcp_pm_free_anno_list(), the timer_done check
is unsynchronized:
 
    if (!entry->timer_done)  // Data race: no lock protecting this read
        sk_stop_timer_sync(sk, &entry->add_timer);
 
This is a classic data race. Your patch keeps it as-is, but I think we should
also fix it by removing this check entirely and always calling timer_shutdown_sync().
 
Would you be open to adding this improvement to your patch? The benefit:
- Eliminates the data race
- Simplifies the code (unconditional shutdown)
- timer_shutdown_sync() is idempotent anyway
 
I'd be happy to:
1. Test the updated patch
2. Help refine the implementation
3. Co-author if you'd like
 
What do you think?
 
Cheers,
Kalpan Jani
kalpan.jani@mpiricsoftware.com

Re: [PATCH mptcp-net] mptcp: prevent add_addr timer rearm during teardown

[Li Xiasong]

Hi Matt, Hi Kalpan,

Thank you for your review and suggestion.

The main reason I have been hesitating to send a v2 is that,
after re-checking the code paths more carefully, I am no longer
sure that the original issue reported by Sashiko can actually
happen in this add_addr timer path.

My current understanding is that both sk_stop_timer_sync() and
sk_shutdown_timer_sync() synchronously stop the timer: if the
callback is running, they wait for it to finish, and if the
callback re-arms the timer before returning, the pending timer
is then deleted before the helper returns.

So for this specific add_addr timer, I do not currently see a
path where teardown completes and a re-armed timer is still
left behind afterward. That is why I have not sent a v2 based
on sk_shutdown_timer_sync() so far.

That said, I think tightening this teardown path could still be
useful for robustness and could help avoid similar issues after
future changes.

On 6/4/2026 3:19 PM, Kalpan Jani wrote:
> Hi Li,
>  
> Great work on this patch series! Your approach of creating sk_shutdown_timer_sync()
> helper is the right pattern.
>  
> I noticed one additional issue: in mptcp_pm_free_anno_list(), the timer_done check
> is unsynchronized:
>  
>     if (!entry->timer_done)  // Data race: no lock protecting this read
>         sk_stop_timer_sync(sk, &entry->add_timer);
>  
> This is a classic data race. Your patch keeps it as-is, but I think we should
> also fix it by removing this check entirely and always calling timer_shutdown_sync().
>  
> Would you be open to adding this improvement to your patch? The benefit:
> - Eliminates the data race
> - Simplifies the code (unconditional shutdown)
> - timer_shutdown_sync() is idempotent anyway
>  

Yes, I think this would be a good direction if timer_done can
be removed safely.

I agree that it would avoid the unsynchronized timer_done check
and simplify the teardown path.

Before removing timer_done, I would like to better understand
the original scenario for which it was introduced.

Matt, if I am understanding it correctly, timer_done was added
to avoid the case where the socket reference held by the timer
becomes the last one, and then sock_put() from the timer
callback triggers socket destruction, which in turn reaches
sk_stop_timer_sync() for the same timer.

If that understanding is correct, my concern is that removing
timer_done without addressing that original case might
re-introduce the self-wait issue during teardown from the timer
callback path.

So overall, I agree with Kalpan's suggestion if timer_done is
indeed no longer needed for that last-reference case. I would
just like to clarify that point first before sending a new
version.

> I'd be happy to:
> 1. Test the updated patch
> 2. Help refine the implementation
> 3. Co-author if you'd like
>  
> What do you think?
>  

Thanks a lot for the offer, I really appreciate it.

Thanks,
Li Xiasong

Re: [PATCH mptcp-net] mptcp: prevent add_addr timer rearm during teardown

[Kalpan Jani]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1248 bytes --]

Hi Li,
 
Thank you for that detailed explanation! I researched the commit 8143a224785c
that introduced timer_done, and I now fully understand the self-wait deadlock
scenario you described.
 
You're absolutely right:
 
When the socket is only held by the timer, and the callback decides not to
reschedule, the sock_put() at the end triggers sk_free(). This in turn calls
sk_stop_timer_sync() on the same timer while the callback is still running,
causing a deadlock.
 
The timer_done flag is the safeguard against this. While I correctly identified
the unsynchronized read as a data race, I failed to understand that this flag
serves a critical safety function beyond just preventing re-arms.
 
My suggestion to remove timer_done without addressing the self-wait scenario
would reintroduce the deadlock bug that commit fixed.
 
Your current approach - using sk_shutdown_timer_sync() while keeping the
timer_done check - is the right balance:
✓ Improved robustness (shutdown_sync is more explicit)
✓ Protection against self-wait (timer_done check)
✓ No new deadlock risks
 
I apologize for not thinking through this scenario fully. Your patch is solid,
and I support it as-is.
 
Thanks for the thorough review and explanation!
 
Cheers,
Kalpan