Re: [PATCH RFC 3/4] mptcp: allow the in kernel PM to set MPC subflow priority

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

On Wed, 29 Jun 2022, Paolo Abeni wrote:

> On Tue, 2022-06-28 at 17:11 -0700, Mat Martineau wrote:
>> On Fri, 24 Jun 2022, Paolo Abeni wrote:
>>
>>> Any local endpoints configured on the address matching the
>>> MPC subflow are currently ignored.
>>>
>>> Specifically, setting a backup flag on them has no effect
>>> on the first subflow, as the MPC handshake can't carry such
>>> info.
>>>
>>> This change refactors the MPC endpoint id accounting to
>>> additionally fetch the priority info from the relevant endpoint
>>> and eventually trigger the MP_PRIO handshake as needed.
>>>
>>> As a result, the MPC subflow now switches to backup priority
>>> after that the MPTCP socket is fully established, according
>>> to the local endpoint configuration.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> net/mptcp/pm_netlink.c | 37 +++++++++++++++----------------------
>>> 1 file changed, 15 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 91f6ed2a076a..a6eb501e5031 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -505,30 +505,14 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
>>> 	struct mptcp_pm_addr_entry *entry;
>>>
>>> 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
>>> -		if ((!lookup_by_id && mptcp_addresses_equal(&entry->addr, info, true)) ||
>>> +		if ((!lookup_by_id &&
>>> +		     mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) ||
>>
>> It seems like we could have multiple entries in the local_addr_list with
>> the same address, but with different entries having port==0 or nonzero
>> ports. If mptcp_nl_cmd_set_flags() is called with a nonzero port, but this
>> lookup function encounters the port==0 entry first, this will match an
>> unexpected entry in local_addr_list.
>>
>> Is there some other constraint preventing this?
>
> Uhmmm.. I updated the mptcp_addresses_equal() statement to match the
> existing one in lookup_id_by_addr() below.
>
> You are right, this check is uncorrect, and generally speaking the
> mptcp_addresses_equal() invocations are a bit fuzzy. I'll try to
> clarify that with an additional patches:
>
> - mptcp_addresses_equal() on subflow list use port matching if the
> looked-up address has  non 0 port (the current code looks correct)
>
> - mptcp_addresses_equal on local_addr_list should use port matching if
> address_use_port(entry), and entry with port matching should be
> preferred over entries without port-matching. The latter can be
> enforces adding entries with address_use_port(entry) at the list head
> and entries with !address_use_port(entry) at tail
>
> WDYT?

That looks like a good plan - thanks

--
Mat Martineau
Intel