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

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2022-03-16 at 19:16 -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>
> ---
>  net/mptcp/pm_netlink.c | 72 ++++++++++++++++++++++++++++++++++++++++++
>  net/mptcp/protocol.c   |  2 ++
>  net/mptcp/protocol.h   |  2 ++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 98e59576415b..d8825bf580b7 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -390,6 +390,24 @@ static bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
>  	return true;
>  }
>  
> +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;
> +
> +	mptcp_data_lock(sk);
> +	list_splice_init(&msk->local_addr_list, &free_list);
> +	mptcp_data_unlock(sk);

Why are you using the mptcp data lock here ? I think that is subject to
lockdep issues. Likely the pm spinlock is a more suitable lock.

> +
> +	list_for_each_entry_safe(entry, tmp, &free_list, list) {
> +		sock_kfree_s(sk, entry, sizeof(*entry));
> +	}
> +}
> +
>  void mptcp_pm_free_anno_list(struct mptcp_sock *msk)
>  {
>  	struct mptcp_pm_add_entry *entry, *tmp;
> @@ -878,6 +896,60 @@ static void __mptcp_pm_release_addr_entry(struct mptcp_pm_addr_entry *entry)
>  	kfree(entry);
>  }
>  
> +static 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);
> +
> +	mptcp_data_lock(sk);
> +	list_for_each_entry(e, &msk->local_addr_list, list) {
> +		addr_match = 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) {
> +			mptcp_data_unlock(sk);
> +			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->local_addr_list);
> +		ret = e->addr.id;
> +	} else if (match) {
> +		ret = entry->addr.id;
> +	}
> +
> +	mptcp_data_unlock(sk);

After "mptcp: introduce implicit endpoints", the above mimics very
closely what mptcp_pm_nl_get_local_id() is already doing for the in
kernel path manager, with the main differences that it avoid the hard
MPTCP_PM_ADDR_MAX limit.

Perhaps we can re-use the same code? e.g. enforcing the
MPTCP_PM_ADDR_MAX only for the in-kernel path manager, or possibly
retaining such constraint even for the user-space one?

/P