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 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