[PATCH mptcp-net v2 0/9] mptcp: fix inconsistent backup usage

[PATCH mptcp-net v2 0/9] mptcp: fix inconsistent backup usage

[Matthieu Baerts (NGI0)]

In all the backup tests we have, the backup flag is set on one side, and
the expected behaviour is to have both sides respecting this decision.

On the scheduler side, only the 'backup' field is checked, which is
supposed to be set only if the other peer flagged a subflow as backup.
But in various places, this flag is set when the local host flagged the
subflow as backup.

One last thing that has been fixed is the backup flag in 'signal'
endpoints: it was working only when setting the backup flag while the
connection was already opened. That's what the MPJ selftests were doing
to check that the signal endpoint were supporting the backup flag (on
purpose I guess, because the support was broken in MP_JOIN...).

The last two patches adapt the BPF selftests to mimic what is now done
by the in-kernel packets scheduler.

Note that the Packetdrill tests need to be updated because the behaviour
has been changed / fixed:

  https://github.com/multipath-tcp/packetdrill/pull/152

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v2:
- new patches 4 and 5.
- split patch 6 in two (code then tests) to ease the backports.
- patch 6 addresses Mat's comments, and support the ID0 case.
- Link to v1: https://lore.kernel.org/r/20240711-mptcp-backup-mpj-v1-0-d45506182a9e@kernel.org

---
Matthieu Baerts (NGI0) (9):
      mptcp: sched: check both directions for backup
      mptcp: distinguish rcv vs sent backup flag in requests
      mptcp: pm: only set request_bkup flag when sending MP_PRIO
      mptcp: mib: count MPJ with backup flag
      selftests: mptcp: join: validate backup in MPJ
      mptcp: pm: fix backup support in signal endpoints
      selftests: mptcp: join: check backup support in signal endp
      Squash to "selftests/bpf: Add bpf_bkup scheduler & test"
      Squash to "selftests/bpf: Add bpf_burst scheduler & test"

 include/trace/events/mptcp.h                       |  2 +-
 net/mptcp/mib.c                                    |  2 +
 net/mptcp/mib.h                                    |  2 +
 net/mptcp/options.c                                |  2 +-
 net/mptcp/pm.c                                     | 15 +++--
 net/mptcp/pm_netlink.c                             | 24 +++++++-
 net/mptcp/pm_userspace.c                           | 25 +++++++-
 net/mptcp/protocol.c                               | 10 +--
 net/mptcp/protocol.h                               | 12 +++-
 net/mptcp/subflow.c                                | 16 ++++-
 tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c |  3 +-
 .../testing/selftests/bpf/progs/mptcp_bpf_burst.c  | 12 ++--
 tools/testing/selftests/net/mptcp/mptcp_join.sh    | 72 +++++++++++++++++-----
 13 files changed, 159 insertions(+), 38 deletions(-)
---
base-commit: 06bd6d6c3416101af4881b6f3a0830ccca97c056
change-id: 20240711-mptcp-backup-mpj-2f8369fad366

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

[PATCH mptcp-net v2 1/9] mptcp: sched: check both directions for backup

[Matthieu Baerts (NGI0)]

The 'mptcp_subflow_context' structure has two items related to the
backup flags:

 - 'backup': the subflow has been marked as backup by the other peer

- 'request_bkup': the backup flag has been set by the host

Before this patch, the scheduler was only looking at the 'backup' flag.
That can make sense in some cases, but it looks like that's not what we
wanted for the general use, because either the path-manager was setting
both of them when sending an MP_PRIO, or the receiver was duplicating
the 'backup' flag in the subflow request.

Note that the use of these two flags in the path-manager are going to be
fixed in the next commits, but this change here is needed not to modify
the behaviour.

Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 include/trace/events/mptcp.h |  2 +-
 net/mptcp/protocol.c         | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/mptcp.h b/include/trace/events/mptcp.h
