The special C-flag case expects the ADD_ADDR to be received when
switching to 'fully-established'. But for various reasons, the ADD_ADDR
could be sent after the "4th ACK", and the special case doesn't work.
On NIPA, the new test validating this special case for the C-flag failed
a few times, e.g.
102 default limits, server deny join id 0
syn rx [FAIL] got 0 JOIN[s] syn rx expected 2
Server ns stats
(...)
MPTcpExtAddAddrTx 1
MPTcpExtEchoAdd 1
Client ns stats
(...)
MPTcpExtAddAddr 1
MPTcpExtEchoAddTx 1
synack rx [FAIL] got 0 JOIN[s] synack rx expected 2
ack rx [FAIL] got 0 JOIN[s] ack rx expected 2
join Rx [FAIL] see above
syn tx [FAIL] got 0 JOIN[s] syn tx expected 2
join Tx [FAIL] see above
I had a suspicion about what the issue could be: the ADD_ADDR might have
been received after the switch to the 'fully-established' state. The
issue was not easy to reproduce. The packet capture shown that the
ADD_ADDR can indeed be sent with a delay, and the client would not try
to establish subflows to it as expected.
A simple fix is not to mark the endpoints as 'used' in the C-flag case,
when looking at creating subflows to the remote initial IP address and
port. In this case, there is no need to try.
Note: newly added fullmesh endpoints will still continue to be used as
expected, thanks to the conditions behind mptcp_pm_add_addr_c_flag_case.
Fixes: 4b1ff850e0c1 ("mptcp: pm: in-kernel: usable client side with C-flag")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
net/mptcp/pm_kernel.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index da431da16ae0..df2bc36593d0 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -370,6 +370,10 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
}
subflow:
+ /* No need to try establishing subflows to remote id0 if not allowed */
+ if (mptcp_pm_add_addr_c_flag_case(msk))
+ goto exit;
+
/* check if should create a new subflow */
while (msk->pm.local_addr_used < endp_subflow_max &&
msk->pm.extra_subflows < limit_extra_subflows) {
@@ -401,6 +405,8 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
__mptcp_subflow_connect(sk, &local, &addrs[i]);
spin_lock_bh(&msk->pm.lock);
}
+
+exit:
mptcp_pm_nl_check_work_pending(msk);
}
--
2.51.0Hi Matt,
On Wed, 2025-10-08 at 15:59 +0200, Matthieu Baerts (NGI0) wrote:
> The special C-flag case expects the ADD_ADDR to be received when
> switching to 'fully-established'. But for various reasons, the
> ADD_ADDR
> could be sent after the "4th ACK", and the special case doesn't work.
>
> On NIPA, the new test validating this special case for the C-flag
> failed
> a few times, e.g.
>
> 102 default limits, server deny join id 0
> syn rx [FAIL] got 0 JOIN[s] syn rx expected 2
>
> Server ns stats
> (...)
> MPTcpExtAddAddrTx 1
> MPTcpExtEchoAdd 1
>
> Client ns stats
> (...)
> MPTcpExtAddAddr 1
> MPTcpExtEchoAddTx 1
>
> synack rx [FAIL] got 0 JOIN[s] synack rx
> expected 2
> ack rx [FAIL] got 0 JOIN[s] ack rx expected 2
> join Rx [FAIL] see above
> syn tx [FAIL] got 0 JOIN[s] syn tx expected 2
> join Tx [FAIL] see above
>
> I had a suspicion about what the issue could be: the ADD_ADDR might
> have
> been received after the switch to the 'fully-established' state. The
> issue was not easy to reproduce. The packet capture shown that the
It was indeed not easy to reproduce, but after spending a lot of time,
I finally managed to reproduce it and also verified that this patch
indeed works.
> ADD_ADDR can indeed be sent with a delay, and the client would not
> try
> to establish subflows to it as expected.
>
> A simple fix is not to mark the endpoints as 'used' in the C-flag
> case,
> when looking at creating subflows to the remote initial IP address
> and
> port. In this case, there is no need to try.
>
> Note: newly added fullmesh endpoints will still continue to be used
> as
> expected, thanks to the conditions behind
> mptcp_pm_add_addr_c_flag_case.
>
> Fixes: 4b1ff850e0c1 ("mptcp: pm: in-kernel: usable client side with
> C-flag")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/pm_kernel.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index da431da16ae0..df2bc36593d0 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -370,6 +370,10 @@ static void
> mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> }
>
> subflow:
> + /* No need to try establishing subflows to remote id0 if not
> allowed */
> + if (mptcp_pm_add_addr_c_flag_case(msk))
> + goto exit;
> +
I think it would be better to place this c_flag check inside the while
loop below.
> /* check if should create a new subflow */
> while (msk->pm.local_addr_used < endp_subflow_max &&
> msk->pm.extra_subflows < limit_extra_subflows) {
> @@ -401,6 +405,8 @@ static void
> mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> __mptcp_subflow_connect(sk, &local,
> &addrs[i]);
> spin_lock_bh(&msk->pm.lock);
> }
> +
> +exit:
This way, we won't need to add an exit label and can simply use a
break. Something like this:
subflow:
/* check if should create a new subflow */
while (msk->pm.local_addr_used < endp_subflow_max &&
msk->pm.extra_subflows < limit_extra_subflows) {
struct mptcp_addr_info addrs[MPTCP_PM_ADDR_MAX];
bool fullmesh;
int i, nr;
/* No need to try establishing subflows to remote
id0 if not allowed */
if (mptcp_pm_add_addr_c_flag_case(msk))
break;
if (signal_and_subflow)
signal_and_subflow = false;
else if (!select_local_address(pernet, msk, &local))
break;
WDYT?
Thanks,
-Geliang
> mptcp_pm_nl_check_work_pending(msk);
> }
>
>
Hi Geliang,
Thank you for the review!
17 Oct 2025 08:11:37 Geliang Tang <geliang@kernel.org>:
> Hi Matt,
>
> On Wed, 2025-10-08 at 15:59 +0200, Matthieu Baerts (NGI0) wrote:
>> The special C-flag case expects the ADD_ADDR to be received when
>> switching to 'fully-established'. But for various reasons, the
>> ADD_ADDR
>> could be sent after the "4th ACK", and the special case doesn't work.
>>
>> On NIPA, the new test validating this special case for the C-flag
>> failed
>> a few times, e.g.
>>
>> 102 default limits, server deny join id 0
>> syn rx [FAIL] got 0 JOIN[s] syn rx expected 2
>>
>> Server ns stats
>> (...)
>> MPTcpExtAddAddrTx 1
>> MPTcpExtEchoAdd 1
>>
>> Client ns stats
>> (...)
>> MPTcpExtAddAddr 1
>> MPTcpExtEchoAddTx 1
>>
>> synack rx [FAIL] got 0 JOIN[s] synack rx
>> expected 2
>> ack rx [FAIL] got 0 JOIN[s] ack rx expected 2
>> join Rx [FAIL] see above
>> syn tx [FAIL] got 0 JOIN[s] syn tx expected 2
>> join Tx [FAIL] see above
>>
>> I had a suspicion about what the issue could be: the ADD_ADDR might
>> have
>> been received after the switch to the 'fully-established' state. The
>> issue was not easy to reproduce. The packet capture shown that the
>
> It was indeed not easy to reproduce, but after spending a lot of time,
> I finally managed to reproduce it and also verified that this patch
> indeed works.
Thanks!
>> ADD_ADDR can indeed be sent with a delay, and the client would not
>> try
>> to establish subflows to it as expected.
>>
>> A simple fix is not to mark the endpoints as 'used' in the C-flag
>> case,
>> when looking at creating subflows to the remote initial IP address
>> and
>> port. In this case, there is no need to try.
>>
>> Note: newly added fullmesh endpoints will still continue to be used
>> as
>> expected, thanks to the conditions behind
>> mptcp_pm_add_addr_c_flag_case.
>>
>> Fixes: 4b1ff850e0c1 ("mptcp: pm: in-kernel: usable client side with
>> C-flag")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> net/mptcp/pm_kernel.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>> index da431da16ae0..df2bc36593d0 100644
>> --- a/net/mptcp/pm_kernel.c
>> +++ b/net/mptcp/pm_kernel.c
>> @@ -370,6 +370,10 @@ static void
>> mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>> }
>>
>> subflow:
>> + /* No need to try establishing subflows to remote id0 if not
>> allowed */
>> + if (mptcp_pm_add_addr_c_flag_case(msk))
>> + goto exit;
>> +
>
> I think it would be better to place this c_flag check inside the while
> loop below.
>
>> /* check if should create a new subflow */
>> while (msk->pm.local_addr_used < endp_subflow_max &&
>> msk->pm.extra_subflows < limit_extra_subflows) {
>> @@ -401,6 +405,8 @@ static void
>> mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>> __mptcp_subflow_connect(sk, &local,
>> &addrs[i]);
>> spin_lock_bh(&msk->pm.lock);
>> }
>> +
>> +exit:
>
> This way, we won't need to add an exit label and can simply use a
> break. Something like this:
>
> subflow:
> /* check if should create a new subflow */
> while (msk->pm.local_addr_used < endp_subflow_max &&
> msk->pm.extra_subflows < limit_extra_subflows) {
> struct mptcp_addr_info addrs[MPTCP_PM_ADDR_MAX];
> bool fullmesh;
> int i, nr;
>
> /* No need to try establishing subflows to remote
> id0 if not allowed */
> if (mptcp_pm_add_addr_c_flag_case(msk))
> break;
I'm not convinced: that means these conditions will be checked for each
local address, for each iteration here, while the result will be always the
same.
Here it sounds better to me to skip the selection before starting, if there
is no need to do that.
Cheers,
Matt
>
> if (signal_and_subflow)
> signal_and_subflow = false;
> else if (!select_local_address(pernet, msk, &local))
> break;
>
> WDYT?
>
> Thanks,
> -Geliang
>
>> mptcp_pm_nl_check_work_pending(msk);
>> }
>>
>>
On Fri, 2025-10-17 at 08:21 +0200, Matthieu Baerts wrote:
> Hi Geliang,
>
> Thank you for the review!
>
> 17 Oct 2025 08:11:37 Geliang Tang <geliang@kernel.org>:
>
> > Hi Matt,
> >
> > On Wed, 2025-10-08 at 15:59 +0200, Matthieu Baerts (NGI0) wrote:
> > > The special C-flag case expects the ADD_ADDR to be received when
> > > switching to 'fully-established'. But for various reasons, the
> > > ADD_ADDR
> > > could be sent after the "4th ACK", and the special case doesn't
> > > work.
> > >
> > > On NIPA, the new test validating this special case for the C-flag
> > > failed
> > > a few times, e.g.
> > >
> > > 102 default limits, server deny join id 0
> > > syn rx [FAIL] got 0 JOIN[s] syn rx
> > > expected 2
> > >
> > > Server ns stats
> > > (...)
> > > MPTcpExtAddAddrTx 1
> > > MPTcpExtEchoAdd 1
> > >
> > > Client ns stats
> > > (...)
> > > MPTcpExtAddAddr 1
> > > MPTcpExtEchoAddTx 1
> > >
> > > synack rx [FAIL] got 0 JOIN[s] synack rx
> > > expected 2
> > > ack rx [FAIL] got 0 JOIN[s] ack rx
> > > expected 2
> > > join Rx [FAIL] see above
> > > syn tx [FAIL] got 0 JOIN[s] syn tx
> > > expected 2
> > > join Tx [FAIL] see above
> > >
> > > I had a suspicion about what the issue could be: the ADD_ADDR
> > > might
> > > have
> > > been received after the switch to the 'fully-established' state.
> > > The
> > > issue was not easy to reproduce. The packet capture shown that
> > > the
> >
> > It was indeed not easy to reproduce, but after spending a lot of
> > time,
> > I finally managed to reproduce it and also verified that this patch
> > indeed works.
>
> Thanks!
>
> > > ADD_ADDR can indeed be sent with a delay, and the client would
> > > not
> > > try
> > > to establish subflows to it as expected.
> > >
> > > A simple fix is not to mark the endpoints as 'used' in the C-flag
> > > case,
> > > when looking at creating subflows to the remote initial IP
> > > address
> > > and
> > > port. In this case, there is no need to try.
> > >
> > > Note: newly added fullmesh endpoints will still continue to be
> > > used
> > > as
> > > expected, thanks to the conditions behind
> > > mptcp_pm_add_addr_c_flag_case.
> > >
> > > Fixes: 4b1ff850e0c1 ("mptcp: pm: in-kernel: usable client side
> > > with
> > > C-flag")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > > net/mptcp/pm_kernel.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> > > index da431da16ae0..df2bc36593d0 100644
> > > --- a/net/mptcp/pm_kernel.c
> > > +++ b/net/mptcp/pm_kernel.c
> > > @@ -370,6 +370,10 @@ static void
> > > mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> > > }
> > >
> > > subflow:
> > > + /* No need to try establishing subflows to remote id0 if not
> > > allowed */
> > > + if (mptcp_pm_add_addr_c_flag_case(msk))
> > > + goto exit;
> > > +
> >
> > I think it would be better to place this c_flag check inside the
> > while
> > loop below.
> >
> > > /* check if should create a new subflow */
> > > while (msk->pm.local_addr_used < endp_subflow_max &&
> > > msk->pm.extra_subflows < limit_extra_subflows) {
> > > @@ -401,6 +405,8 @@ static void
> > > mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> > > __mptcp_subflow_connect(sk, &local,
> > > &addrs[i]);
> > > spin_lock_bh(&msk->pm.lock);
> > > }
> > > +
> > > +exit:
> >
> > This way, we won't need to add an exit label and can simply use a
> > break. Something like this:
> >
> > subflow:
> > /* check if should create a new subflow */
> > while (msk->pm.local_addr_used < endp_subflow_max &&
> > msk->pm.extra_subflows < limit_extra_subflows) {
> > struct mptcp_addr_info addrs[MPTCP_PM_ADDR_MAX];
> > bool fullmesh;
> > int i, nr;
> >
> > /* No need to try establishing subflows to remote
> > id0 if not allowed */
> > if (mptcp_pm_add_addr_c_flag_case(msk))
> > break;
>
> I'm not convinced: that means these conditions will be checked for
> each
> local address, for each iteration here, while the result will be
> always the
> same.
>
> Here it sounds better to me to skip the selection before starting, if
> there
> is no need to do that.
If so, please add my reviewed-by tag and apply this set. Sorry for the
delay.
Reviewed-by: Geliang Tang <geliang@kernel.org>
Thanks,
-Geliang
>
> Cheers,
> Matt
>
> >
> > if (signal_and_subflow)
> > signal_and_subflow = false;
> > else if (!select_local_address(pernet, msk,
> > &local))
> > break;
> >
> > WDYT?
> >
> > Thanks,
> > -Geliang
> >
> > > mptcp_pm_nl_check_work_pending(msk);
> > > }
> > >
> > >
>
© 2016 - 2026 Red Hat, Inc.