Re: [PATCH mptcp-next v4 3/5] mptcp: keep track of local endpoint still available for each msk

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Tue, 7 Dec 2021, Paolo Abeni wrote:

> On Mon, 2021-12-06 at 11:18 +0100, Paolo Abeni wrote:
>> On Fri, 2021-12-03 at 17:05 -0800, Mat Martineau wrote:
>>> On Fri, 3 Dec 2021, Paolo Abeni wrote:
>>>
>>>> Include into the path manager status a bitmap tracking the list
>>>> of local endpoints still available - not yet used - for the
>>>> relavant mptcp socket.
>>>> Keep such map updated at endpoint creation/deleteion time, so
>>>> that we can easily skip already used endpoint at local address
>>>> selection time.
>>>> This allows for fair local endpoints usage in case of subflow
>>>> failure.
>>>>
>>>> As a side effect, this patch also enforces that each endpoint
>>>> is used at most once for each mptcp connection.
>>>
>>> This seems like a significant change, possibly with unexpected
>>> consequences (but maybe I've misunderstood the new constraint?).
>>> Apparently the topoligies in our self tests happen to work fine!
>>
>> Uhmmm... probably less then it may looks like. We already respected the
>> above constraint in almost every scenarios. AFAICS, the only exception
>> prior to this patch was following scenario:
>>
>> 1) subflow is established endpoint A
>>
>> 2) the peer closes the additional subflow
>>
>> 2) a new local endpoint B is created
>>
>> 3) the PM will create the 2nd subflow using again endpoint A.
>>
>> which is arguibly unexpected, as reported by issues/242.
>>
>>> If you have peer A with one interface and peer B with two, it seems like
>>> this should be allowed:
>>>
>>> Peer A connects to Peer B endpoint 0
>>> Peer B announces endpoint 1
>>> Peer A sends an MP_JOIN to Peer B interface 1
>>>
>>> From the description above, it sounds like peer A would not be able to
>>> create that second subflow.
>>
>> Well, AFAICS the peer A will actually create the subflow, if the limits
>> allow that, both with or without this patch.
>> Specifically:
>>
>> 1) no endpoint configured, add_addr_accepted >=1, subflows >=1 on peer
>> A -> the subflow to peer be interface 1 will be created (with or
>> without this patch) because we don't have any other additional
>> constraint beyond the limits.
>>
>> 2) endpoint configured on peer A matching the MPC subflow source
>> address -> again the subflow towards peer B endpoint 1 will be created,
>> because that will be the first time the PM will use the peer A
>> endpoint.
>
> Thinking again about this last scenario, I think there is something to
> improve:
>
> if peer A connects to peer B using the local endpoint address, it can
> later use again such address for additional sublflows, as the MPC
> subflow is no accounted by the current version of this patch. I'll cook
> a v5 adding such account, too - and possibly a related self-tests
>

Thanks Paolo, that will help.

My main concern is what happens when the bitmap fills up. While 
__mptcp_subflow_connect() can still be called from 
mptcp_pm_nl_add_addr_received() (so, you're right, we can still create 
multiple outgoing connections this way), it won't be called from 
mptcp_pm_create_subflow_or_signal_addr() because select_local_address() 
will now return NULL. The latter case doesn't seem like expected/desired 
behavior.


--
Mat Martineau
Intel