Re: [PATCH mptcp-net 2/7] mptcp: pm: userspace: respect deny_join_id0 attr

[Matthieu Baerts <matttbe@kernel.org>]

Hi Christoph, Mat,

On 30/08/2025 02:58, Mat Martineau wrote:
> On Fri, 29 Aug 2025, Christoph Paasch wrote:
> 
>> On Fri, Aug 29, 2025 at 1:33 PM Matthieu Baerts (NGI0)
>> <matttbe@kernel.org> wrote:
>>>
>>> During the connection establishment, a peer can tell the other that it
>>> cannot establish new subflows to the initial IP address and port by
>>> setting the 'C' flag [1]. The RFC8684 is strict about that:
>>>
>>>   (...) therefore the receiver MUST NOT try to open any additional
>>>   subflows toward this address and port.
>>>
>>> It is then important not to let the userspace PM establishing such
>>> subflows, and return an error (ECONNREFUSED) if it tries to do so.
>>>
>>> Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow
>>> establishment")
>>> Link: https://datatracker.ietf.org/doc/html/rfc8684\#section-3.1-20.6
>>> [1]
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>> ---
>>>  net/mptcp/pm_userspace.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>>> index
>>> 1911fe1799fa38a53381247a830a9a0daf1c4492..9968dc9a8b45112114953f66848ea22c971136d6 100644
>>> --- a/net/mptcp/pm_userspace.c
>>> +++ b/net/mptcp/pm_userspace.c
>>> @@ -391,6 +391,12 @@ int mptcp_pm_nl_subflow_create_doit(struct
>>> sk_buff *skb, struct genl_info *info)
>>>         if (err < 0)
>>>                 goto create_err;
>>>
>>> +       if (READ_ONCE(msk->pm.remote_deny_join_id0) && addr_r.id == 0) {
>>
>> If userspace wants to, it could still work around that by still using
>> the initial subflow's IP-address and just use a non-zero address-ID.

Good point, I missed that!

>> Is that really a concern ? Sure, the IETF-draft says "MUST NOT", and
>> so, shouldn't we leave it up to the userspace PM to make that decision
>> ?
>>
> 
> I concur with Christoph on this one, I think the PM (userspace or
> kernel) is responsible for this aspect of the protocol so it doesn't
> need to be double-checked here.

Indeed, we could leave it up to the different PMs to make that decision.
Initially, I didn't have this patch, and I had a simple patch setting
the corresponding sysctl to 0 in userspace_pm.sh, just to check the
event was correctly announced. But when writing the last patch's commit
message ("mptcp: pm: nl: announce deny-join-id0 attribute"), I realised
that the main missing feature was a way for the userspace daemon to know
that the C flag was set. We cannot backport patch 7/7, but we can
backport this one, and have a way to know this info on older kernels. I
should have mentioned that in the commit message.

In this case, I think we have a few options:

- Keep this patch, but update the commit message to make it clearer
what's its purpose, see [1]

- Same, but revert it after patch 7/7 on net-next.

- Keep it, but also check for the address and port.

- Drop it.

What would you prefer?

Personally, I think we should keep it, at least for stable, but we don't
need to check for the address and port: even if it is easy to check, it
might be good to let a way for the userspace to still create such subflow.

[1]: The last paragraph in this commit message would then be:

> It is then important to let the userspace daemon know it is not allowed
> to establish such subflows, and return an error (ECONNREFUSED) if it
> tries to do so. Note that the userspace daemon will still be able to
> establish subflows to the initial IP address and port, but by giving a
> different remote ID: that's OK, that's its responsibility ; worst case,
> the MP_JOIN is rejected by the other peer.

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