[PATCH mptcp-net 0/3] mptcp: fix signal endpoint readd

[PATCH mptcp-net 0/3] mptcp: fix signal endpoint readd

[Paolo Abeni]

Issues/501 showed that the NL PM currently don't add corectly removal
and re-add of signal endpoint.

Patch 1/3 addresses the issue, patch 2/3 introduce a related self-test,
and the last patch address a pre-existing buglet in the self-test infra.

Paolo Abeni (3):
  mptcp: fix announced address accounting
  selftests: mptcp: add explicit test case for remove/readd
  selftests: mptcp: fix error path

 net/mptcp/pm_netlink.c                        | 27 ++++++++++++-----
 .../testing/selftests/net/mptcp/mptcp_join.sh | 30 ++++++++++++++++++-
 2 files changed, 48 insertions(+), 9 deletions(-)

-- 
2.45.1

[PATCH mptcp-net 1/3] mptcp: fix announced address accounting

[Paolo Abeni]

Currently the per connection announced address counter is never
decreased. As a consequence, after connection establishment, if
the NL PM deletes an endpoint and adds a new/different one, no
additional subflow is created for the new endpoint even if the
current limits allow that.

Address the issue properly updating the signaled address counter
every time one of such address is removed.

Fixes: 01cacb00b35c ("mptcp: add netlink-based PM")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does
things slightly differently from mptcp_pm_remove_addrs_and_subflows(),
with the latter IMHO doing the right thing (TM)
---
 net/mptcp/pm_netlink.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index ea9e5817b9e9..624f9ea66aea 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
 	ret = remove_anno_list_by_saddr(msk, addr);
 	if (ret || force) {
 		spin_lock_bh(&msk->pm.lock);
+		msk->pm.add_addr_signaled--;
 		mptcp_pm_remove_addr(msk, &list);
 		spin_unlock_bh(&msk->pm.lock);
 	}
@@ -1534,16 +1535,25 @@ void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
 {
 	struct mptcp_rm_list alist = { .nr = 0 };
 	struct mptcp_pm_addr_entry *entry;
+	int anno_nr = 0;
 
 	list_for_each_entry(entry, rm_list, list) {
-		if ((remove_anno_list_by_saddr(msk, &entry->addr) ||
-		     lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) &&
-		    alist.nr < MPTCP_RM_IDS_MAX)
-			alist.ids[alist.nr++] = entry->addr.id;
+		if (alist.nr >= MPTCP_RM_IDS_MAX)
+			break;
+
+		/* only delete if either announced or matching a subflow */
+		if (remove_anno_list_by_saddr(msk, &entry->addr))
+			anno_nr++;
+		else if (!lookup_subflow_by_saddr(&msk->conn_list,
+						  &entry->addr))
+			continue;
+
+		alist.ids[alist.nr++] = entry->addr.id;
 	}
 
 	if (alist.nr) {
 		spin_lock_bh(&msk->pm.lock);
+		msk->pm.add_addr_signaled -= anno_nr;
 		mptcp_pm_remove_addr(msk, &alist);
 		spin_unlock_bh(&msk->pm.lock);
 	}
@@ -1556,17 +1566,18 @@ static void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 	struct mptcp_pm_addr_entry *entry;
 
 	list_for_each_entry(entry, rm_list, list) {
-		if (lookup_subflow_by_saddr(&msk->conn_list, &entry->addr) &&
-		    slist.nr < MPTCP_RM_IDS_MAX)
+		if (slist.nr < MPTCP_RM_IDS_MAX &&
+		    lookup_subflow_by_saddr(&msk->conn_list, &entry->addr))
 			slist.ids[slist.nr++] = entry->addr.id;
 
-		if (remove_anno_list_by_saddr(msk, &entry->addr) &&
-		    alist.nr < MPTCP_RM_IDS_MAX)
+		if (alist.nr < MPTCP_RM_IDS_MAX &&
+		    remove_anno_list_by_saddr(msk, &entry->addr))
 			alist.ids[alist.nr++] = entry->addr.id;
 	}
 
 	if (alist.nr) {
 		spin_lock_bh(&msk->pm.lock);
+		msk->pm.add_addr_signaled -= alist.nr;
 		mptcp_pm_remove_addr(msk, &alist);
 		spin_unlock_bh(&msk->pm.lock);
 	}
-- 
2.45.1

[PATCH mptcp-net 2/3] selftests: mptcp: add explicit test case for remove/readd

[Paolo Abeni]

Delete and re-create a signal endpoint and ensure that the PM
actually deletes and re-create the subflow.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 108aeeb84ef1..228fecee61f9 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3526,6 +3526,34 @@ endpoint_tests()
 		chk_mptcp_info subflows 1 subflows 1
 		mptcp_lib_kill_wait $tests_pid
 	fi
+
+	# remove and re-add
+	if reset "delete re-add signal"; then
+		pm_nl_set_limits $ns1 1 1
+		pm_nl_set_limits $ns2 1 1
+		pm_nl_add_endpoint $ns1 10.0.2.1 id 1 flags signal
+		test_linkfail=4 speed=20 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+
+		wait_mpj $ns2
+		pm_nl_check_endpoint "creation" \
+			$ns1 10.0.2.1 id 1 flags signal
+		chk_subflow_nr "before delete" 2
+		chk_mptcp_info subflows 1 subflows 1
+
+		pm_nl_del_endpoint $ns1 1 10.0.2.1
+		sleep 0.5
+		chk_subflow_nr "after delete" 1
+		chk_mptcp_info subflows 0 subflows 0
+
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		wait_mpj $ns2
+		chk_subflow_nr "after re-add" 2
+		chk_mptcp_info subflows 1 subflows 1
+		mptcp_lib_kill_wait $tests_pid
+	fi
+
 }
 
 # [$1: error message]