index 09e72215b9f9..085b749cdd97 100644
--- a/include/trace/events/mptcp.h
+++ b/include/trace/events/mptcp.h
@@ -34,7 +34,7 @@ TRACE_EVENT(mptcp_subflow_get_send,
 		struct sock *ssk;
 
 		__entry->active = mptcp_subflow_active(subflow);
-		__entry->backup = subflow->backup;
+		__entry->backup = subflow->backup || subflow->request_bkup;
 
 		if (subflow->tcp_sock && sk_fullsock(subflow->tcp_sock))
 			__entry->free = sk_stream_memory_free(subflow->tcp_sock);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ac94225489f8..b3a48d97f009 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1422,13 +1422,15 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 	}
 
 	mptcp_for_each_subflow(msk, subflow) {
+		bool backup = subflow->backup || subflow->request_bkup;
+
 		trace_mptcp_subflow_get_send(subflow);
 		ssk =  mptcp_subflow_tcp_sock(subflow);
 		if (!mptcp_subflow_active(subflow))
 			continue;
 
 		tout = max(tout, mptcp_timeout_from_subflow(subflow));
-		nr_active += !subflow->backup;
+		nr_active += !backup;
 		pace = subflow->avg_pacing_rate;
 		if (unlikely(!pace)) {
 			/* init pacing rate from socket */
@@ -1439,9 +1441,9 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 		}
 
 		linger_time = div_u64((u64)READ_ONCE(ssk->sk_wmem_queued) << 32, pace);
-		if (linger_time < send_info[subflow->backup].linger_time) {
-			send_info[subflow->backup].ssk = ssk;
-			send_info[subflow->backup].linger_time = linger_time;
+		if (linger_time < send_info[backup].linger_time) {
+			send_info[backup].ssk = ssk;
+			send_info[backup].linger_time = linger_time;
 		}
 	}
 	__mptcp_set_timeout(sk, tout);

-- 
2.45.2

Re: [PATCH mptcp-net v2 1/9] mptcp: sched: check both directions for backup

[Geliang Tang]

On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
> The 'mptcp_subflow_context' structure has two items related to the
> backup flags:
> 
>  - 'backup': the subflow has been marked as backup by the other peer
> 
> - 'request_bkup': the backup flag has been set by the host

The two lines are not aligned.

> 
> Before this patch, the scheduler was only looking at the 'backup'
> flag.
> That can make sense in some cases, but it looks like that's not what
> we
> wanted for the general use, because either the path-manager was
> setting
> both of them when sending an MP_PRIO, or the receiver was duplicating
> the 'backup' flag in the subflow request.
> 
> Note that the use of these two flags in the path-manager are going to
> be
> fixed in the next commits, but this change here is needed not to
> modify
> the behaviour.
> 
> Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN
> requests")

Patch 3 can be squashed into this one, with two "Fixes" tags here.

WDYT?

Thanks,
-Geliang

> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  include/trace/events/mptcp.h |  2 +-
>  net/mptcp/protocol.c         | 10 ++++++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/trace/events/mptcp.h
> b/include/trace/events/mptcp.h
> index 09e72215b9f9..085b749cdd97 100644
> --- a/include/trace/events/mptcp.h
> +++ b/include/trace/events/mptcp.h
> @@ -34,7 +34,7 @@ TRACE_EVENT(mptcp_subflow_get_send,
>  		struct sock *ssk;
>  
>  		__entry->active = mptcp_subflow_active(subflow);
> -		__entry->backup = subflow->backup;
> +		__entry->backup = subflow->backup || subflow-
> >request_bkup;
>  
>  		if (subflow->tcp_sock && sk_fullsock(subflow-
> >tcp_sock))
>  			__entry->free =
> sk_stream_memory_free(subflow->tcp_sock);
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ac94225489f8..b3a48d97f009 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1422,13 +1422,15 @@ struct sock *mptcp_subflow_get_send(struct
> mptcp_sock *msk)
>  	}
>  
>  	mptcp_for_each_subflow(msk, subflow) {
> +		bool backup = subflow->backup || subflow-
> >request_bkup;
> +
>  		trace_mptcp_subflow_get_send(subflow);
>  		ssk =  mptcp_subflow_tcp_sock(subflow);
>  		if (!mptcp_subflow_active(subflow))
>  			continue;
>  
>  		tout = max(tout,
> mptcp_timeout_from_subflow(subflow));
> -		nr_active += !subflow->backup;
> +		nr_active += !backup;
>  		pace = subflow->avg_pacing_rate;
>  		if (unlikely(!pace)) {
>  			/* init pacing rate from socket */
> @@ -1439,9 +1441,9 @@ struct sock *mptcp_subflow_get_send(struct
> mptcp_sock *msk)
>  		}
>  
>  		linger_time = div_u64((u64)READ_ONCE(ssk-
> >sk_wmem_queued) << 32, pace);
> -		if (linger_time < send_info[subflow-
> >backup].linger_time) {
> -			send_info[subflow->backup].ssk = ssk;
> -			send_info[subflow->backup].linger_time =
> linger_time;
> +		if (linger_time < send_info[backup].linger_time) {
> +			send_info[backup].ssk = ssk;
> +			send_info[backup].linger_time = linger_time;
>  		}
>  	}
>  	__mptcp_set_timeout(sk, tout);
> 

Re: [PATCH mptcp-net v2 1/9] mptcp: sched: check both directions for backup

[Matthieu Baerts]

Hi Geliang,

Thank you for the review!

On 17/07/2024 06:25, Geliang Tang wrote:
> On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
>> The 'mptcp_subflow_context' structure has two items related to the
>> backup flags:
>>
>>  - 'backup': the subflow has been marked as backup by the other peer
>>
>> - 'request_bkup': the backup flag has been set by the host
> 
> The two lines are not aligned.

Good catch!

>> Before this patch, the scheduler was only looking at the 'backup'
>> flag.
>> That can make sense in some cases, but it looks like that's not what
>> we
>> wanted for the general use, because either the path-manager was
>> setting
>> both of them when sending an MP_PRIO, or the receiver was duplicating
>> the 'backup' flag in the subflow request.
>>
>> Note that the use of these two flags in the path-manager are going to
>> be
>> fixed in the next commits, but this change here is needed not to
>> modify
>> the behaviour.
>>
>> Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN
>> requests")
> 
> Patch 3 can be squashed into this one, with two "Fixes" tags here.

I think we should avoid putting two Fixes tags, because it makes the
backports harder. Also, the issue is really there since MP_JOIN got
supported: at the beginning, the extra subflows were flagged as backup,
and since the beginning, the backup flag in the SYN+MPJ is reflected in
the SYN+ACK+MPJ (patch 2). The scheduler was then always looking at both
side by accident since the beginning. By fixing use of the two flags
internally, we should also modify the scheduler to keep the same
behaviour (looking at both sides is what we should have done from the
beginning to me). No?

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

[PATCH mptcp-net v2 2/9] mptcp: distinguish rcv vs sent backup flag in requests

[Matthieu Baerts (NGI0)]

When sending an MP_JOIN + SYN + ACK, it is possible to mark the subflow
as 'backup' by setting the flag with the same name. Before this patch,
the backup was set if the other peer set it in its MP_JOIN + SYN
request.

It is not correct: the backup flag should be set in the MPJ+SYN+ACK only
if the host asks for it, and not mirroring what was done by the other
peer. It is then required to have a dedicated bit for each direction,
similar to what is done in the subflow context.

Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c  | 2 +-
 net/mptcp/protocol.h | 1 +
 net/mptcp/subflow.c  | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c0832df3b0a3..2f8e357f58a3 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -909,7 +909,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
 		return true;
 	} else if (subflow_req->mp_join) {
 		opts->suboptions = OPTION_MPTCP_MPJ_SYNACK;
-		opts->backup = subflow_req->backup;
+		opts->backup = subflow_req->request_bkup;
 		opts->join_id = subflow_req->local_id;
 		opts->thmac = subflow_req->thmac;
 		opts->nonce = subflow_req->local_nonce;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 19d60b6d5b45..6b6b76152db5 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -448,6 +448,7 @@ struct mptcp_subflow_request_sock {
 	u16	mp_capable : 1,
 		mp_join : 1,
 		backup : 1,
+		request_bkup : 1,
 		csum_reqd : 1,
 		allow_join_id0 : 1;
 	u8	local_id;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 39e2cbdf3801..a3778aee4e77 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -2005,6 +2005,7 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		new_ctx->fully_established = 1;
 		new_ctx->remote_key_valid = 1;
 		new_ctx->backup = subflow_req->backup;
+		new_ctx->request_bkup = subflow_req->request_bkup;
 		WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);
 		new_ctx->token = subflow_req->token;
 		new_ctx->thmac = subflow_req->thmac;

-- 
2.45.2

[PATCH mptcp-net v2 3/9] mptcp: pm: only set request_bkup flag when sending MP_PRIO

[Matthieu Baerts (NGI0)]

The 'backup' flag from mptcp_subflow_context structure is supposed to be
set only when the other peer flagged a subflow as backup, not the
opposite.

Fixes: 067065422fcd ("mptcp: add the outgoing MP_PRIO support")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index f65831de5c1a..7635fac91539 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -471,7 +471,6 @@ static void __mptcp_pm_send_ack(struct mptcp_sock *msk, struct mptcp_subflow_con
 	slow = lock_sock_fast(ssk);
 	if (prio) {
 		subflow->send_mp_prio = 1;
-		subflow->backup = backup;
 		subflow->request_bkup = backup;
 	}
 

-- 
2.45.2

[PATCH mptcp-net v2 4/9] mptcp: mib: count MPJ with backup flag

[Matthieu Baerts (NGI0)]

Without such counters, it is difficult to easily debug issues with MPJ
not having the backup flags on production servers.

This is not strictly a fix, but it eases to validate the following patch
without requiring to take packet traces, to query ongoing connections
with Netlink with admin permissions, or to guess by looking at the
behaviour of the scheduler.

Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/mib.c     | 2 ++
 net/mptcp/mib.h     | 2 ++
 net/mptcp/subflow.c | 6 ++++++
 3 files changed, 10 insertions(+)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index c30405e76833..7884217f33eb 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -19,7 +19,9 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("MPTCPRetrans", MPTCP_MIB_RETRANSSEGS),
 	SNMP_MIB_ITEM("MPJoinNoTokenFound", MPTCP_MIB_JOINNOTOKEN),
 	SNMP_MIB_ITEM("MPJoinSynRx", MPTCP_MIB_JOINSYNRX),
+	SNMP_MIB_ITEM("MPJoinSynBackupRx", MPTCP_MIB_JOINSYNBACKUPRX),
 	SNMP_MIB_ITEM("MPJoinSynAckRx", MPTCP_MIB_JOINSYNACKRX),
+	SNMP_MIB_ITEM("MPJoinSynAckBackupRx", MPTCP_MIB_JOINSYNACKBACKUPRX),
 	SNMP_MIB_ITEM("MPJoinSynAckHMacFailure", MPTCP_MIB_JOINSYNACKMAC),
 	SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
 	SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 2704afd0dfe4..66aa67f49d03 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -14,7 +14,9 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_RETRANSSEGS,		/* Segments retransmitted at the MPTCP-level */
 	MPTCP_MIB_JOINNOTOKEN,		/* Received MP_JOIN but the token was not found */
 	MPTCP_MIB_JOINSYNRX,		/* Received a SYN + MP_JOIN */
+	MPTCP_MIB_JOINSYNBACKUPRX,	/* Received a SYN + MP_JOIN + backup flag */
 	MPTCP_MIB_JOINSYNACKRX,		/* Received a SYN/ACK + MP_JOIN */
+	MPTCP_MIB_JOINSYNACKBACKUPRX,	/* Received a SYN/ACK + MP_JOIN + backup flag */
 	MPTCP_MIB_JOINSYNACKMAC,	/* HMAC was wrong on SYN/ACK + MP_JOIN */
 	MPTCP_MIB_JOINACKRX,		/* Received an ACK + MP_JOIN */
 	MPTCP_MIB_JOINACKMAC,		/* HMAC was wrong on ACK + MP_JOIN */
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a3778aee4e77..be406197b1c4 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -168,6 +168,9 @@ static int subflow_check_req(struct request_sock *req,
 			return 0;
 	} else if (opt_mp_join) {
 		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINSYNRX);
+
+		if (mp_opt.backup)
+			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINSYNBACKUPRX);
 	}
 
 	if (opt_mp_capable && listener->request_mptcp) {
@@ -577,6 +580,9 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 		subflow->mp_join = 1;
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNACKRX);
 
+		if (subflow->backup)
+			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNACKBACKUPRX);
+
 		if (subflow_use_different_dport(msk, sk)) {
 			pr_debug("synack inet_dport=%d %d",
 				 ntohs(inet_sk(sk)->inet_dport),

-- 
2.45.2

[PATCH mptcp-net v2 5/9] selftests: mptcp: join: validate backup in MPJ

[Matthieu Baerts (NGI0)]

A peer can notify the other one that a subflow has to be treated as
"backup" by two different ways: either by sending a dedicated MP_PRIO
notification, or by setting the backup flag in the MP_JOIN handshake.

The selftests were previously monitoring the former, but not the latter.
This is what is now done here by looking at these new MIB counters when
validating the 'backup' cases:

  MPTcpExtMPJoinSynBackupRx
  MPTcpExtMPJoinSynAckBackupRx

The 'Fixes' tag here below is the same as the one from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it will help to validate a new fix for an issue introduced by this
commit ID.

Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 42 +++++++++++++++++++------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 55d84a1bde15..175127a9c00c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1634,6 +1634,8 @@ chk_prio_nr()
 {
 	local mp_prio_nr_tx=$1
 	local mp_prio_nr_rx=$2
+	local mpj_syn=$3
+	local mpj_syn_ack=$4
 	local count
 
 	print_check "ptx"
@@ -1655,6 +1657,26 @@ chk_prio_nr()
 	else
 		print_ok
 	fi
+
+	print_check "syn backup"
+	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinSynBackupRx")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$mpj_syn" ]; then
+		fail_test "got $count MPJ SYN[s] with Backup expected $mpj_syn"
+	else
+		print_ok
+	fi
+
+	print_check "synack backup"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynAckBackupRx")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$mpj_syn_ack" ]; then
+		fail_test "got $count MPJ SYNACK[s] with Backup expected $mpj_syn_ack"
+	else
+		print_ok
+	fi
 }
 
 chk_subflow_nr()
