[v2] mptcp: pm: fix ADD_ADDR timer infinite retry on option space shortage

[PATCH mptcp-net v2 2/2] selftests: mptcp: join: cover ADD_ADDR tx drop and list progress

Posted by Li Xiasong 1 month, 1 week ago

Extend add_addr_ports_tests with IPv6 signaling cases that exercise
ADD_ADDR tx-space shortage when tcp_timestamps are enabled.

Add one case to verify ADD_ADDR tx drop accounting via
MPTcpExtAddAddrTxDrop, and another case to verify PM still progresses
to later signal endpoints after the first one is dropped.

This covers both failure accounting and the non-blocking behavior of
the announce list after a tx-space drop on pure ACK.

Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index beec41f6662a..59beddb487e3 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1828,6 +1828,22 @@ chk_add_tx_nr()
 	fi
 }
 
+chk_add_drop_tx_nr()
+{
+	local add_drop_tx_nr=$1
+	local count
+
+	print_check "add addr tx drop"
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtAddAddrTxDrop")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$add_drop_tx_nr" ]; then
+		fail_test "got $count ADD_ADDR drop[s] TX, expected $add_drop_tx_nr"
+	else
+		print_ok
+	fi
+}
+
 chk_rm_nr()
 {
 	local rm_addr_nr=$1
@@ -3278,6 +3294,37 @@ add_addr_ports_tests()
 
 		chk_mpc_endp_attempt ${retl} 1
 	fi
+
+	# signal address with port IPv6: tx fails with tcp timestamp
+	if reset "signal address with port IPv6 tx drop"; then
+		pm_nl_set_limits $ns1 0 1
+		pm_nl_set_limits $ns2 1 0
+		ip netns exec $ns1 sysctl -q net.ipv4.tcp_timestamps=1
+		ip netns exec $ns2 sysctl -q net.ipv4.tcp_timestamps=1
+		pm_nl_add_endpoint $ns1 dead:beef:2::1 flags signal port 10100
+		speed=slow \
+			run_tests $ns1 $ns2 dead:beef:1::1
+		chk_add_nr 0 0 0
+		chk_add_drop_tx_nr 1
+	fi
+
+	# first signal address drops, second one still progresses
+	if reset "signal addr list progresses after tx drop"; then
+		pm_nl_set_limits $ns1 0 2
+		pm_nl_set_limits $ns2 1 0
+		ip netns exec $ns1 sysctl -q net.ipv4.tcp_timestamps=1
+		ip netns exec $ns2 sysctl -q net.ipv4.tcp_timestamps=1
+
+		pm_nl_add_endpoint $ns1 dead:beef:2::1 flags signal port 10100
+		pm_nl_add_endpoint $ns1 dead:beef:3::1 flags signal
+
+		speed=slow \
+			run_tests $ns1 $ns2 dead:beef:1::1
+
+		chk_add_drop_tx_nr 1
+		chk_add_tx_nr 1 1
+		chk_add_nr 1 1 0
+	fi
 }
 
 bind_tests()
-- 
2.34.1

Re: [PATCH mptcp-net v2 2/2] selftests: mptcp: join: cover ADD_ADDR tx drop and list progress

Posted by Matthieu Baerts 1 month, 1 week ago

On Thu, 30 Apr 2026 19:20:26 +0800, Li Xiasong <lixiasong1@huawei.com> wrote:
> Extend add_addr_ports_tests with IPv6 signaling cases that exercise
> ADD_ADDR tx-space shortage when tcp_timestamps are enabled.
> 
> Add one case to verify ADD_ADDR tx drop accounting via
> MPTcpExtAddAddrTxDrop, and another case to verify PM still progresses
> to later signal endpoints after the first one is dropped.
> 
> This covers both failure accounting and the non-blocking behavior of
> the announce list after a tx-space drop on pure ACK.

Thank you for this new test!

>
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index beec41f6662a..59beddb487e3 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1828,6 +1828,22 @@ chk_add_tx_nr()
>  	fi
>  }
>  
> +chk_add_drop_tx_nr()
> +{
> +	local add_drop_tx_nr=$1
> +	local count
> +
> +	print_check "add addr tx drop"
> +	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtAddAddrTxDrop")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$add_drop_tx_nr" ]; then
> +		fail_test "got $count ADD_ADDR drop[s] TX, expected $add_drop_tx_nr"

