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

On 17/08/2026 15:09, Paolo Abeni wrote:
> On 8/17/26 1:07 PM, Matthieu Baerts wrote:
>> 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) <[email protected]>
>>>> ---
>>>>  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.
> 
> Sorry, I was not clear enough. I'm specifically doubtful WRT the remove
> operations (i.e. mptcp_nl_flush_addrs_list,
> mptcp_nl_remove_id_zero_address,
> mptcp_nl_remove_subflow_and_signal_addr) and _not_ vs the 'add' one
> (mptcp_nl_add_subflow_or_signal_addr). I'm fine with the check change there.

Sorry, I think it was me not being clear enough:

- In mptcp_nl_add_subflow_or_signal_addr: use mptcp_is_fully_established
- In the others (flush, 2 remove): check only msk->fully_established

To avoid acting on fallback MPTCP connections, should we target patch 1
(reset fully_estab after a fallback) and this one here for -net?
Or only patch 1?

>>> 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;
> Yup, I prefer the check this way more.
> 
> My doubt is that user-space will allow creating subflow even for not
> established msk.

It shouldn't: in __mptcp_subflow_connect(), it will return early if
mptcp_is_fully_established() is false.

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