@@ -2612,7 +2634,7 @@ backup_tests()
 		sflags=nobackup speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
-		chk_prio_nr 0 1
+		chk_prio_nr 0 1 1 0
 	fi
 
 	# single address, backup
@@ -2625,7 +2647,7 @@ backup_tests()
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
-		chk_prio_nr 1 1
+		chk_prio_nr 1 1 0 0
 	fi
 
 	# single address with port, backup
@@ -2638,7 +2660,7 @@ backup_tests()
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
-		chk_prio_nr 1 1
+		chk_prio_nr 1 1 0 0
 	fi
 
 	if reset "mpc backup" &&
@@ -2647,7 +2669,7 @@ backup_tests()
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
-		chk_prio_nr 0 1
+		chk_prio_nr 0 1 0 0
 	fi
 
 	if reset "mpc backup both sides" &&
@@ -2657,7 +2679,7 @@ backup_tests()
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
-		chk_prio_nr 1 1
+		chk_prio_nr 1 1 0 0
 	fi
 
 	if reset "mpc switch to backup" &&
@@ -2666,7 +2688,7 @@ backup_tests()
 		sflags=backup speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
-		chk_prio_nr 0 1
+		chk_prio_nr 0 1 0 0
 	fi
 
 	if reset "mpc switch to backup both sides" &&
@@ -2676,7 +2698,7 @@ backup_tests()
 		sflags=backup speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
-		chk_prio_nr 1 1
+		chk_prio_nr 1 1 0 0
 	fi
 }
 
@@ -3053,7 +3075,7 @@ fullmesh_tests()
 		addr_nr_ns2=1 sflags=backup,fullmesh speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 2 2 2
-		chk_prio_nr 0 1
+		chk_prio_nr 0 1 1 0
 		chk_rm_nr 0 1
 	fi
 
@@ -3066,7 +3088,7 @@ fullmesh_tests()
 		sflags=nobackup,nofullmesh speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 2 2 2
-		chk_prio_nr 0 1
+		chk_prio_nr 0 1 1 0
 		chk_rm_nr 0 1
 	fi
 }
@@ -3318,7 +3340,7 @@ userspace_tests()
 		sflags=backup speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 0
-		chk_prio_nr 0 0
+		chk_prio_nr 0 0 0 0
 	fi
 
 	# userspace pm type prevents rm_addr

-- 
2.45.2

Re: [PATCH mptcp-net v2 5/9] selftests: mptcp: join: validate backup in MPJ

[Geliang Tang]

Hi Matt,

Thanks for these patches.

On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
> A peer can notify the other one that a subflow has to be treated as
> "backup" by two different ways: either by sending a dedicated MP_PRIO
> notification, or by setting the backup flag in the MP_JOIN handshake.
> 
> The selftests were previously monitoring the former, but not the
> latter.
> This is what is now done here by looking at these new MIB counters
> when
> validating the 'backup' cases:
> 
>   MPTcpExtMPJoinSynBackupRx
>   MPTcpExtMPJoinSynAckBackupRx
> 
> The 'Fixes' tag here below is the same as the one from the previous
> commit: this patch here is not fixing anything wrong in the
> selftests,
> but it will help to validate a new fix for an issue introduced by
> this
> commit ID.
> 
> Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 42
> +++++++++++++++++++------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 55d84a1bde15..175127a9c00c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1634,6 +1634,8 @@ chk_prio_nr()
>  {
>  	local mp_prio_nr_tx=$1
>  	local mp_prio_nr_rx=$2
> +	local mpj_syn=$3
> +	local mpj_syn_ack=$4

How about setting default values for them:

        local mpj_syn=${3:-0}
        local mpj_syn_ack=${4:-0}

>  	local count
>  
>  	print_check "ptx"
> @@ -1655,6 +1657,26 @@ chk_prio_nr()
>  	else
>  		print_ok
>  	fi
> +
> +	print_check "syn backup"
> +	count=$(mptcp_lib_get_counter ${ns1}
> "MPTcpExtMPJoinSynBackupRx")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$mpj_syn" ]; then
> +		fail_test "got $count MPJ SYN[s] with Backup
> expected $mpj_syn"

And make this log more like the one in chk_join_nr:

	fail_test "got $count JOIN[s] syn with Backup expected
$mpj_syn"

> +	else
> +		print_ok
> +	fi
> +
> +	print_check "synack backup"
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynAckBackupRx")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$mpj_syn_ack" ]; then
> +		fail_test "got $count MPJ SYNACK[s] with Backup
> expected $mpj_syn_ack"

The same here.

> +	else
> +		print_ok
> +	fi
>  }
>  
>  chk_subflow_nr()
> @@ -2612,7 +2634,7 @@ backup_tests()
>  		sflags=nobackup speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 1
> -		chk_prio_nr 0 1
> +		chk_prio_nr 0 1 1 0

The last value is the default one, only "chk_prio_nr 0 1 1" is enough.

>  	fi
>  
>  	# single address, backup
> @@ -2625,7 +2647,7 @@ backup_tests()
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 1
>  		chk_add_nr 1 1
> -		chk_prio_nr 1 1
> +		chk_prio_nr 1 1 0 0

With default values, no need to change this then.


WDYT?

Thanks,
-Geliang

