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

[Matthieu Baerts <matttbe@kernel.org>]

On 02/09/2025 18:43, Christoph Paasch wrote:
> On Mon, Sep 1, 2025 at 3:08 AM Matthieu Baerts <matttbe@kernel.org> wrote:
>>
>> 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
> 
> Why not ? Yes, it is not typical to backport a new netlink message,
> but I think it makes sense here.
> It basically means that the fix involves not only the kernel but also
> the userspace PMs.

Yes, I guess we could backport it.

I still don't know if we should use the (existing but not used) "flags"
attribute, or a new dedicated one.

>> , 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.
> 
> The problem is that even this here is not complete, right ? A
> userspace pm can still attempt to connect to the IP of the initial
> subflow simply by using another address-ID.

Yes, but that's not an issue for me: that's because the userspace PM
didn't use the expected ID, so it is the userspace PM daemon
responsibility :)

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