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