>  	fi
>  
>  	# single address with port, backup
> @@ -2638,7 +2660,7 @@ backup_tests()
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 1
>  		chk_add_nr 1 1
> -		chk_prio_nr 1 1
> +		chk_prio_nr 1 1 0 0
>  	fi
>  
>  	if reset "mpc backup" &&
> @@ -2647,7 +2669,7 @@ backup_tests()
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 0 0 0
> -		chk_prio_nr 0 1
> +		chk_prio_nr 0 1 0 0
>  	fi
>  
>  	if reset "mpc backup both sides" &&
> @@ -2657,7 +2679,7 @@ backup_tests()
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 0 0 0
> -		chk_prio_nr 1 1
> +		chk_prio_nr 1 1 0 0
>  	fi
>  
>  	if reset "mpc switch to backup" &&
> @@ -2666,7 +2688,7 @@ backup_tests()
>  		sflags=backup speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 0 0 0
> -		chk_prio_nr 0 1
> +		chk_prio_nr 0 1 0 0
>  	fi
>  
>  	if reset "mpc switch to backup both sides" &&
> @@ -2676,7 +2698,7 @@ backup_tests()
>  		sflags=backup speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 0 0 0
> -		chk_prio_nr 1 1
> +		chk_prio_nr 1 1 0 0
>  	fi
>  }
>  
> @@ -3053,7 +3075,7 @@ fullmesh_tests()
>  		addr_nr_ns2=1 sflags=backup,fullmesh speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 2 2 2
> -		chk_prio_nr 0 1
> +		chk_prio_nr 0 1 1 0
>  		chk_rm_nr 0 1
>  	fi
>  
> @@ -3066,7 +3088,7 @@ fullmesh_tests()
>  		sflags=nobackup,nofullmesh speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 2 2 2
> -		chk_prio_nr 0 1
> +		chk_prio_nr 0 1 1 0
>  		chk_rm_nr 0 1
>  	fi
>  }
> @@ -3318,7 +3340,7 @@ userspace_tests()
>  		sflags=backup speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 0
> -		chk_prio_nr 0 0
> +		chk_prio_nr 0 0 0 0
>  	fi
>  
>  	# userspace pm type prevents rm_addr
> 

Re: [PATCH mptcp-net v2 5/9] selftests: mptcp: join: validate backup in MPJ

[Matthieu Baerts]

Hi Geliang,

On 17/07/2024 03:25, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for these patches.
> 
> On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
>> A peer can notify the other one that a subflow has to be treated as
>> "backup" by two different ways: either by sending a dedicated MP_PRIO
>> notification, or by setting the backup flag in the MP_JOIN handshake.
>>
>> The selftests were previously monitoring the former, but not the
>> latter.
>> This is what is now done here by looking at these new MIB counters
>> when
>> validating the 'backup' cases:
>>
>>   MPTcpExtMPJoinSynBackupRx
>>   MPTcpExtMPJoinSynAckBackupRx
>>
>> The 'Fixes' tag here below is the same as the one from the previous
>> commit: this patch here is not fixing anything wrong in the
>> selftests,
>> but it will help to validate a new fix for an issue introduced by
>> this
>> commit ID.
>>
>> Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 42
>> +++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 55d84a1bde15..175127a9c00c 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1634,6 +1634,8 @@ chk_prio_nr()
>>  {
>>  	local mp_prio_nr_tx=$1
>>  	local mp_prio_nr_rx=$2
>> +	local mpj_syn=$3
>> +	local mpj_syn_ack=$4
> 
> How about setting default values for them:
> 
>         local mpj_syn=${3:-0}
>         local mpj_syn_ack=${4:-0}

I prefer not to: in our existing test suite, these counters can be set,
it is then important to review all of these test cases.

I also think it is important to check them in all our backup cases.
That's why I don't want to not check them if they are not set.

> 
>>  	local count
>>  
>>  	print_check "ptx"
>> @@ -1655,6 +1657,26 @@ chk_prio_nr()
>>  	else
>>  		print_ok
>>  	fi
>> +
>> +	print_check "syn backup"
>> +	count=$(mptcp_lib_get_counter ${ns1}
>> "MPTcpExtMPJoinSynBackupRx")
>> +	if [ -z "$count" ]; then
>> +		print_skip
>> +	elif [ "$count" != "$mpj_syn" ]; then
>> +		fail_test "got $count MPJ SYN[s] with Backup
>> expected $mpj_syn"
> 
> And make this log more like the one in chk_join_nr:
> 
> 	fail_test "got $count JOIN[s] syn with Backup expected
> $mpj_syn"

Good point!

>> +	else
>> +		print_ok
>> +	fi
>> +
>> +	print_check "synack backup"
>> +	count=$(mptcp_lib_get_counter ${ns2}
>> "MPTcpExtMPJoinSynAckBackupRx")
>> +	if [ -z "$count" ]; then
>> +		print_skip
>> +	elif [ "$count" != "$mpj_syn_ack" ]; then
>> +		fail_test "got $count MPJ SYNACK[s] with Backup
>> expected $mpj_syn_ack"
> 
> The same here.

Will do!

>> +	else
>> +		print_ok
>> +	fi
>>  }
>>  
>>  chk_subflow_nr()
>> @@ -2612,7 +2634,7 @@ backup_tests()
>>  		sflags=nobackup speed=slow \
>>  			run_tests $ns1 $ns2 10.0.1.1
>>  		chk_join_nr 1 1 1
>> -		chk_prio_nr 0 1
>> +		chk_prio_nr 0 1 1 0
> 
> The last value is the default one, only "chk_prio_nr 0 1 1" is enough.

I think it is also better to avoid that in the 'chk_*' helpers,
otherwise it is hard to follow what is going to be done.

> 
>>  	fi
>>  
>>  	# single address, backup
>> @@ -2625,7 +2647,7 @@ backup_tests()
>>  			run_tests $ns1 $ns2 10.0.1.1
>>  		chk_join_nr 1 1 1
>>  		chk_add_nr 1 1
>> -		chk_prio_nr 1 1
>> +		chk_prio_nr 1 1 0 0
> 
> With default values, no need to change this then.

Please note that I also find this change safer for the backports: if the
commit cannot be backported without conflicts, we can check why: maybe
the test behaviour was different before, and the number of MPJ with
backup flags was different. (But this is only because here it is a
series for -net. If we extend a check to verify more things in very
specific cases, something that doesn't make sense to check in other
cases, then it can be fine to have optional parameters I think.)

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

[PATCH mptcp-net v2 6/9] mptcp: pm: fix backup support in signal endpoints

[Matthieu Baerts (NGI0)]

There was a support for signal endpoints, but only when the endpoint's
flag was changed during a connection. If an endpoint with the signal and
backup was already present, the MP_JOIN reply was not containing the
backup flag as expected.

That's confusing to have this inconsistent behaviour. On the other hand,
the infrastructure to set the backup flag in the SYN + ACK + MP_JOIN was
already there, it was just never set before. Now when requesting the
local ID from the path-manager, the backup status is also requested.

There is a special case for the ID0: the PM has to return this ID0 if
the local address of the initial subflow is being used, and not the ID
of the related endpoint. Still, it is required to look at the different
endpoints to find if one has been defined for this address with the
backup flag.

Note that when the userspace PM is used, the backup flag can be set if
the local address was already used before with a backup flag, e.g. if
the address was announced with the 'backup' flag, or a subflow was
created with the 'backup' flag.

Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/507
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
 - v2:
   - Only set *backup on success (Mat).
   - Support MPJ to/from ID0 with backup.
   - Split the tests in a new dedicated commit to ease the backports.
---
 net/mptcp/pm.c           | 15 +++++++++++----
 net/mptcp/pm_netlink.c   | 23 ++++++++++++++++++++++-
 net/mptcp/pm_userspace.c | 25 +++++++++++++++++++++++--
 net/mptcp/protocol.h     | 11 ++++++++---
 net/mptcp/subflow.c      |  9 +++++++--
 5 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 55406720c607..359738b8826e 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -405,7 +405,8 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 	return ret;
 }
 
