Hi Geliang,
On 14/04/2023 11:11, Geliang Tang wrote:
> Add the address into userspace_pm_local_addr_list when the subflow is
> created.
I'm sorry to send the same kind of comment as on the previous patches
and on the version 5 but can you also add the reason why you need to do
that in the commit message please?
Also, it looks like you are fixing two issues (or adding two features) here:
- Being able to send a remove addr for any additional subflow that have
been created (MPTCP_PM_CMD_SUBFLOW_CREATE) but not announced
(MPTCP_PM_CMD_ADD_ADDR): a part of issue #379 (this doesn't fix the
possibility to remove ID 0 from what I see)
- Increment local_addr_used: a part of issue #329
No?
Maybe could you split this in two? e.g. modifying local_addr_used
counter in another commit?
Could you add:
Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow
establishment")
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/379
and:
Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow
establishment")
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/329
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 312fdce174fa..99a3968f38ac 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -301,6 +301,17 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
> goto create_err;
> }
>
> + err = mptcp_userspace_pm_get_local_id(msk, &addr_l);
> + if (err < 0) {
> + GENL_SET_ERR_MSG(info, "did not match address and id");
> + goto create_err;
> + }
> +
> + spin_lock_bh(&msk->pm.lock);
> + mptcp_pm_alloc_anno_list(msk, &addr_l);
> + msk->pm.local_addr_used++;
Correct me if I'm wrong but if the local address has already been used
before, we are going to increment the counter while we should not, no?
e.g. if the client re-use the same local address to create multiple
subflows to different IP.
mptcp_userspace_pm_append_new_local_addr() should probably report if
there was a match of if a new entry has been added.
> + spin_unlock_bh(&msk->pm.lock);
> +
> lock_sock(sk);
>
> err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
Also, should we not increment the counter if there was no errors? (or
maybe in case of errors, the counter is decremented elsewhere? I didn't
check but I don't think so)
While at it, in case of errors, should we not also remove addr_l from
the local list?
> @@ -419,6 +430,18 @@ int mptcp_nl_cmd_sf_destroy(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, *tmp;
> +
> + spin_lock_bh(&msk->pm.lock);
> + list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> + if (mptcp_addresses_equal(&entry->addr, &addr_l, false)) {
> + list_del_rcu(&entry->list);
> + kfree(entry);
> + msk->pm.local_addr_used--;
Here as well, I don't think you can remove the entry and decrement the
counter if the local address is used by multiple subflows (e.g. fullmesh
mode), no?
The entry might need a refcount.
> + break;
> + }
> + }
> + spin_unlock_bh(&msk->pm.lock);
Do we need to do the same (remove the entry and decrement the counter)
when the subflow is removed from the other side or because of a network
error?
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net