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

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

On 3/21/22 3:53 AM, Paolo Abeni wrote:
> 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.

I needed a spin lock to serialize access to that list so had used mptcp_data_lock
instead of adding a new lock. But yeah, I could use the pm spinlock instead and may
be also move this list var into mptcp_pm_data.

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

I don't think so. The semantics of mptcp_userspace_pm_append_new_local_addr()
are different and work in concert with the ANNOUNCE impl. It always looks for a matching 
addr+port and id to return a stored addr; if one matches and the other doesn't it rejects
that insertion; if neither match it adds a new entry to the list; when adding an entry if
an id wasn't provided (which can happen only in the "implicit" case), one is assigned. 
Also, id assignments apply within the scope of each connection (i.e. per msk). 

> 
> /P
>