-int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
+int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc,
+			  bool *backup)
 {
 	struct mptcp_addr_info skc_local;
 	struct mptcp_addr_info msk_local;
@@ -418,12 +419,18 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 	 */
 	mptcp_local_address((struct sock_common *)msk, &msk_local);
 	mptcp_local_address((struct sock_common *)skc, &skc_local);
-	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
+	if (mptcp_addresses_equal(&msk_local, &skc_local, false)) {
+		if (mptcp_pm_is_userspace(msk))
+			*backup = mptcp_userspace_pm_is_backup(msk, &skc_local);
+		else
+			*backup = mptcp_pm_nl_is_backup(msk, &skc_local);
+
 		return 0;
+	}
 
 	if (mptcp_pm_is_userspace(msk))
-		return mptcp_userspace_pm_get_local_id(msk, &skc_local);
-	return mptcp_pm_nl_get_local_id(msk, &skc_local);
+		return mptcp_userspace_pm_get_local_id(msk, &skc_local, backup);
+	return mptcp_pm_nl_get_local_id(msk, &skc_local, backup);
 }
 
 int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id,
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 7635fac91539..796800a7fe96 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1064,7 +1064,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	return err;
 }
 
-int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
+int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc,
+			     bool *backup)
 {
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
@@ -1076,6 +1077,7 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
 		if (mptcp_addresses_equal(&entry->addr, skc, entry->addr.port)) {
 			ret = entry->addr.id;
+			*backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 			break;
 		}
 	}
@@ -1094,6 +1096,7 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	entry->ifindex = 0;
 	entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
 	entry->lsk = NULL;
+	*backup = false;
 	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry, true);
 	if (ret < 0)
 		kfree(entry);
@@ -1101,6 +1104,24 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	return ret;
 }
 
+bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
+{
+	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+	struct mptcp_pm_addr_entry *entry;
+	bool backup = false;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
+		if (mptcp_addresses_equal(&entry->addr, skc, entry->addr.port)) {
+			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+			break;
+		}
+	}
+	rcu_read_unlock();
+
+	return backup;
+}
+
 #define MPTCP_PM_CMD_GRP_OFFSET       0
 #define MPTCP_PM_EV_GRP_OFFSET        1
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index f0a4590506c6..4e3829f96c88 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -137,7 +137,7 @@ int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
 }
 
 int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_addr_info *skc)
+				    struct mptcp_addr_info *skc, bool *backup)
 {
 	struct mptcp_pm_addr_entry *entry = NULL, *e, new_entry;
 	__be16 msk_sport =  ((struct inet_sock *)
@@ -151,13 +151,16 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
 		}
 	}
 	spin_unlock_bh(&msk->pm.lock);
-	if (entry)
+	if (entry) {
+		*backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 		return entry->addr.id;
+	}
 
 	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
 	new_entry.addr = *skc;
 	new_entry.addr.id = 0;
 	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
+	*backup = false;
 
 	if (new_entry.addr.port == msk_sport)
 		new_entry.addr.port = 0;
@@ -165,6 +168,24 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
 	return mptcp_userspace_pm_append_new_local_addr(msk, &new_entry, true);
 }
 
+bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
+				  struct mptcp_addr_info *skc)
+{
+	struct mptcp_pm_addr_entry *entry;
+	bool backup = false;
+
+	spin_lock_bh(&msk->pm.lock);
+	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
+		if (mptcp_addresses_equal(&entry->addr, skc, false)) {
+			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+			break;
+		}
+	}
+	spin_unlock_bh(&msk->pm.lock);
+
+	return backup;
+}
+
 int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 6b6b76152db5..e6cc7111f843 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1111,9 +1111,14 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, const struct sk_buff *skb,
 			      bool *drop_other_suboptions);
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
-int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
-int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
-int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
+int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc,
+			  bool *backup);
+int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc,
+			     bool *backup);
+int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc,
+				    bool *backup);
+bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
+bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
 int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb);
 int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			  struct netlink_callback *cb);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index be406197b1c4..e60ba0327e4d 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -87,6 +87,7 @@ static struct mptcp_sock *subflow_token_join_request(struct request_sock *req)
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
 	struct mptcp_sock *msk;
 	int local_id;
+	bool backup;
 
 	msk = mptcp_token_get_sock(sock_net(req_to_sk(req)), subflow_req->token);
 	if (!msk) {
@@ -94,12 +95,13 @@ static struct mptcp_sock *subflow_token_join_request(struct request_sock *req)
 		return NULL;
 	}
 
-	local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req);
+	local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req, &backup);
 	if (local_id < 0) {
 		sock_put((struct sock *)msk);
 		return NULL;
 	}
 	subflow_req->local_id = local_id;
+	subflow_req->request_bkup = backup;
 
 	return msk;
 }
@@ -610,16 +612,19 @@ static int subflow_chk_local_id(struct sock *sk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+	bool backup;
 	int err;
 
 	if (likely(subflow->local_id >= 0))
 		return 0;
 
-	err = mptcp_pm_get_local_id(msk, (struct sock_common *)sk);
+	err = mptcp_pm_get_local_id(msk, (struct sock_common *)sk, &backup);
 	if (err < 0)
 		return err;
 
 	subflow_set_local_id(subflow, err);
+	subflow->request_bkup = backup;
+
 	return 0;
 }
 

-- 
2.45.2

Re: [PATCH mptcp-net v2 6/9] mptcp: pm: fix backup support in signal endpoints

[Geliang Tang]

Hi Matt,

On Tue, Jul 16, 2024 at 10:53:18PM +0200, Matthieu Baerts (NGI0) wrote:
> There was a support for signal endpoints, but only when the endpoint's
> flag was changed during a connection. If an endpoint with the signal and
> backup was already present, the MP_JOIN reply was not containing the
> backup flag as expected.
> 
> That's confusing to have this inconsistent behaviour. On the other hand,
> the infrastructure to set the backup flag in the SYN + ACK + MP_JOIN was
> already there, it was just never set before. Now when requesting the
> local ID from the path-manager, the backup status is also requested.
> 
> There is a special case for the ID0: the PM has to return this ID0 if
> the local address of the initial subflow is being used, and not the ID
> of the related endpoint. Still, it is required to look at the different
> endpoints to find if one has been defined for this address with the
> backup flag.
> 
> Note that when the userspace PM is used, the backup flag can be set if
> the local address was already used before with a backup flag, e.g. if
> the address was announced with the 'backup' flag, or a subflow was
> created with the 'backup' flag.
> 
> Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/507
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>  - v2:
>    - Only set *backup on success (Mat).
>    - Support MPJ to/from ID0 with backup.
>    - Split the tests in a new dedicated commit to ease the backports.
> ---
>  net/mptcp/pm.c           | 15 +++++++++++----
>  net/mptcp/pm_netlink.c   | 23 ++++++++++++++++++++++-
>  net/mptcp/pm_userspace.c | 25 +++++++++++++++++++++++--
>  net/mptcp/protocol.h     | 11 ++++++++---
>  net/mptcp/subflow.c      |  9 +++++++--
>  5 files changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 55406720c607..359738b8826e 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -405,7 +405,8 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>  	return ret;
>  }
>  
> -int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
> +int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc,
> +			  bool *backup)
>  {
>  	struct mptcp_addr_info skc_local;
>  	struct mptcp_addr_info msk_local;
> @@ -418,12 +419,18 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  	 */
>  	mptcp_local_address((struct sock_common *)msk, &msk_local);
>  	mptcp_local_address((struct sock_common *)skc, &skc_local);
> -	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
> +	if (mptcp_addresses_equal(&msk_local, &skc_local, false)) {
> +		if (mptcp_pm_is_userspace(msk))
> +			*backup = mptcp_userspace_pm_is_backup(msk, &skc_local);
> +		else
> +			*backup = mptcp_pm_nl_is_backup(msk, &skc_local);
> +
>  		return 0;
> +	}
>  
>  	if (mptcp_pm_is_userspace(msk))
> -		return mptcp_userspace_pm_get_local_id(msk, &skc_local);
> -	return mptcp_pm_nl_get_local_id(msk, &skc_local);
> +		return mptcp_userspace_pm_get_local_id(msk, &skc_local, backup);
> +	return mptcp_pm_nl_get_local_id(msk, &skc_local, backup);
>  }