Nit, just to avoid a checkpatch warning, maybe rename add_drop_tx_nr?
'expected'?

  -> WARNING: line length of 84 exceeds 80 columns

> @@ -3278,6 +3294,37 @@ add_addr_ports_tests()
> [ ... skip 22 lines ... ]
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_timestamps=1
> +
> +		pm_nl_add_endpoint $ns1 dead:beef:2::1 flags signal port 10100
> +		pm_nl_add_endpoint $ns1 dead:beef:3::1 flags signal
> +
> +		speed=slow \

Do you need 'speed=slow' here? Having a longer transfer is typically
needed when endpoints are manipulated during the transfer, or when a few
there are >= 3 signal endpoints. I don't think that's needed here. Or
did you experimented any issues? (if yes, makes sure you are on top of
our export/export-net or for-review/for-review-net branches)

> +			run_tests $ns1 $ns2 dead:beef:1::1
> +
> +		chk_add_drop_tx_nr 1
> +		chk_add_tx_nr 1 1
> +		chk_add_nr 1 1 0
> +	fi

It feels like the second test covers the case tested by the previous
one, no? I would only keep the second one, best to avoid redundant
tests, this selftest is already slow enough :)

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

Re: [PATCH mptcp-net v2 2/2] selftests: mptcp: join: cover ADD_ADDR tx drop and list progress

Posted by Li Xiasong 1 month ago

Hi, Matt

Many thanks for the review and for pointing these out.

On 4/30/26 9:20 PM, Matthieu Baerts wrote:
> On Thu, 30 Apr 2026 19:20:26 +0800, Li Xiasong <lixiasong1@huawei.com> wrote:
>> Extend add_addr_ports_tests with IPv6 signaling cases that exercise
>> ADD_ADDR tx-space shortage when tcp_timestamps are enabled.
>>
>> Add one case to verify ADD_ADDR tx drop accounting via
>> MPTcpExtAddAddrTxDrop, and another case to verify PM still progresses
>> to later signal endpoints after the first one is dropped.
>>
>> This covers both failure accounting and the non-blocking behavior of
>> the announce list after a tx-space drop on pure ACK.
> 
> Thank you for this new test!
> 
>>
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index beec41f6662a..59beddb487e3 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1828,6 +1828,22 @@ chk_add_tx_nr()
>>  	fi
>>  }
>>  
>> +chk_add_drop_tx_nr()
>> +{
>> +	local add_drop_tx_nr=$1
>> +	local count
>> +
>> +	print_check "add addr tx drop"
>> +	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtAddAddrTxDrop")
>> +	if [ -z "$count" ]; then
>> +		print_skip
>> +	elif [ "$count" != "$add_drop_tx_nr" ]; then
>> +		fail_test "got $count ADD_ADDR drop[s] TX, expected $add_drop_tx_nr"
> 
> Nit, just to avoid a checkpatch warning, maybe rename add_drop_tx_nr?
> 'expected'?
> 
>   -> WARNING: line length of 84 exceeds 80 columns
> 

Thanks, I’ll rename the variable to `expected` and shorten the fail
message to avoid the 80-column warning.

>> @@ -3278,6 +3294,37 @@ add_addr_ports_tests()
>> [ ... skip 22 lines ... ]
>> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_timestamps=1
>> +
>> +		pm_nl_add_endpoint $ns1 dead:beef:2::1 flags signal port 10100
>> +		pm_nl_add_endpoint $ns1 dead:beef:3::1 flags signal
>> +
>> +		speed=slow \
> 
> Do you need 'speed=slow' here? Having a longer transfer is typically
> needed when endpoints are manipulated during the transfer, or when a few
> there are >= 3 signal endpoints. I don't think that's needed here. Or
> did you experimented any issues? (if yes, makes sure you are on top of
> our export/export-net or for-review/for-review-net branches)
> 

I’ll remove `speed=slow` unless I can show it is actually needed.

>> +			run_tests $ns1 $ns2 dead:beef:1::1
>> +
>> +		chk_add_drop_tx_nr 1
>> +		chk_add_tx_nr 1 1
>> +		chk_add_nr 1 1 0
>> +	fi
> 
> It feels like the second test covers the case tested by the previous
> one, no? I would only keep the second one, best to avoid redundant
> tests, this selftest is already slow enough :)
> 

I originally split this into two cases to validate accounting and
forward progress separately, but I agree the second one already covers
both, so I'll keep only that one in v3.

Thanks,
Li Xiasong