-- 
2.45.1

[PATCH mptcp-net 3/3] selftests: mptcp: fix error path

[Paolo Abeni]

pm_nl_check_endpoint() currently calls an not existing helper
to mark the test as failed. Fix the wrong call.

Fixes: 03668c65d153 ("selftests: mptcp: join: rework detailed report")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 228fecee61f9..c4593677a567 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -661,7 +661,7 @@ pm_nl_check_endpoint()
 	done
 
 	if [ -z "${id}" ]; then
-		test_fail "bad test - missing endpoint id"
+		fail_test "bad test - missing endpoint id"
 		return
 	fi
 
-- 
2.45.1

Re: [PATCH mptcp-net 0/3] mptcp: fix signal endpoint readd

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/9715703255

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/f42115410887
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=866666


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-net 1/3] mptcp: fix announced address accounting

[Mat Martineau]

On Fri, 28 Jun 2024, Paolo Abeni wrote:

> Currently the per connection announced address counter is never
> decreased. As a consequence, after connection establishment, if
> the NL PM deletes an endpoint and adds a new/different one, no
> additional subflow is created for the new endpoint even if the
> current limits allow that.
>
> Address the issue properly updating the signaled address counter
> every time one of such address is removed.
>
> Fixes: 01cacb00b35c ("mptcp: add netlink-based PM")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does
> things slightly differently from mptcp_pm_remove_addrs_and_subflows(),
> with the latter IMHO doing the right thing (TM)

Hi Paolo -

I don't remember why mptcp_pm_remove_addrs() has the extra code to send 
the RM_ADDR for addresses that are in the conn_list but not the anno_list. 
Maybe to allow a way to sent RM_ADDR as a "request for the peer to 
initiate a disconnect" even on implicitly added address IDs (from outgoing 
subflow connections instead of ADD_ADDRs)?