I think that instead of mixing "backup" into get_local_id() interface, it is much
better to add a new interface is_backup() for PM in pm.c:

bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
{
       struct mptcp_addr_info skc_local;

       mptcp_local_address((struct sock_common *)skc, &skc_local);

       if (mptcp_pm_is_userspace(msk))
               return mptcp_userspace_pm_is_backup(msk, &skc_local);
       return mptcp_pm_nl_is_backup(msk, &skc_local);
}

No need to test whether the two addresses msk_local and skc_local are the same
in mptcp_pm_is_backup(), just invoke mptcp_pm_nl_is_backup() or
mptcp_userspace_pm_is_backup() with skc_local.

>  
>  int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id,
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 7635fac91539..796800a7fe96 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1064,7 +1064,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  	return err;
>  }
>  
> -int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
> +int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc,
> +			     bool *backup)
>  {
>  	struct mptcp_pm_addr_entry *entry;
>  	struct pm_nl_pernet *pernet;
> @@ -1076,6 +1077,7 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
>  	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
>  		if (mptcp_addresses_equal(&entry->addr, skc, entry->addr.port)) {
>  			ret = entry->addr.id;
> +			*backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
>  			break;
>  		}
>  	}
> @@ -1094,6 +1096,7 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
>  	entry->ifindex = 0;
>  	entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
>  	entry->lsk = NULL;
> +	*backup = false;
>  	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry, true);
>  	if (ret < 0)
>  		kfree(entry);
> @@ -1101,6 +1104,24 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
>  	return ret;
>  }
>  
> +bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
> +{
> +	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
> +	struct mptcp_pm_addr_entry *entry;
> +	bool backup = false;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
> +		if (mptcp_addresses_equal(&entry->addr, skc, entry->addr.port)) {
> +			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> +			break;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	return backup;
> +}
> +
>  #define MPTCP_PM_CMD_GRP_OFFSET       0
>  #define MPTCP_PM_EV_GRP_OFFSET        1
>  
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index f0a4590506c6..4e3829f96c88 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -137,7 +137,7 @@ int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
>  }
>  
>  int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> -				    struct mptcp_addr_info *skc)
> +				    struct mptcp_addr_info *skc, bool *backup)
>  {
>  	struct mptcp_pm_addr_entry *entry = NULL, *e, new_entry;
>  	__be16 msk_sport =  ((struct inet_sock *)
> @@ -151,13 +151,16 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
>  		}
>  	}
>  	spin_unlock_bh(&msk->pm.lock);
> -	if (entry)
> +	if (entry) {
> +		*backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
>  		return entry->addr.id;
> +	}
>  
>  	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
>  	new_entry.addr = *skc;
>  	new_entry.addr.id = 0;
>  	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> +	*backup = false;
>  
>  	if (new_entry.addr.port == msk_sport)
>  		new_entry.addr.port = 0;
> @@ -165,6 +168,24 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
>  	return mptcp_userspace_pm_append_new_local_addr(msk, &new_entry, true);
>  }
>  
> +bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
> +				  struct mptcp_addr_info *skc)
> +{
> +	struct mptcp_pm_addr_entry *entry;
> +	bool backup = false;
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
> +		if (mptcp_addresses_equal(&entry->addr, skc, false)) {
> +			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> +			break;
> +		}
> +	}
> +	spin_unlock_bh(&msk->pm.lock);
> +
> +	return backup;
> +}
> +
>  int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 6b6b76152db5..e6cc7111f843 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1111,9 +1111,14 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, const struct sk_buff *skb,
>  			      bool *drop_other_suboptions);
>  bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>  			     struct mptcp_rm_list *rm_list);
> -int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> -int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
> -int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
> +int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc,
> +			  bool *backup);
> +int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc,
> +			     bool *backup);
> +int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc,
> +				    bool *backup);
> +bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
> +bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
>  int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb);
>  int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  			  struct netlink_callback *cb);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index be406197b1c4..e60ba0327e4d 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -87,6 +87,7 @@ static struct mptcp_sock *subflow_token_join_request(struct request_sock *req)
>  	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
>  	struct mptcp_sock *msk;
>  	int local_id;
> +	bool backup;
>  
>  	msk = mptcp_token_get_sock(sock_net(req_to_sk(req)), subflow_req->token);
>  	if (!msk) {
> @@ -94,12 +95,13 @@ static struct mptcp_sock *subflow_token_join_request(struct request_sock *req)
>  		return NULL;
>  	}
>  
> -	local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req);
> +	local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req, &backup);
>  	if (local_id < 0) {
>  		sock_put((struct sock *)msk);
>  		return NULL;
>  	}
>  	subflow_req->local_id = local_id;
> +	subflow_req->request_bkup = backup;

Always invoke is_backup() after get_local_id():
	
	subflow_req->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)req);

>  
>  	return msk;
>  }
> @@ -610,16 +612,19 @@ static int subflow_chk_local_id(struct sock *sk)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>  	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> +	bool backup;
>  	int err;
>  
>  	if (likely(subflow->local_id >= 0))
>  		return 0;
>  
> -	err = mptcp_pm_get_local_id(msk, (struct sock_common *)sk);
> +	err = mptcp_pm_get_local_id(msk, (struct sock_common *)sk, &backup);
>  	if (err < 0)
>  		return err;
>  
>  	subflow_set_local_id(subflow, err);
> +	subflow->request_bkup = backup;

	subflow->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)sk);

WDYT?

Thanks,
-Geliang

> +
>  	return 0;
>  }
>  
> 
> -- 
> 2.45.2
> 

Re: [PATCH mptcp-net v2 6/9] mptcp: pm: fix backup support in signal endpoints

[Matthieu Baerts]

Hi Geliang,

Thank you for the review!

On 17/07/2024 05:04, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, Jul 16, 2024 at 10:53:18PM +0200, Matthieu Baerts (NGI0) wrote:
>> There was a support for signal endpoints, but only when the endpoint's
>> flag was changed during a connection. If an endpoint with the signal and
>> backup was already present, the MP_JOIN reply was not containing the
>> backup flag as expected.
>>
>> That's confusing to have this inconsistent behaviour. On the other hand,
>> the infrastructure to set the backup flag in the SYN + ACK + MP_JOIN was
>> already there, it was just never set before. Now when requesting the
>> local ID from the path-manager, the backup status is also requested.
>>
>> There is a special case for the ID0: the PM has to return this ID0 if
>> the local address of the initial subflow is being used, and not the ID
>> of the related endpoint. Still, it is required to look at the different
>> endpoints to find if one has been defined for this address with the
>> backup flag.
>>
>> Note that when the userspace PM is used, the backup flag can be set if
>> the local address was already used before with a backup flag, e.g. if
>> the address was announced with the 'backup' flag, or a subflow was
>> created with the 'backup' flag.

(...)

