Re: [PATCH mptcp-next RFC 3/3] mptcp: pm: kernel: skip operating on closing connections

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

On 17/08/2026 09:39, Paolo Abeni wrote:
> On 8/14/26 8:31 PM, Matthieu Baerts (NGI0) wrote:
>> When iterating over each MPTCP connection after having manipulated MPTCP
>> endpoints, there is no need to operate on closing (or not ready)
>> connections.
>>
>> We can then safely skip those.
>>
>> Note that mptcp_nl_add_subflow_or_signal_addr() was already checking the
>> fully_established state, but it is better to check the connection state,
>> which is what mptcp_is_fully_established() is doing.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/pm_kernel.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>> index d3014bf57bf3..c185e53fe42b 100644
>> --- a/net/mptcp/pm_kernel.c
>> +++ b/net/mptcp/pm_kernel.c
>> @@ -969,7 +969,7 @@ static int mptcp_nl_add_subflow_or_signal_addr(struct net *net,
>>  		struct sock *sk = (struct sock *)msk;
>>  		struct mptcp_addr_info mpc_addr;
>>  
>> -		if (!READ_ONCE(msk->fully_established) ||
>> +		if (!mptcp_is_fully_established(sk) ||
>>  		    mptcp_pm_is_userspace(msk))
>>  			goto next;
> 
> I'm a little double-minded here.
> 
> The add/removal operations are issued by the user space to update
> subflows, as a consequence of endpoint addition/removal.
> 
> Possibly removing the subflow even for not fully established connections
> could be the right thing to do, I'm not 110% sure either way. Possibly
> worth to call this out with a comment?

Indeed, I'm also not sure. Initially, I only converted this
READ_ONCE(msk->fully_established), also because later in
mptcp_pm_create_subflow_or_signal_addr(), mptcp_is_fully_established()
is used.

Probably best to stick to this one.

On the other hand, I just noticed that there is no check for "fallback
done" here: so the operations leading to a notification (ADD_ADDR,
RM_ADDR) will send a dup-ACK (pure ACK without MPTCP options), and
adding an MPTCP endpoint will lead to... the creation of a new subflow,
even after a fallback, e.g.

-------------------------- 8< --------------------------
C > S: Flags [S], seq 0, win 65535, options [mss 1460,sackOK,nop,nop,nop,wscale 6,mptcp 4 capable v1], length 0
S > C: Flags [S.], seq 2252073282, ack 1, win 65535, options [mss 1460,nop,nop,sackOK,nop,wscale 8,mptcp 12 capable v1 {0xb80fe7957527e1f7}], length 0
C > S: Flags [.], ack 1, win 2048, options [mptcp 20 capable v1 {0x200000000000000,0xb80fe7957527e1f7}], length 0
C > S: Flags [P.], seq 1:501, ack 1, win 2048, options [mptcp 22 capable v1 {0xb80fe7957527e1f7,0x200000000000000},nop,nop], length 500: HTTP
S > C: Flags [.], ack 501, win 1049, options [mptcp 12 dss ack 13263177308786789273], length 0

// Here, the client receives the ACK without MPTCP options due to middleboxes → fallback

S > C: Flags [P.], seq 1:101, ack 501, win 1049, options [mptcp 26 dss ack 13263177308786789273 seq 14591534807969507511 subseq 1 len 100,nop,nop], length 100: HTTP
C > S: Flags [.], ack 101, win 2048, length 0

// fallback on the server side

C > S: Flags [P.], seq 501:601, ack 101, win 2048, length 100: HTTP
S > C: Flags [.], ack 601, win 1049, length 0

// Data OK
// Adding a new subflow endpoint now:

S2 > C: Flags [S], seq 618398320, win 65535, options [mss 1460,sackOK,TS val 3041525989 ecr 0,nop,wscale 8,mptcp 12 join id 1 token 0xd86e8112 nonce 0x6cca79f5], length 0
-------------------------- 8< --------------------------

Oops. Still, not a big deal, but probably better and safer to avoid
that, no?

Maybe patch 1 (reset fully_estab after a fallback) and this patch here
but only looking at READ_ONCE(msk->fully_established) is enough?
Patch 2 would be for -next.

> Also the asymmetry with the user-space PM is a bit strange. I can't
> recall why it was needed in the first place?!?

Are you talking about all the mptcp_pm_is_userspace() checks? If yes,
they are required because the pm_type sysctl could be changed, and we
could end-up with a mix of msk handled by different PMs in the same
netns.

It would be better to do the opposite I guess:

  if (!mptcp_pm_is_kernel(msk))
      goto next;

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