* Re: [RFC PATCH net] mptcp: pm: fix ADD_ADDR timer infinite retry on option space insufficient
2026-04-18 10:00 [RFC PATCH net] mptcp: pm: fix ADD_ADDR timer infinite retry on option space insufficient Li Xiasong
@ 2026-04-20 9:20 ` Matthieu Baerts
0 siblings, 0 replies; 2+ messages in thread
From: Matthieu Baerts @ 2026-04-20 9:20 UTC (permalink / raw)
To: Li Xiasong
Cc: netdev, mptcp, linux-kernel, yuehaibing, zhangchangzhong,
weiyongjun1, Mat Martineau, Geliang Tang, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman
Hi Li,
On 18/04/2026 12:00, Li Xiasong wrote:
> When TCP option space is insufficient (e.g., IPv6 with tcp_timestamps
> enabled), the original code jumped to out_unlock without clearing the
> addr_signal flag. This caused mptcp_pm_add_timer to keep rescheduling
> indefinitely without sending ADD_ADDR,
Funny, I was looking at this issue on Friday evening :)
> preventing the endpoint list from being traversed.
It might help to add a bit of context: I guess here you meant that it
prevent advertising other ADD_ADDR, not using other subflows when
sending data, right?
> In a pure ACK scenario (indicated by drop_other_suboptions=true), if
> the option space is insufficient to carry the ADD_ADDR suboption, it
> is appropriate to drop this address signal to allow the timer handler
> to move on to other addresses.
>
> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
> ---
>
> Seeking feedback on:
>
> When announcing addresses to the peer, MPTCP sends a pure ACK packet
> to carry MPTCP options (ADD_ADDR). In this scenario, if the option space
> is insufficient for ADD_ADDR, clearing addr_signal would:
>
> - Prevent the timer from retrying infinitely
> - Allow the timer to continue traversing and processing other addresses
> - Not block other subflow creation or address announcement operations
>
> Is there any scenario where we should retry later instead of clearing
> the address signal/echo flag? However, if a pure ACK doesn't have
> enough space for the flag, subsequent packets won't either.
That's correct: for the moment, if it is a pure ACK and there is not
enough space, no need to retry later because it is not possible to have
more space. It should only happen with an ADD_ADDR containing an IPv6
address and a port number. It might be good to specify this in the
commit message.
> ---
> net/mptcp/pm.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 57a456690406..1d49779c6a1f 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -881,19 +881,18 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, const struct sk_buff *skb,
> }
>
> *echo = mptcp_pm_should_add_signal_echo(msk);
> + add_addr = msk->pm.addr_signal &
> + ~(*echo ? BIT(MPTCP_ADD_ADDR_ECHO) : BIT(MPTCP_ADD_ADDR_SIGNAL));
> port = !!(*echo ? msk->pm.remote.port : msk->pm.local.port);
> -
> family = *echo ? msk->pm.remote.family : msk->pm.local.family;
nit: while at it, maybe clearer to have a dedicated 'if (*echo)' instead
of 3 lines with '*echo ? ... : ..., no?
if (*echo) {
add_addr = ...
port = ...
family = ...
} else {
add_addr = ...
port = ...
family = ...
}
> - if (remaining < mptcp_add_addr_len(family, *echo, port))
> - goto out_unlock;
>
> - if (*echo) {
> - *addr = msk->pm.remote;
> - add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_ECHO);
> - } else {
> - *addr = msk->pm.local;
> - add_addr = msk->pm.addr_signal & ~BIT(MPTCP_ADD_ADDR_SIGNAL);
> + if (remaining < mptcp_add_addr_len(family, *echo, port)) {
> + if (*drop_other_suboptions)
> + WRITE_ONCE(msk->pm.addr_signal, add_addr);
If it is dropped, it would be helpful to increment the ADDADDRTXDROP MIB
counter, and ideally check that in the MPTCP selftests (e.g. adding a
new subtest in mptcp_join.sh, in add_addr_ports_tests()?).
Also, I wonder if it would not be clearer to jump to a new label here...
> + goto out_unlock;
> }
> +
> + *addr = *echo ? msk->pm.remote : msk->pm.local;
> WRITE_ONCE(msk->pm.addr_signal, add_addr);
> ret = true;
... inverting the two lines above, and adding "drop_signal_mark" label?
Apart from the comments above, I think your patch is doing the right thing.
Also, one last request: do you mind sending the v2 only to the mptcp ML,
please? I have a bunch of related fixes [1] plus this one is not urgent.
In fact, except for (urgent) fixes, it might be better to send MPTCP
patches only the to MPTCP ML: to a restricted number of people for the
first versions, there is enough traffic on Netdev.
[1]
https://lore.kernel.org/20260415-mptcp-inc-limits-v5-0-e54c3bf80e4e@kernel.org
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 2+ messages in thread