>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index 55406720c607..359738b8826e 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -405,7 +405,8 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>  	return ret;
>>  }
>>  
>> -int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>> +int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc,
>> +			  bool *backup)
>>  {
>>  	struct mptcp_addr_info skc_local;
>>  	struct mptcp_addr_info msk_local;
>> @@ -418,12 +419,18 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>>  	 */
>>  	mptcp_local_address((struct sock_common *)msk, &msk_local);
>>  	mptcp_local_address((struct sock_common *)skc, &skc_local);
>> -	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
>> +	if (mptcp_addresses_equal(&msk_local, &skc_local, false)) {
>> +		if (mptcp_pm_is_userspace(msk))
>> +			*backup = mptcp_userspace_pm_is_backup(msk, &skc_local);
>> +		else
>> +			*backup = mptcp_pm_nl_is_backup(msk, &skc_local);
>> +
>>  		return 0;
>> +	}
>>  
>>  	if (mptcp_pm_is_userspace(msk))
>> -		return mptcp_userspace_pm_get_local_id(msk, &skc_local);
>> -	return mptcp_pm_nl_get_local_id(msk, &skc_local);
>> +		return mptcp_userspace_pm_get_local_id(msk, &skc_local, backup);
>> +	return mptcp_pm_nl_get_local_id(msk, &skc_local, backup);
>>  }
> 
> I think that instead of mixing "backup" into get_local_id() interface, it is much
> better to add a new interface is_backup() for PM in pm.c:
> 
> bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
> {
>        struct mptcp_addr_info skc_local;
> 
>        mptcp_local_address((struct sock_common *)skc, &skc_local);
> 
>        if (mptcp_pm_is_userspace(msk))
>                return mptcp_userspace_pm_is_backup(msk, &skc_local);
>        return mptcp_pm_nl_is_backup(msk, &skc_local);
> }
> 
> No need to test whether the two addresses msk_local and skc_local are the same
> in mptcp_pm_is_backup(), just invoke mptcp_pm_nl_is_backup() or
> mptcp_userspace_pm_is_backup() with skc_local.
I initially wanted to avoid iterating the entry list twice in some
cases, but this special case for the ID0 increases the complexity of the
code. I can indeed switch to a new dedicated helper, thanks!

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

[PATCH mptcp-net v2 7/9] selftests: mptcp: join: check backup support in signal endp

[Matthieu Baerts (NGI0)]

Before the previous commit, 'signal' endpoints with the 'backup' flag
were ignored when sending the MP_JOIN.

The MPTCP Join selftest has then been modified to validate this case:
the "single address, backup" test, is now validating the MP_JOIN with a
backup flag as it is what we expect it to do with such name. The
previous version has been kept, but renamed to "single address, switch
to backup" to avoid confusions.

The "single address with port, backup" test is also now validating the
MPJ with a backup flag, which makes more sense than checking the switch
to backup with an MP_PRIO.

The "mpc backup both sides" test is now validating that the backup flag
is also set in MP_JOIN from and to the addresses used in the initial
subflow, using the special ID 0.

The 'Fixes' tag here below is the same as the one from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it validates the previous fix for an issue introduced by this commit
ID.

Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
 - v2:
   - Also validating MPJ to/from ID0 with backup
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 34 ++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 175127a9c00c..ffcf558b4610 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2639,6 +2639,19 @@ backup_tests()
 
 	# single address, backup
 	if reset "single address, backup" &&
+	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
+		pm_nl_set_limits $ns1 0 1
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
+		pm_nl_set_limits $ns2 1 1
+		sflags=nobackup speed=slow \
+			run_tests $ns1 $ns2 10.0.1.1
+		chk_join_nr 1 1 1
+		chk_add_nr 1 1
+		chk_prio_nr 1 0 0 1
+	fi
+
+	# single address, switch to backup
+	if reset "single address, switch to backup" &&
 	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
 		pm_nl_set_limits $ns1 0 1
 		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
@@ -2654,13 +2667,13 @@ backup_tests()
 	if reset "single address with port, backup" &&
 	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
 		pm_nl_set_limits $ns1 0 1
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal port 10100
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup port 10100
 		pm_nl_set_limits $ns2 1 1
-		sflags=backup speed=slow \
+		sflags=nobackup speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
-		chk_prio_nr 1 1 0 0
+		chk_prio_nr 1 0 0 1
 	fi
 
 	if reset "mpc backup" &&
@@ -2674,12 +2687,21 @@ backup_tests()
 
 	if reset "mpc backup both sides" &&
 	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
-		pm_nl_add_endpoint $ns1 10.0.1.1 flags subflow,backup
+		pm_nl_set_limits $ns1 0 2
+		pm_nl_set_limits $ns2 1 2
+		pm_nl_add_endpoint $ns1 10.0.1.1 flags signal,backup
 		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow,backup
+
+		# 10.0.2.2 (non-backup) -> 10.0.1.1 (backup)
+		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
+		# 10.0.1.2 (backup) -> 10.0.2.1 (non-backup)
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		ip -net "$ns2" route add 10.0.2.1 via 10.0.1.1 dev ns2eth1 # force this path
+
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 0 0 0
-		chk_prio_nr 1 1 0 0
+		chk_join_nr 2 2 2
+		chk_prio_nr 1 1 1 1
 	fi
 
 	if reset "mpc switch to backup" &&

-- 
2.45.2

Re: [PATCH mptcp-net v2 7/9] selftests: mptcp: join: check backup support in signal endp

[Geliang Tang]

On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
> Before the previous commit, 'signal' endpoints with the 'backup' flag
> were ignored when sending the MP_JOIN.
> 
> The MPTCP Join selftest has then been modified to validate this case:
> the "single address, backup" test, is now validating the MP_JOIN with
> a
> backup flag as it is what we expect it to do with such name. The
> previous version has been kept, but renamed to "single address,
> switch
> to backup" to avoid confusions.
> 
> The "single address with port, backup" test is also now validating
> the
> MPJ with a backup flag, which makes more sense than checking the
> switch
> to backup with an MP_PRIO.
> 
> The "mpc backup both sides" test is now validating that the backup
> flag
> is also set in MP_JOIN from and to the addresses used in the initial
> subflow, using the special ID 0.
> 
> The 'Fixes' tag here below is the same as the one from the previous
> commit: this patch here is not fixing anything wrong in the
> selftests,
> but it validates the previous fix for an issue introduced by this
> commit
> ID.
> 
> Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>  - v2:
>    - Also validating MPJ to/from ID0 with backup
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 34
> ++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 175127a9c00c..ffcf558b4610 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2639,6 +2639,19 @@ backup_tests()
>  
>  	# single address, backup
>  	if reset "single address, backup" &&
> +	   continue_if mptcp_lib_kallsyms_has
> "subflow_rebuild_header$"; then
> +		pm_nl_set_limits $ns1 0 1
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
> +		pm_nl_set_limits $ns2 1 1
> +		sflags=nobackup speed=slow \
> +			run_tests $ns1 $ns2 10.0.1.1
> +		chk_join_nr 1 1 1
> +		chk_add_nr 1 1
> +		chk_prio_nr 1 0 0 1
> +	fi
> +
> +	# single address, switch to backup
> +	if reset "single address, switch to backup" &&
>  	   continue_if mptcp_lib_kallsyms_has
> "subflow_rebuild_header$"; then
>  		pm_nl_set_limits $ns1 0 1
>  		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> @@ -2654,13 +2667,13 @@ backup_tests()

'''
>  	if reset "single address with port, backup" &&
>  	   continue_if mptcp_lib_kallsyms_has
> "subflow_rebuild_header$"; then
>  		pm_nl_set_limits $ns1 0 1
> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal port
> 10100
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
> port 10100
>  		pm_nl_set_limits $ns2 1 1
> -		sflags=backup speed=slow \
> +		sflags=nobackup speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 1
>  		chk_add_nr 1 1
> -		chk_prio_nr 1 1 0 0
> +		chk_prio_nr 1 0 0 1
>  	fi
'''

How about keep this block unchanged. "nobackup" is already tested in
the above newly added test "single address, backup"? They are almost
the same now.

WDYT?

Thanks,
-Geliang

