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

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.

>> 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.

/P