> ---
> net/mptcp/pm_netlink.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index ea9e5817b9e9..624f9ea66aea 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
> 	ret = remove_anno_list_by_saddr(msk, addr);
> 	if (ret || force) {
> 		spin_lock_bh(&msk->pm.lock);
> +		msk->pm.add_addr_signaled--;

Looks like this only be decremented if ret is true, since that indicates a 
list element was removed and add_addr_signaled is supposed (?) to track 
the length of the anno_list.


- Mat

> 		mptcp_pm_remove_addr(msk, &list);
> 		spin_unlock_bh(&msk->pm.lock);
> 	}
> @@ -1534,16 +1535,25 @@ void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
> {
> 	struct mptcp_rm_list alist = { .nr = 0 };
> 	struct mptcp_pm_addr_entry *entry;
> +	int anno_nr = 0;
>
> 	list_for_each_entry(entry, rm_list, list) {
> -		if ((remove_anno_list_by_saddr(msk, &entry->addr) ||
> -		     lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) &&
> -		    alist.nr < MPTCP_RM_IDS_MAX)
> -			alist.ids[alist.nr++] = entry->addr.id;
> +		if (alist.nr >= MPTCP_RM_IDS_MAX)
> +			break;
> +
> +		/* only delete if either announced or matching a subflow */
> +		if (remove_anno_list_by_saddr(msk, &entry->addr))
> +			anno_nr++;
> +		else if (!lookup_subflow_by_saddr(&msk->conn_list,
> +						  &entry->addr))
> +			continue;
> +
> +		alist.ids[alist.nr++] = entry->addr.id;
> 	}
>
> 	if (alist.nr) {
> 		spin_lock_bh(&msk->pm.lock);
> +		msk->pm.add_addr_signaled -= anno_nr;
> 		mptcp_pm_remove_addr(msk, &alist);
> 		spin_unlock_bh(&msk->pm.lock);
> 	}
> @@ -1556,17 +1566,18 @@ static void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
> 	struct mptcp_pm_addr_entry *entry;
>
> 	list_for_each_entry(entry, rm_list, list) {
> -		if (lookup_subflow_by_saddr(&msk->conn_list, &entry->addr) &&
> -		    slist.nr < MPTCP_RM_IDS_MAX)
> +		if (slist.nr < MPTCP_RM_IDS_MAX &&
> +		    lookup_subflow_by_saddr(&msk->conn_list, &entry->addr))
> 			slist.ids[slist.nr++] = entry->addr.id;
>
> -		if (remove_anno_list_by_saddr(msk, &entry->addr) &&
> -		    alist.nr < MPTCP_RM_IDS_MAX)
> +		if (alist.nr < MPTCP_RM_IDS_MAX &&
> +		    remove_anno_list_by_saddr(msk, &entry->addr))
> 			alist.ids[alist.nr++] = entry->addr.id;
> 	}
>
> 	if (alist.nr) {
> 		spin_lock_bh(&msk->pm.lock);
> +		msk->pm.add_addr_signaled -= alist.nr;
> 		mptcp_pm_remove_addr(msk, &alist);
> 		spin_unlock_bh(&msk->pm.lock);
> 	}
> -- 
> 2.45.1
>
>
>

Re: [PATCH mptcp-net 1/3] mptcp: fix announced address accounting

[Matthieu Baerts]

Hi Paolo, Mat,

On 01/07/2024 20:08, Mat Martineau wrote:
> On Fri, 28 Jun 2024, Paolo Abeni wrote:
> 
>> Currently the per connection announced address counter is never
>> decreased. As a consequence, after connection establishment, if
>> the NL PM deletes an endpoint and adds a new/different one, no
>> additional subflow is created for the new endpoint even if the
>> current limits allow that.
>>
>> Address the issue properly updating the signaled address counter
>> every time one of such address is removed.

Thank you for having looked at that!

>> Fixes: 01cacb00b35c ("mptcp: add netlink-based PM")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does
>> things slightly differently from mptcp_pm_remove_addrs_and_subflows(),
>> with the latter IMHO doing the right thing (TM)
> 
> Hi Paolo -
> 
> I don't remember why mptcp_pm_remove_addrs() has the extra code to send
> the RM_ADDR for addresses that are in the conn_list but not the
> anno_list. Maybe to allow a way to sent RM_ADDR as a "request for the
> peer to initiate a disconnect" even on implicitly added address IDs
> (from outgoing subflow connections instead of ADD_ADDRs)?

mptcp_pm_remove_addrs() is only used by the userspace PM, it should
probably be moved to pm_userspace.c in fact (but it uses helpers from
pm_netlink.c, I guess that's why it is there). My understanding is that
it also checks the local subflows list because the userspace can ask to
send a remove ADDR for addresses that have been "implicitly" announced
via the MPJoin as well.

Maybe the modification in mptcp_pm_remove_addrs() should be in a
different commit? With a different Fixes tag then:

  Fixes: 8b1c94da1e48 ("mptcp: only send RM_ADDR in nl_cmd_remove")

> 
>> ---
>> net/mptcp/pm_netlink.c | 27 +++++++++++++++++++--------
>> 1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index ea9e5817b9e9..624f9ea66aea 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct
>> mptcp_sock *msk,
>>     ret = remove_anno_list_by_saddr(msk, addr);
>>     if (ret || force) {
>>         spin_lock_bh(&msk->pm.lock);
>> +        msk->pm.add_addr_signaled--;
> 
> Looks like this only be decremented if ret is true, since that indicates
> a list element was removed and add_addr_signaled is supposed (?) to
> track the length of the anno_list.

Good point.

Should we not also set the corresponding bit in:

  msk->pm.id_avail_signals_bitmap

Linked to my series:


https://lore.kernel.org/mptcp/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org/T/

Otherwise, we will not be able to re-use the same ID, no?

And if we change add_addr_signaled counter here after having removed an
address, maybe now we are no longer at the signalled limit, should we
then call mptcp_pm_create_subflow_or_signal_addr()?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net 2/3] selftests: mptcp: add explicit test case for remove/readd

[Matthieu Baerts]

Hi Paolo,

On 28/06/2024 17:54, Paolo Abeni wrote:
> Delete and re-create a signal endpoint and ensure that the PM
> actually deletes and re-create the subflow.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 108aeeb84ef1..228fecee61f9 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3526,6 +3526,34 @@ endpoint_tests()
>  		chk_mptcp_info subflows 1 subflows 1
>  		mptcp_lib_kill_wait $tests_pid
>  	fi
> +
> +	# remove and re-add
> +	if reset "delete re-add signal"; then

In the previous tests -- "delete and re-add", we added:

  mptcp_lib_kallsyms_has "subflow_rebuild_header$"

At the beginning of the endpoint_tests() function there is this comment:

  # subflow_rebuild_header is needed to support the implicit flag

Maybe I put the same check on "delete and re-add" by mistake. I don't
think this test depends on the "implicit" type. Or maybe it is, because
its introduction is also linked to some big modifications on the PM
side. In this case, should we have the same check?

  && mptcp_lib_kallsyms_has "subflow_rebuild_header$"

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net 3/3] selftests: mptcp: fix error path

[Matthieu Baerts]

On 28/06/2024 17:54, Paolo Abeni wrote:
> pm_nl_check_endpoint() currently calls an not existing helper
> to mark the test as failed. Fix the wrong call.
> 
> Fixes: 03668c65d153 ("selftests: mptcp: join: rework detailed report")

Good catch!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net 1/3] mptcp: fix announced address accounting

