Re: [PATCH mptcp-next v6 02/14] mptcp: handle local addrs announced by userspace PMs

[Kishen Maloor <kishen.maloor@intel.com>]

On 4/1/22 7:43 AM, Paolo Abeni wrote:
> On Mon, 2022-03-28 at 22:14 -0400, Kishen Maloor wrote:
>> This change adds an internal function to store/retrieve local
>> addrs announced by userspace PM implementations to/from its kernel
>> context. The function addresses the requirements of three scenarios:
>> 1) ADD_ADDR announcements (which require that a local id be
>> provided), 2) retrieving the local id associated with an address,
>> and also where one may need to be assigned, and 3) reissuance of
>> ADD_ADDRs when there's a successful match of addr/id.
>>
>> The list of all stored local addr entries is held under the
>> MPTCP sock structure. Memory for these entries is allocated from
>> the sock option buffer, so the list of addrs is bounded by optmem_max.
>> The list if not released via REMOVE_ADDR signals is ultimately
>> freed when the sock is destructed.
>>
>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
>> ---
>> v6:
>> -Move local_addr_list into struct mptcp_pm_data.
>> -Replace mptcp_data_lock() with the pm spinlock.
>> -Move mptcp_userspace_pm_append_new_local_addr() and
>> mptcp_free_local_addr_list() into a new pm_userspace.c.
>> -Make addresses_equal() helper non-static and rename to
>> mptcp_addresses_equal().
>> ---
>>  net/mptcp/Makefile       |  2 +-
>>  net/mptcp/pm.c           |  1 +
>>  net/mptcp/pm_netlink.c   | 34 +++++++++---------
>>  net/mptcp/pm_userspace.c | 74 ++++++++++++++++++++++++++++++++++++++++
>>  net/mptcp/protocol.c     |  1 +
>>  net/mptcp/protocol.h     |  7 ++++
>>  6 files changed, 101 insertions(+), 18 deletions(-)
>>  create mode 100644 net/mptcp/pm_userspace.c
>>
>> diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
>> index e54daceac58b..cb7f53f6ab22 100644
>> --- a/net/mptcp/Makefile
>> +++ b/net/mptcp/Makefile
>> @@ -2,7 +2,7 @@
>>  obj-$(CONFIG_MPTCP) += mptcp.o
>>  
>>  mptcp-y := protocol.o subflow.o options.o token.o crypto.o ctrl.o pm.o diag.o \
>> -	   mib.o pm_netlink.o sockopt.o
>> +	   mib.o pm_netlink.o sockopt.o pm_userspace.o
>>  
>>  obj-$(CONFIG_SYN_COOKIES) += syncookies.o
>>  obj-$(CONFIG_INET_MPTCP_DIAG) += mptcp_diag.o
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index 5d6832c4d9f2..cdc2d79071f8 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -469,6 +469,7 @@ void mptcp_pm_data_init(struct mptcp_sock *msk)
>>  {
>>  	spin_lock_init(&msk->pm.lock);
>>  	INIT_LIST_HEAD(&msk->pm.anno_list);
>> +	INIT_LIST_HEAD(&msk->pm.userspace_pm_local_addr_list);
>>  	mptcp_pm_data_reset(msk);
>>  }
>>  
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index 8d04c8d8a8df..836f6df9f744 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -55,8 +55,8 @@ struct pm_nl_pernet {
>>  #define MPTCP_PM_ADDR_MAX	8
>>  #define ADD_ADDR_RETRANS_MAX	3
>>  
>> -static bool addresses_equal(const struct mptcp_addr_info *a,
>> -			    const struct mptcp_addr_info *b, bool use_port)
>> +bool mptcp_addresses_equal(const struct mptcp_addr_info *a,
>> +			   const struct mptcp_addr_info *b, bool use_port)
>>  {
>>  	bool addr_equals = false;
>>  
>> @@ -120,7 +120,7 @@ static bool lookup_subflow_by_saddr(const struct list_head *list,
>>  		skc = (struct sock_common *)mptcp_subflow_tcp_sock(subflow);
>>  
>>  		local_address(skc, &cur);
>> -		if (addresses_equal(&cur, saddr, saddr->port))
>> +		if (mptcp_addresses_equal(&cur, saddr, saddr->port))
>>  			return true;
>>  	}
>>  
>> @@ -138,7 +138,7 @@ static bool lookup_subflow_by_daddr(const struct list_head *list,
>>  		skc = (struct sock_common *)mptcp_subflow_tcp_sock(subflow);
>>  
>>  		remote_address(skc, &cur);
>> -		if (addresses_equal(&cur, daddr, daddr->port))
>> +		if (mptcp_addresses_equal(&cur, daddr, daddr->port))
>>  			return true;
>>  	}
>>  
>> @@ -262,7 +262,7 @@ mptcp_lookup_anno_list_by_saddr(const struct mptcp_sock *msk,
>>  	lockdep_assert_held(&msk->pm.lock);
>>  
>>  	list_for_each_entry(entry, &msk->pm.anno_list, list) {
>> -		if (addresses_equal(&entry->addr, addr, true))
>> +		if (mptcp_addresses_equal(&entry->addr, addr, true))
>>  			return entry;
>>  	}
>>  
>> @@ -279,7 +279,7 @@ bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk)
>>  
>>  	spin_lock_bh(&msk->pm.lock);
>>  	list_for_each_entry(entry, &msk->pm.anno_list, list) {
>> -		if (addresses_equal(&entry->addr, &saddr, true)) {
>> +		if (mptcp_addresses_equal(&entry->addr, &saddr, true)) {
>>  			ret = true;
>>  			goto out;
>>  		}
>> @@ -414,7 +414,7 @@ static bool lookup_address_in_vec(const struct mptcp_addr_info *addrs, unsigned
>>  	int i;
>>  
>>  	for (i = 0; i < nr; i++) {
>> -		if (addresses_equal(&addrs[i], addr, addr->port))
>> +		if (mptcp_addresses_equal(&addrs[i], addr, addr->port))
>>  			return true;
>>  	}
>>  
>> @@ -450,7 +450,7 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
>>  		mptcp_for_each_subflow(msk, subflow) {
>>  			ssk = mptcp_subflow_tcp_sock(subflow);
>>  			remote_address((struct sock_common *)ssk, &addrs[i]);
>> -			if (deny_id0 && addresses_equal(&addrs[i], &remote, false))
>> +			if (deny_id0 && mptcp_addresses_equal(&addrs[i], &remote, false))
>>  				continue;
>>  
>>  			if (!lookup_address_in_vec(addrs, i, &addrs[i]) &&
>> @@ -483,7 +483,7 @@ __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 && addresses_equal(&entry->addr, info, true)) ||
>> +		if ((!lookup_by_id && mptcp_addresses_equal(&entry->addr, info, true)) ||
>>  		    (lookup_by_id && entry->addr.id == info->id))
>>  			return entry;
>>  	}
>> @@ -498,7 +498,7 @@ lookup_id_by_addr(const struct pm_nl_pernet *pernet, const struct mptcp_addr_inf
>>  
>>  	rcu_read_lock();
>>  	list_for_each_entry(entry, &pernet->local_addr_list, list) {
>> -		if (addresses_equal(&entry->addr, addr, entry->addr.port)) {
>> +		if (mptcp_addresses_equal(&entry->addr, addr, entry->addr.port)) {
>>  			ret = entry->addr.id;
>>  			break;
>>  		}
>> @@ -732,7 +732,7 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>>  		struct mptcp_addr_info local;
>>  
>>  		local_address((struct sock_common *)ssk, &local);
>> -		if (!addresses_equal(&local, addr, addr->port))
>> +		if (!mptcp_addresses_equal(&local, addr, addr->port))
>>  			continue;
>>  
>>  		if (subflow->backup != bkup)
>> @@ -902,9 +902,9 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
>>  	 * singled addresses
>>  	 */
>>  	list_for_each_entry(cur, &pernet->local_addr_list, list) {
>> -		if (addresses_equal(&cur->addr, &entry->addr,
>> -				    address_use_port(entry) &&
>> -				    address_use_port(cur))) {
>> +		if (mptcp_addresses_equal(&cur->addr, &entry->addr,
>> +					  address_use_port(entry) &&
>> +					  address_use_port(cur))) {
>>  			/* allow replacing the exiting endpoint only if such
>>  			 * endpoint is an implicit one and the user-space
>>  			 * did not provide an endpoint id
>> @@ -1031,14 +1031,14 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>>  	 */
>>  	local_address((struct sock_common *)msk, &msk_local);
>>  	local_address((struct sock_common *)skc, &skc_local);
>> -	if (addresses_equal(&msk_local, &skc_local, false))
>> +	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
>>  		return 0;
>>  
>>  	pernet = net_generic(sock_net((struct sock *)msk), pm_nl_pernet_id);
>>  
>>  	rcu_read_lock();
>>  	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
>> -		if (addresses_equal(&entry->addr, &skc_local, entry->addr.port)) {
>> +		if (mptcp_addresses_equal(&entry->addr, &skc_local, entry->addr.port)) {
>>  			ret = entry->addr.id;
>>  			break;
>>  		}
>> @@ -1409,7 +1409,7 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
>>  			goto next;
>>  
>>  		local_address((struct sock_common *)msk, &msk_local);
>> -		if (!addresses_equal(&msk_local, addr, addr->port))
>> +		if (!mptcp_addresses_equal(&msk_local, addr, addr->port))
>>  			goto next;
>>  
>>  		lock_sock(sk);
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> new file mode 100644
>> index 000000000000..cb8321cdb4ea
>> --- /dev/null
>> +++ b/net/mptcp/pm_userspace.c
>> @@ -0,0 +1,74 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include "protocol.h"
>> +
>> +void mptcp_free_local_addr_list(struct mptcp_sock *msk)
>> +{
>> +	struct mptcp_pm_addr_entry *entry, *tmp;
>> +	struct sock *sk = (struct sock *)msk;
>> +	LIST_HEAD(free_list);
>> +
>> +	if (!mptcp_pm_is_userspace(msk))
>> +		return;
>> +
>> +	spin_lock_bh(&msk->pm.lock);
>> +	list_splice_init(&msk->pm.userspace_pm_local_addr_list, &free_list);
>> +	spin_unlock_bh(&msk->pm.lock);
>> +
>> +	list_for_each_entry_safe(entry, tmp, &free_list, list) {
>> +		sock_kfree_s(sk, entry, sizeof(*entry));
>> +	}
>> +}
>> +
>> +int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>> +					     struct mptcp_pm_addr_entry *entry)
>> +{
>> +	DECLARE_BITMAP(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
>> +	struct mptcp_pm_addr_entry *match = NULL;
>> +	struct sock *sk = (struct sock *)msk;
>> +	struct mptcp_pm_addr_entry *e;
>> +	bool addr_match = false;
>> +	bool id_match = false;
>> +	int ret = -EINVAL;
>> +
>> +	bitmap_zero(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
>> +
>> +	spin_lock_bh(&msk->pm.lock);
>> +	list_for_each_entry(e, &msk->pm.userspace_pm_local_addr_list, list) {
>> +		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
>> +		if (addr_match && entry->addr.id == 0)
>> +			entry->addr.id = e->addr.id;
>> +		id_match = (e->addr.id == entry->addr.id);
>> +		if (addr_match && id_match) {
>> +			match = e;
>> +			break;
>> +		} else if (addr_match || id_match) {
>> +			break;
>> +		}
>> +		__set_bit(e->addr.id, id_bitmap);
>> +	}
>> +
>> +	if (!match && !addr_match && !id_match) {
>> +		/* Memory for the entry is allocated from the
>> +		 * sock option buffer.
>> +		 */
>> +		e = sock_kmalloc(sk, sizeof(*e), GFP_ATOMIC);
>> +		if (!e) {
>> +			spin_unlock_bh(&msk->pm.lock);
>> +			return -ENOMEM;
>> +		}
>> +
>> +		*e = *entry;
>> +		if (!e->addr.id)
>> +			e->addr.id = find_next_zero_bit(id_bitmap,
>> +							MPTCP_PM_MAX_ADDR_ID + 1,
>> +							1);
>> +		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
>> +		ret = e->addr.id;
>> +	} else if (match) {
>> +		ret = entry->addr.id;
>> +	}
>> +
>> +	spin_unlock_bh(&msk->pm.lock);
>> +	return ret;
>> +}
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index b2c654992de0..b016822f44dc 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -3103,6 +3103,7 @@ void mptcp_destroy_common(struct mptcp_sock *msk)
>>  	msk->rmem_fwd_alloc = 0;
>>  	mptcp_token_destroy(msk);
>>  	mptcp_pm_free_anno_list(msk);
>> +	mptcp_free_local_addr_list(msk);
>>  }
>>  
>>  static void mptcp_destroy(struct sock *sk)
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 187c932deef0..40dabf9462a8 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -208,6 +208,7 @@ struct mptcp_pm_data {
>>  	struct mptcp_addr_info local;
>>  	struct mptcp_addr_info remote;
>>  	struct list_head anno_list;
>> +	struct list_head userspace_pm_local_addr_list;
> 
> It looks like anno_list is never used when the user-space PM is enabled
> - and userspace_pm_local_addr_list is never used by the NL PM.
> 
> What about reusing the same, single field? Ev. using an union, but not
> strictly needed, since in both case this contains addresses [to be]
> announced, right?
> 
> I really think we should avoid adding more data to the msk struct for
> user-space PM consumption only, it's really counterintuitive.


anno_list is used by both PMs to store struct mptcp_pm_add_entry entries.

userspace_pm_local_addr_list is used only to store per-msk 
struct mptcp_pm_addr_entry entries that were populated by the userspace PM.

Unfortunately the local_addr_list used by the in-kernel PM is in the
pm_nl_pernet structure.