>  
>  	if reset "mpc backup" &&
> @@ -2674,12 +2687,21 @@ backup_tests()
>  
>  	if reset "mpc backup both sides" &&
>  	   continue_if mptcp_lib_kallsyms_doesnt_have "T
> mptcp_subflow_send_ack$"; then
> -		pm_nl_add_endpoint $ns1 10.0.1.1 flags
> subflow,backup
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 1 2
> +		pm_nl_add_endpoint $ns1 10.0.1.1 flags signal,backup
>  		pm_nl_add_endpoint $ns2 10.0.1.2 flags
> subflow,backup
> +
> +		# 10.0.2.2 (non-backup) -> 10.0.1.1 (backup)
> +		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
> +		# 10.0.1.2 (backup) -> 10.0.2.1 (non-backup)
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> +		ip -net "$ns2" route add 10.0.2.1 via 10.0.1.1 dev
> ns2eth1 # force this path
> +
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 0 0 0
> -		chk_prio_nr 1 1 0 0
> +		chk_join_nr 2 2 2
> +		chk_prio_nr 1 1 1 1
>  	fi
>  
>  	if reset "mpc switch to backup" &&
> 

Re: [PATCH mptcp-net v2 7/9] selftests: mptcp: join: check backup support in signal endp

[Matthieu Baerts]

On 17/07/2024 06:39, Geliang Tang wrote:
> On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
>> Before the previous commit, 'signal' endpoints with the 'backup' flag
>> were ignored when sending the MP_JOIN.
>>
>> The MPTCP Join selftest has then been modified to validate this case:
>> the "single address, backup" test, is now validating the MP_JOIN with
>> a
>> backup flag as it is what we expect it to do with such name. The
>> previous version has been kept, but renamed to "single address,
>> switch
>> to backup" to avoid confusions.
>>
>> The "single address with port, backup" test is also now validating
>> the
>> MPJ with a backup flag, which makes more sense than checking the
>> switch
>> to backup with an MP_PRIO.
>>
>> The "mpc backup both sides" test is now validating that the backup
>> flag
>> is also set in MP_JOIN from and to the addresses used in the initial
>> subflow, using the special ID 0.
>>
>> The 'Fixes' tag here below is the same as the one from the previous
>> commit: this patch here is not fixing anything wrong in the
>> selftests,
>> but it validates the previous fix for an issue introduced by this
>> commit
>> ID.
>>
>> Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>>  - v2:
>>    - Also validating MPJ to/from ID0 with backup
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 34
>> ++++++++++++++++++++-----
>>  1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 175127a9c00c..ffcf558b4610 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -2639,6 +2639,19 @@ backup_tests()
>>  
>>  	# single address, backup
>>  	if reset "single address, backup" &&
>> +	   continue_if mptcp_lib_kallsyms_has
>> "subflow_rebuild_header$"; then
>> +		pm_nl_set_limits $ns1 0 1
>> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
>> +		pm_nl_set_limits $ns2 1 1
>> +		sflags=nobackup speed=slow \
>> +			run_tests $ns1 $ns2 10.0.1.1
>> +		chk_join_nr 1 1 1
>> +		chk_add_nr 1 1
>> +		chk_prio_nr 1 0 0 1
>> +	fi
>> +
>> +	# single address, switch to backup
>> +	if reset "single address, switch to backup" &&
>>  	   continue_if mptcp_lib_kallsyms_has
>> "subflow_rebuild_header$"; then
>>  		pm_nl_set_limits $ns1 0 1
>>  		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>> @@ -2654,13 +2667,13 @@ backup_tests()
> 
> '''
>>  	if reset "single address with port, backup" &&
>>  	   continue_if mptcp_lib_kallsyms_has
>> "subflow_rebuild_header$"; then
>>  		pm_nl_set_limits $ns1 0 1
>> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal port
>> 10100
>> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
>> port 10100
>>  		pm_nl_set_limits $ns2 1 1
>> -		sflags=backup speed=slow \
>> +		sflags=nobackup speed=slow \
>>  			run_tests $ns1 $ns2 10.0.1.1
>>  		chk_join_nr 1 1 1
>>  		chk_add_nr 1 1
>> -		chk_prio_nr 1 1 0 0
>> +		chk_prio_nr 1 0 0 1
>>  	fi
> '''
> 
> How about keep this block unchanged. "nobackup" is already tested in
> the above newly added test "single address, backup"? They are almost
> the same now.

The switch from 'nobackup' to 'backup' during the connection is also
tested just before, in "single address, switch to backup".

I think it makes more sense to validate the cases where the endpoints
are set with the backup flag before establishing connections: that
sounds like the most common use-cases. I guess people don't play with
the 'endpoint change (no)backup' option that often, no?

Supporting the switch of the backup flag for existing connections seems
like a nice to have feature to me, but not the main one related to the
backup case. So I think we should reflect that in our test suite. (And
still, we validate the switch here, but now we also check that the flag
is correctly set in the most common cases).

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

[PATCH mptcp-net v2 8/9] Squash to "selftests/bpf: Add bpf_bkup scheduler & test"

[Matthieu Baerts (NGI0)]

Similar to "mptcp: sched: check both directions for backup": it didn't
matter before because the two flags were very likely set by accident.
But that's no longer the case now.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
index 14091bcc4cde..296f0318d843 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_bkup.c
@@ -29,7 +29,8 @@ int BPF_PROG(bpf_bkup_get_subflow, struct mptcp_sock *msk,
 		if (!subflow)
 			break;
 
-		if (!BPF_CORE_READ_BITFIELD_PROBED(subflow, backup)) {
+		if (!BPF_CORE_READ_BITFIELD_PROBED(subflow, backup) ||
+		    !BPF_CORE_READ_BITFIELD_PROBED(subflow, request_bkup)) {
 			nr = i;
 			break;
 		}

-- 
2.45.2

[PATCH mptcp-net v2 9/9] Squash to "selftests/bpf: Add bpf_burst scheduler & test"

[Matthieu Baerts (NGI0)]

Similar to "mptcp: sched: check both directions for backup": it didn't
matter before because the two flags were very likely set by accident.
But that's no longer the case now.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
index 0026587a94df..3a673291b4dd 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_burst.c
@@ -85,10 +85,14 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
 	}
 
 	for (i = 0; i < data->subflows && i < MPTCP_SUBFLOWS_MAX; i++) {
+		bool backup;
+
 		subflow = bpf_mptcp_subflow_ctx_by_pos(data, i);
 		if (!subflow)
 			break;
 
+		backup = subflow->backup || subflow->request_bkup;
+
 		ssk = mptcp_subflow_tcp_sock(subflow);
 		if (!mptcp_subflow_active(subflow))
 			continue;
@@ -103,9 +107,9 @@ static int bpf_burst_get_send(struct mptcp_sock *msk,
 		}
 
 		linger_time = div_u64((__u64)ssk->sk_wmem_queued << 32, pace);
-		if (linger_time < send_info[subflow->backup].linger_time) {
-			send_info[subflow->backup].subflow_id = i;
-			send_info[subflow->backup].linger_time = linger_time;
+		if (linger_time < send_info[backup].linger_time) {
+			send_info[backup].subflow_id = i;
+			send_info[backup].linger_time = linger_time;
 		}
 	}
 	mptcp_set_timeout(sk);
@@ -160,7 +164,7 @@ static int bpf_burst_get_retrans(struct mptcp_sock *msk,
 			continue;
 		}
 
-		if (subflow->backup) {
+		if (subflow->backup || subflow->request_bkup) {
 			if (backup == MPTCP_SUBFLOWS_MAX)
 				backup = i;
 			continue;

-- 
2.45.2

Re: [PATCH mptcp-net v2 0/9] mptcp: fix inconsistent backup usage

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Unstable: 4 failed test(s): packetdrill_mp_join packetdrill_mp_prio packetdrill_regressions packetdrill_syscalls 🔴
- KVM Validation: debug: Unstable: 4 failed test(s): packetdrill_mp_join packetdrill_mp_prio packetdrill_regressions packetdrill_syscalls 🔴
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/9964158437

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


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)