Re: [PATCH mptcp-next v3 17/29] mptcp: add userspace_pm_get_entry helper

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

Thank you for looking at the issue 403!

On 25/09/2023 10:41, Geliang Tang wrote:
> This patch adds a new helper mptcp_userspace_pm_get_entry() to find out
> the address entry on the userspace_pm_local_addr_list through the given
> address. Use this helper in mptcp_userspace_pm_delete_local_addr() and
> mptcp_nl_cmd_sf_destroy().
Please always explain why this is needed, e.g. for this case:

  This will be reused in the following commits.

(also, please see my comment here below and on the next commit: maybe
you don't need this helper? → Except to clarify the code?)

> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 43 ++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 8d97cf475cac..30f4dd074a70 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -80,6 +80,19 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  	return ret;
>  }
>  
> +static struct mptcp_pm_addr_entry *mptcp_userspace_pm_get_entry(struct mptcp_sock *msk,
> +								struct mptcp_addr_info *addr)
> +{
> +	struct mptcp_pm_addr_entry *entry, *tmp;
> +
> +	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> +		if (mptcp_addresses_equal(&entry->addr, addr, false))
> +			return entry;
> +	}
> +
> +	return NULL;
> +}
> +
>  /* If the subflow is closed from the other peer (not via a
>   * subflow destroy command then), we want to keep the entry
>   * not to assign the same ID to another address and to be
> @@ -88,21 +101,19 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  						struct mptcp_pm_addr_entry *addr)
>  {
> -	struct mptcp_pm_addr_entry *entry, *tmp;
> +	struct mptcp_pm_addr_entry *entry;
>  
> -	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> -		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
> -			/* TODO: a refcount is needed because the entry can
> -			 * be used multiple times (e.g. fullmesh mode).
> -			 */
> -			list_del_rcu(&entry->list);
> -			kfree(entry);
> -			msk->pm.local_addr_used--;
> -			return 0;
> -		}
> -	}
> +	entry = mptcp_userspace_pm_get_entry(msk, &addr->addr);
> +	if (!entry)
> +		return -EINVAL;
>  
> -	return -EINVAL;
> +	/* TODO: a refcount is needed because the entry can
> +	 * be used multiple times (e.g. fullmesh mode).
> +	 */
> +	list_del_rcu(&entry->list);
> +	kfree(entry);
> +	msk->pm.local_addr_used--;
> +	return 0;
>  }
>  
>  int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> @@ -495,10 +506,12 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
>  	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
>  	if (ssk) {
>  		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> -		struct mptcp_pm_addr_entry entry = { .addr = addr_l };
> +		struct mptcp_pm_addr_entry *entry;
>  
>  		spin_lock_bh(&msk->pm.lock);
> -		mptcp_userspace_pm_delete_local_addr(msk, &entry);
> +		entry = mptcp_userspace_pm_get_entry(msk, &addr_l);

Why are you calling this function here? ... (see below)

> +		if (entry)
> +			mptcp_userspace_pm_delete_local_addr(msk, entry);

Because this function will also call mptcp_userspace_pm_get_entry().

>  		spin_unlock_bh(&msk->pm.lock);
>  		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
>  		mptcp_close_ssk(sk, ssk, subflow);

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