Re: [MPTCP][PATCH v5 mptcp-next 2/5] mptcp: local addresses fullmesh

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2021-07-27 at 15:58 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> In mptcp_pm_nl_add_addr_received(), fill a temporary allocate array of
> all local address corresponding to the fullmesh endpoint. If such array
> is empty, keep the current behavior.
> 
> Elsewhere loop on such array and create a subflow for each local address
> towards the given remote address
> 
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/pm_netlink.c | 60 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 2259c424485f..a85bac950f3b 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -554,13 +554,62 @@ static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
>  	mptcp_pm_create_subflow_or_signal_addr(msk);
>  }
>  
> +static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
> +					     struct mptcp_addr_info *remote,
> +					     struct mptcp_pm_addr_entry *entries)

Minor nit: some comments here before the function describing it would
be helpful, thanks!

Also 'remote' could be 'const', I think.

> +{
> +	struct mptcp_pm_addr_entry local, *entry;
> +	struct sock *sk = (struct sock *)msk;
> +	struct pm_nl_pernet *pernet;
> +	unsigned int subflows_max;
> +	int i = 0;
> +
> +	pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
> +	subflows_max = mptcp_pm_get_subflows_max(msk);
> +
> +	rcu_read_lock();
> +	__mptcp_flush_join_list(msk);
> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
> +		if (!(entry->flags & MPTCP_PM_ADDR_FLAG_FULLMESH))
> +			continue;
> +
> +		if (entry->addr.family != sk->sk_family) {
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +			if ((entry->addr.family == AF_INET &&
> +			     !ipv6_addr_v4mapped(&sk->sk_v6_daddr)) ||
> +			    (sk->sk_family == AF_INET &&
> +			     !ipv6_addr_v4mapped(&entry->addr.addr6)))
> +#endif
> +				continue;
> +		}
> +
> +		if (!lookup_subflow_by_addrs(&msk->conn_list, &entry->addr, remote) &&
> +		    msk->pm.subflows < subflows_max) {
> +			msk->pm.subflows++;
> +			entries[i++] = *entry;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	if (!i) {
> +		memset(&local, 0, sizeof(local));
> +		local.addr.family = remote->family;
> +
> +		msk->pm.subflows++;
> +		entries[i++] = local;
> +	}
> +
> +	return i;
> +}
> +
>  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  {
> +	struct mptcp_pm_addr_entry entries[MPTCP_PM_ADDR_MAX];

mptcp_pm_addr_entry is quite larger than mptcp_addr_info (should be 64
bytes vs 24). 64 * 8 == 512 bytes could be a bit too much storage for
the stack. What about using instead:

struct mptcp_pm_addr_info addresses[MPTCP_PM_ADDR_MAX];
u8 flags[MPTCP_PM_ADDR_MAX];

?

And than pass the 2 arguments to fill_local_addresses_vec(), instead of
the single 'entries' arg.

Cheers,

Paolo