[Matthieu Baerts]

Hi Paolo,

On 02/07/2024 10:51, Matthieu Baerts wrote:
> Hi Paolo, Mat,
> 
> On 01/07/2024 20:08, Mat Martineau wrote:
>> On Fri, 28 Jun 2024, Paolo Abeni wrote:
>>
>>> Currently the per connection announced address counter is never
>>> decreased. As a consequence, after connection establishment, if
>>> the NL PM deletes an endpoint and adds a new/different one, no
>>> additional subflow is created for the new endpoint even if the
>>> current limits allow that.
>>>
>>> Address the issue properly updating the signaled address counter
>>> every time one of such address is removed.
> 
> Thank you for having looked at that!
> 
>>> Fixes: 01cacb00b35c ("mptcp: add netlink-based PM")
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does
>>> things slightly differently from mptcp_pm_remove_addrs_and_subflows(),
>>> with the latter IMHO doing the right thing (TM)
>>
>> Hi Paolo -
>>
>> I don't remember why mptcp_pm_remove_addrs() has the extra code to send
>> the RM_ADDR for addresses that are in the conn_list but not the
>> anno_list. Maybe to allow a way to sent RM_ADDR as a "request for the
>> peer to initiate a disconnect" even on implicitly added address IDs
>> (from outgoing subflow connections instead of ADD_ADDRs)?
> 
> mptcp_pm_remove_addrs() is only used by the userspace PM, it should
> probably be moved to pm_userspace.c in fact (but it uses helpers from
> pm_netlink.c, I guess that's why it is there). My understanding is that
> it also checks the local subflows list because the userspace can ask to
> send a remove ADDR for addresses that have been "implicitly" announced
> via the MPJoin as well.
> 
> Maybe the modification in mptcp_pm_remove_addrs() should be in a
> different commit? With a different Fixes tag then:
> 
>   Fixes: 8b1c94da1e48 ("mptcp: only send RM_ADDR in nl_cmd_remove")
> 
>>
>>> ---
>>> net/mptcp/pm_netlink.c | 27 +++++++++++++++++++--------
>>> 1 file changed, 19 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index ea9e5817b9e9..624f9ea66aea 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct
>>> mptcp_sock *msk,
>>>     ret = remove_anno_list_by_saddr(msk, addr);
>>>     if (ret || force) {
>>>         spin_lock_bh(&msk->pm.lock);
>>> +        msk->pm.add_addr_signaled--;
>>
>> Looks like this only be decremented if ret is true, since that indicates
>> a list element was removed and add_addr_signaled is supposed (?) to
>> track the length of the anno_list.
> 
> Good point.
> 
> Should we not also set the corresponding bit in:
> 
>   msk->pm.id_avail_signals_bitmap
> 
> Linked to my series:
> 
> 
> https://lore.kernel.org/mptcp/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org/T/
> 
> Otherwise, we will not be able to re-use the same ID, no?

It is a bit confusing, but it looks like the bitmap is modified in some
cases:

- mptcp_pm_remove_anno_addr() is only called from
mptcp_nl_remove_subflow_and_signal_addr(), but for 2 different cases:

  - If there are no additional subflows (list_empty(&msk->conn_list)),
    force == false → the bitmap is not modified

  - If there are additional subflows, and a matching one, force could be
    set to true (if not implicit), but also mptcp_pm_remove_subflow()
    will be called. In this case, the bitmap is modified.

I wonder if it would not be clearer to set the available "signals" bit
in mptcp_pm_remove_addr(), and set the available "subflows" bit in
mptcp_pm_nl_rm_addr_or_subflow(MPTCP_MIB_RMSUBFLOW) -- what the v1 of my
patch is doing [1] -- so one is dedicated to remove endpoints with  the
'signal' flag, and the other one for endpoints with the 'subflow' flag.
WDYT?

[1]
https://lore.kernel.org/r/20240621-mptcp-pm-avail-v1-2-b692d5eb89b5@kernel.org

> And if we change add_addr_signaled counter here after having removed an
> address, maybe now we are no longer at the signalled limit, should we
> then call mptcp_pm_create_subflow_or_signal_addr()?
> 
> Cheers,
> Matt

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net 1/3] mptcp: fix announced address accounting

[Paolo Abeni]

On Tue, 2024-07-02 at 10:51 +0200, Matthieu Baerts wrote:
> Hi Paolo, Mat,
> 
> On 01/07/2024 20:08, Mat Martineau wrote:
> > On Fri, 28 Jun 2024, Paolo Abeni wrote:
> > 
> > > Currently the per connection announced address counter is never
> > > decreased. As a consequence, after connection establishment, if
> > > the NL PM deletes an endpoint and adds a new/different one, no
> > > additional subflow is created for the new endpoint even if the
> > > current limits allow that.
> > > 
> > > Address the issue properly updating the signaled address counter
> > > every time one of such address is removed.
> 
> Thank you for having looked at that!
> 
> > > Fixes: 01cacb00b35c ("mptcp: add netlink-based PM")
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > > slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does
> > > things slightly differently from mptcp_pm_remove_addrs_and_subflows(),
> > > with the latter IMHO doing the right thing (TM)
> > 
> > Hi Paolo -
> > 
> > I don't remember why mptcp_pm_remove_addrs() has the extra code to send
> > the RM_ADDR for addresses that are in the conn_list but not the
> > anno_list. Maybe to allow a way to sent RM_ADDR as a "request for the
> > peer to initiate a disconnect" even on implicitly added address IDs
> > (from outgoing subflow connections instead of ADD_ADDRs)?
> 
> mptcp_pm_remove_addrs() is only used by the userspace PM, it should
> probably be moved to pm_userspace.c in fact (but it uses helpers from
> pm_netlink.c, I guess that's why it is there). My understanding is that
> it also checks the local subflows list because the userspace can ask to
> send a remove ADDR for addresses that have been "implicitly" announced
> via the MPJoin as well.
> 
> Maybe the modification in mptcp_pm_remove_addrs() should be in a
> different commit? With a different Fixes tag then:
> 
>   Fixes: 8b1c94da1e48 ("mptcp: only send RM_ADDR in nl_cmd_remove")
> 
> > 
> > > ---
> > > net/mptcp/pm_netlink.c | 27 +++++++++++++++++++--------
> > > 1 file changed, 19 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index ea9e5817b9e9..624f9ea66aea 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct
> > > mptcp_sock *msk,
> > >     ret = remove_anno_list_by_saddr(msk, addr);
> > >     if (ret || force) {
> > >         spin_lock_bh(&msk->pm.lock);
> > > +        msk->pm.add_addr_signaled--;
> > 
> > Looks like this only be decremented if ret is true, since that indicates
> > a list element was removed and add_addr_signaled is supposed (?) to
> > track the length of the anno_list.
> 
> Good point.
> 
> Should we not also set the corresponding bit in:
> 
>   msk->pm.id_avail_signals_bitmap
> 
> Linked to my series:
> 
> 
> https://lore.kernel.org/mptcp/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org/T/
> 
> Otherwise, we will not be able to re-use the same ID, no?

This is for net, your series is rightfully for net-next, I guess you
should do the id_avail_signals_bitmap accounting only on the latter.

Thanks,

Paolo

Re: [PATCH mptcp-net 2/3] selftests: mptcp: add explicit test case for remove/readd

[Paolo Abeni]

On Tue, 2024-07-02 at 11:01 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 28/06/2024 17:54, Paolo Abeni wrote:
> > Delete and re-create a signal endpoint and ensure that the PM
> > actually deletes and re-create the subflow.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 28 +++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 108aeeb84ef1..228fecee61f9 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -3526,6 +3526,34 @@ endpoint_tests()
> >  		chk_mptcp_info subflows 1 subflows 1
> >  		mptcp_lib_kill_wait $tests_pid
> >  	fi
> > +
> > +	# remove and re-add
> > +	if reset "delete re-add signal"; then
> 
> In the previous tests -- "delete and re-add", we added:
> 
>   mptcp_lib_kallsyms_has "subflow_rebuild_header$"
> 
> At the beginning of the endpoint_tests() function there is this comment:
> 
>   # subflow_rebuild_header is needed to support the implicit flag
> 
> Maybe I put the same check on "delete and re-add" by mistake. I don't
> think this test depends on the "implicit" type. Or maybe it is, because
> its introduction is also linked to some big modifications on the PM
> side. In this case, should we have the same check?
> 
>   && mptcp_lib_kallsyms_has "subflow_rebuild_header$"

Uhmm... sounds a little too much defensive programming, but I guess I
can add the test here

Thanks!

Paolo