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)) ||
 		    (lookup_by_id && entry->addr.id == info->id))
 			return entry;
 	}
 	return NULL;
 }
 
-static int
-lookup_id_by_addr(const struct pm_nl_pernet *pernet, const struct mptcp_addr_info *addr)
-{
-	const struct mptcp_pm_addr_entry *entry;
-	int ret = -1;
-
-	rcu_read_lock();
-	list_for_each_entry(entry, &pernet->local_addr_list, list) {
-		if (mptcp_addresses_equal(&entry->addr, addr, entry->addr.port)) {
-			ret = entry->addr.id;
-			break;
-		}
-	}
-	rcu_read_unlock();
-	return ret;
-}
-
 static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 {
 	struct sock *sk = (struct sock *)msk;
@@ -546,13 +530,22 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 
 	/* do lazy endpoint usage accounting for the MPC subflows */
 	if (unlikely(!(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED))) && msk->first) {
+		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
+		struct mptcp_pm_addr_entry *entry;
 		struct mptcp_addr_info mpc_addr;
-		int mpc_id;
+		bool backup = false;
 
 		local_address((struct sock_common *)msk->first, &mpc_addr);
-		mpc_id = lookup_id_by_addr(pernet, &mpc_addr);
-		if (mpc_id >= 0)
-			__clear_bit(mpc_id, msk->pm.id_avail_bitmap);
+		rcu_read_lock();
+		entry = __lookup_addr(pernet, &mpc_addr, false);
+		if (entry) {
+			__clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
+			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+		}
+		rcu_read_unlock();
+
+		if (backup)
+			mptcp_pm_send_ack(msk, subflow, true, backup);
 
 		msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
 	}
-- 
2.35.3

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?

> 		    (lookup_by_id && entry->addr.id == info->id))
> 			return entry;
> 	}
> 	return NULL;
> }
>
> -static int
> -lookup_id_by_addr(const struct pm_nl_pernet *pernet, const struct mptcp_addr_info *addr)
> -{
> -	const struct mptcp_pm_addr_entry *entry;
> -	int ret = -1;
> -
> -	rcu_read_lock();
> -	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> -		if (mptcp_addresses_equal(&entry->addr, addr, entry->addr.port)) {
> -			ret = entry->addr.id;
> -			break;
> -		}
> -	}
> -	rcu_read_unlock();
> -	return ret;
> -}
> -
> static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> {
> 	struct sock *sk = (struct sock *)msk;
> @@ -546,13 +530,22 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>
> 	/* do lazy endpoint usage accounting for the MPC subflows */
> 	if (unlikely(!(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED))) && msk->first) {
> +		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
> +		struct mptcp_pm_addr_entry *entry;
> 		struct mptcp_addr_info mpc_addr;
> -		int mpc_id;
> +		bool backup = false;
>
> 		local_address((struct sock_common *)msk->first, &mpc_addr);
> -		mpc_id = lookup_id_by_addr(pernet, &mpc_addr);
> -		if (mpc_id >= 0)
> -			__clear_bit(mpc_id, msk->pm.id_avail_bitmap);
> +		rcu_read_lock();
> +		entry = __lookup_addr(pernet, &mpc_addr, false);
> +		if (entry) {
> +			__clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
> +			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> +		}
> +		rcu_read_unlock();
> +
> +		if (backup)
> +			mptcp_pm_send_ack(msk, subflow, true, backup);

This looks ok to me. I think I follow your example in #285 for how to 
create endpoints that would allow the priority to be changed later, but I 
think the selftests might make that clearer.

Thanks!

>
> 		msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
> 	}
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

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?

Thanks!

Paolo

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