Re: [PATCH mptcp-next v3 18/29] mptcp: add userspace pm addr entry refcount

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds userspace PM address entry refcount. Add a new filed
> 'refcont' in struct mptcp_pm_addr_entry, inited to 1.

Small nits: s/refcont/refcnt/  and s/inited/initiated/

(same comment for the next patch)

> Increase this counter in mptcp_nl_cmd_sf_create(), and decrease it in
> mptcp_userspace_pm_delete_local_addr() according the subflows value.

Please *always* explain why this commit is needed: why do we need a
refcount per address entry? Feel free to look at the ticket 403 for
inspirations.

(same comment for the next patch)

Also, please add the Closes tag:

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403

Another thing: should we consider this as a bug-fix? I think we can
because without this modification, we were not able to send a RM_ADDR if
another subflow using the same local address has been removed before. It
should not be a too annoying issue but if we consider it as an issue,
this should target 'mptcp-net' and have a Fixes tag:

Fixes: 24430f8bf516 ("mptcp: add address into userspace pm list")

One last thing: do you mind adding a new test to cover this case please?
e.g.

- the client creates an MPTCP connection: A <-> A
- it asks the userspace PM to add 2 subflows using the same source IP
address: B <-> B ; B <-> C
- it deletes one subflow: B <-> B
- it sends a RM_ADDR for B

Before the patch, it should fail: the kernel has removed the
corresponding entry for the local address from the list when removing
the subflow while another subflow was using the same local address.
After the patch, it should succeed.

Also, trying to send yet another RM_ADDR for B after the previous one
should result in an expected error. (it is possible we don't handle that
correctly)

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 21 +++++++++++++++------
>  net/mptcp/protocol.h     |  2 ++
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 30f4dd074a70..8efca1602e11 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -70,6 +70,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  							1);
>  		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
>  		msk->pm.local_addr_used++;
> +		refcount_set(&e->refcnt, 1);
>  		ret = e->addr.id;
>  	} else if (match) {
>  		ret = entry->addr.id;

Should you not increment the refcount here if there is a match?

> @@ -107,9 +108,12 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  	if (!entry)
>  		return -EINVAL;
>  
> -	/* TODO: a refcount is needed because the entry can
> -	 * be used multiple times (e.g. fullmesh mode).
> -	 */
> +	if (!refcount_dec_not_one(&entry->refcnt)) {
> +		pr_debug("userspace refcount error: refcnt=%d",
> +			 refcount_read(&entry->refcnt));
> +		return -EINVAL;

I'm not sure to understand why you treat that as an error.

Should you not simply use refcount_dec_and_test()? If after the
decrement, the counter is not 0, there is nothing to do: the entry is
still being used by another subflow, that's fine. You can keep a
pr_debug() but please do not mention 'error' and do not return a
negative value, no?

> +	}
> +
>  	list_del_rcu(&entry->list);
>  	kfree(entry);
>  	msk->pm.local_addr_used--;
> @@ -387,10 +391,15 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
>  	release_sock(sk);
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	if (err)
> +	if (err) {
>  		mptcp_userspace_pm_delete_local_addr(msk, &local);
> -	else
> -		msk->pm.subflows++;
> +	} else {
> +		struct mptcp_pm_addr_entry *entry;
> +
> +		entry = mptcp_userspace_pm_get_entry(msk, &addr_l);
> +		if (entry && refcount_inc_not_zero(&entry->refcnt))

I don't think you need to change the code here: before creating the
subflow, 'mptcp_userspace_pm_append_new_local_addr()' has been called
which has initialised or incremented the refcount. Then it cannot be 0
and it doesn't need to be incremented, no?

> +			msk->pm.subflows++;
> +	}
>  	spin_unlock_bh(&msk->pm.lock);
>  
>   create_err:

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net