Add the mibs required to cover the few possible fallback causes still
lacking suck info.

Move the relevant mib increment into the fallback helper, so that no
eventual future fallback operation will miss a paired mib increment.

Additionally track failed fallback via its own mib.

While at the above, rename an existing helper to reduce long lines
problems all along.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
still a couple of long lines I could not trim
---
 net/mptcp/ctrl.c     |  4 ++--
 net/mptcp/mib.c      |  5 +++++
 net/mptcp/mib.h      |  7 +++++++
 net/mptcp/options.c  |  2 +-
 net/mptcp/protocol.c | 44 ++++++++++++++++++++++++++++++--------------
 net/mptcp/protocol.h | 31 ++++++++-----------------------
 net/mptcp/subflow.c  | 10 ++++------
 7 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index d9290c5bb6c7..fed40dae5583 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -533,9 +533,9 @@ void mptcp_active_detect_blackhole(struct sock *ssk, bool expired)
 	to_max = mptcp_get_pernet(net)->syn_retrans_before_tcp_fallback;
 
 	if (timeouts == to_max || (timeouts < to_max && expired)) {
-		MPTCP_INC_STATS(net, MPTCP_MIB_MPCAPABLEACTIVEDROP);
 		subflow->mpc_drop = 1;
-		mptcp_subflow_early_fallback(mptcp_sk(subflow->conn), subflow);
+		mptcp_early_fallback(mptcp_sk(subflow->conn), subflow,
+				     MPTCP_MIB_MPCAPABLEACTIVEDROP);
 	}
 }
 
diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 0c24545f0e8d..064f5eaf0595 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -80,6 +80,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("RcvWndConflict", MPTCP_MIB_RCVWNDCONFLICT),
 	SNMP_MIB_ITEM("MPCurrEstab", MPTCP_MIB_CURRESTAB),
 	SNMP_MIB_ITEM("Blackhole", MPTCP_MIB_BLACKHOLE),
+	SNMP_MIB_ITEM("DataFallback", MPTCP_MIB_DATA_FALLBACK),
+	SNMP_MIB_ITEM("MD5sumFallback", MPTCP_MIB_MD5SUM_FALLBACK),
+	SNMP_MIB_ITEM("DssFallback", MPTCP_MIB_DSS_FALLBACK),
+	SNMP_MIB_ITEM("SimultFallback", MPTCP_MIB_SIMULT_FALLBACK),
+	SNMP_MIB_ITEM("FallbackFailed", MPTCP_MIB_FALLBACK_FAILED),
 	SNMP_MIB_SENTINEL
 };
 
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 250c6b77977e..99c8788694c3 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -81,6 +81,13 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_RCVWNDCONFLICT,	/* Conflict with while updating msk rcv wnd */
 	MPTCP_MIB_CURRESTAB,		/* Current established MPTCP connections */
 	MPTCP_MIB_BLACKHOLE,		/* A blackhole has been detected */
+	MPTCP_MIB_DATA_FALLBACK,	/* Missing DSS/MPC+data on first
+					 * established packet
+					 */
+	MPTCP_MIB_MD5SUM_FALLBACK,	/* Conflicting TCP option enabled */
+	MPTCP_MIB_DSS_FALLBACK,		/* Bad or missing DSS */
+	MPTCP_MIB_SIMULT_FALLBACK,	/* Simultaneous connect */
+	MPTCP_MIB_FALLBACK_FAILED,	/* Can't fallback due to msk status */
 	__MPTCP_MIB_MAX
 };
 
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 4680d980e605..c9e1f3ec3772 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -978,7 +978,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 		if (subflow->mp_join)
 			goto reset;
 		subflow->mp_capable = 0;
-		if (mptcp_do_fallback(ssk))
+		if (mptcp_do_fallback(ssk, MPTCP_MIB_DATA_FALLBACK))
 			goto reset;
 		pr_fallback(msk);
 		return false;
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0dc55a17f274..5f99f02c827c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -67,6 +67,27 @@ static const struct proto_ops *mptcp_fallback_tcp_ops(const struct sock *sk)
 	return &inet_stream_ops;
 }
 
+bool __mptcp_do_fallback(struct mptcp_sock *msk, int fb_mib)
+{
+	struct net *net = sock_net((struct sock *)msk);
+
+	if (__mptcp_check_fallback(msk))
+		return true;
+
+	spin_lock_bh(&msk->fallback_lock);
+	if (!msk->allow_infinite_fallback) {
+		__MPTCP_INC_STATS(net, MPTCP_MIB_SIMULT_FALLBACK);
+		spin_unlock_bh(&msk->fallback_lock);
+		return false;
+	}
+
+	msk->allow_subflows = false;
+	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
+	__MPTCP_INC_STATS(net, fb_mib);
+	spin_unlock_bh(&msk->fallback_lock);
+	return true;
+}
+
 static int __mptcp_socket_create(struct mptcp_sock *msk)
 {
 	struct mptcp_subflow_context *subflow;
@@ -560,10 +581,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
 
 static void mptcp_dss_corruption(struct mptcp_sock *msk, struct sock *ssk)
 {
-	if (mptcp_do_fallback(ssk)) {
-		MPTCP_INC_STATS(sock_net(ssk),
-				MPTCP_MIB_DSSCORRUPTIONFALLBACK);
-	} else {
+	if (!mptcp_do_fallback(ssk, MPTCP_MIB_DSSCORRUPTIONFALLBACK)) {
 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSCORRUPTIONRESET);
 		mptcp_subflow_reset(ssk);
 	}
@@ -1142,12 +1160,11 @@ static void mptcp_update_infinite_map(struct mptcp_sock *msk,
 	mpext->infinite_map = 1;
 	mpext->data_len = 0;
 
-	if (!mptcp_do_fallback(ssk)) {
+	if (!mptcp_do_fallback(ssk, MPTCP_MIB_INFINITEMAPTX)) {
 		mptcp_subflow_reset(ssk);
 		return;
 	}
 
-	MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPTX);
 	mptcp_subflow_ctx(ssk)->send_infinite_map = 0;
 	pr_fallback(msk);
 }
@@ -3717,16 +3734,15 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	 * TCP option space.
 	 */
 	if (rcu_access_pointer(tcp_sk(ssk)->md5sig_info))
-		mptcp_subflow_early_fallback(msk, subflow);
+		mptcp_early_fallback(msk, subflow, MPTCP_MIB_MD5SUM_FALLBACK);
 #endif
 	if (subflow->request_mptcp) {
-		if (mptcp_active_should_disable(sk)) {
-			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_MPCAPABLEACTIVEDISABLED);
-			mptcp_subflow_early_fallback(msk, subflow);
-		} else if (mptcp_token_new_connect(ssk) < 0) {
-			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_TOKENFALLBACKINIT);
-			mptcp_subflow_early_fallback(msk, subflow);
-		}
+		if (mptcp_active_should_disable(sk))
+			mptcp_early_fallback(msk, subflow,
+					    MPTCP_MIB_MPCAPABLEACTIVEDISABLED);
+		else if (mptcp_token_new_connect(ssk) < 0)
+			mptcp_early_fallback(msk, subflow,
+					     MPTCP_MIB_TOKENFALLBACKINIT);
 	}
 
 	WRITE_ONCE(msk->write_seq, subflow->idsn);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 34c0863f1eae..ceb2f2ba2cc7 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1220,24 +1220,6 @@ static inline bool mptcp_check_fallback(const struct sock *sk)
 	return __mptcp_check_fallback(msk);
 }
 
-static inline bool __mptcp_do_fallback(struct mptcp_sock *msk)
-{
-	if (__mptcp_check_fallback(msk)) {
-		pr_debug("TCP fallback already done (msk=%p)\n", msk);
-		return true;
-	}
-	spin_lock_bh(&msk->fallback_lock);
-	if (!msk->allow_infinite_fallback) {
-		spin_unlock_bh(&msk->fallback_lock);
-		return false;
-	}
-
-	msk->allow_subflows = false;
-	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
-	spin_unlock_bh(&msk->fallback_lock);
-	return true;
-}
-
 static inline bool __mptcp_has_initial_subflow(const struct mptcp_sock *msk)
 {
 	struct sock *ssk = READ_ONCE(msk->first);
@@ -1247,14 +1229,16 @@ static inline bool __mptcp_has_initial_subflow(const struct mptcp_sock *msk)
 			TCPF_SYN_RECV | TCPF_LISTEN));
 }
 
-static inline bool mptcp_do_fallback(struct sock *ssk)
+bool __mptcp_do_fallback(struct mptcp_sock *msk, int fb_mib);
+
+static inline bool mptcp_do_fallback(struct sock *ssk, int fb_mib)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	struct sock *sk = subflow->conn;
 	struct mptcp_sock *msk;
 
 	msk = mptcp_sk(sk);
-	if (!__mptcp_do_fallback(msk))
+	if (!__mptcp_do_fallback(msk, fb_mib))
 		return false;
 	if (READ_ONCE(msk->snd_data_fin_enable) && !(ssk->sk_shutdown & SEND_SHUTDOWN)) {
 		gfp_t saved_allocation = ssk->sk_allocation;
@@ -1272,12 +1256,13 @@ static inline bool mptcp_do_fallback(struct sock *ssk)
 
 #define pr_fallback(a) pr_debug("%s:fallback to TCP (msk=%p)\n", __func__, a)
 
-static inline void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
-						struct mptcp_subflow_context *subflow)
+static inline void mptcp_early_fallback(struct mptcp_sock *msk,
+					struct mptcp_subflow_context *subflow,
+					int fb_mib)
 {
 	pr_fallback(msk);
 	subflow->request_mptcp = 0;
-	__mptcp_do_fallback(msk);
+	__mptcp_do_fallback(msk, fb_mib);
 }
 
 static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 11c5b042c2e1..e140d032eb3e 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -544,11 +544,10 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 	mptcp_get_options(skb, &mp_opt);
 	if (subflow->request_mptcp) {
 		if (!(mp_opt.suboptions & OPTION_MPTCP_MPC_SYNACK)) {
-			if (!mptcp_do_fallback(sk))
+			if (!mptcp_do_fallback(sk,
+					    MPTCP_MIB_MPCAPABLEACTIVEFALLBACK))
 				goto do_reset;
 
-			MPTCP_INC_STATS(sock_net(sk),
-					MPTCP_MIB_MPCAPABLEACTIVEFALLBACK);
 			pr_fallback(msk);
 			goto fallback;
 		}
@@ -1406,7 +1405,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 			return true;
 		}
 
-		if (!mptcp_do_fallback(ssk)) {
+		if (!mptcp_do_fallback(ssk, MPTCP_MIB_DSS_FALLBACK)) {
 			/* fatal protocol error, close the socket.
 			 * subflow_error_report() will introduce the appropriate barriers
 			 */
@@ -1424,7 +1423,6 @@ static bool subflow_check_data_avail(struct sock *ssk)
 			WRITE_ONCE(subflow->data_avail, false);
 			return false;
 		}
-
 	}
 
 	skb = skb_peek(&ssk->sk_receive_queue);
@@ -1860,7 +1858,7 @@ static void subflow_state_change(struct sock *sk)
 
 	msk = mptcp_sk(parent);
 	if (subflow_simultaneous_connect(sk)) {
-		mptcp_do_fallback(sk);
+		mptcp_do_fallback(sk, MPTCP_MIB_SIMULT_FALLBACK);
 		pr_fallback(msk);
 		subflow->conn_finished = 1;
 		mptcp_propagate_state(parent, sk, subflow, NULL);
-- 
2.49.0

Hi Paolo,

On 05/07/2025 09:24, Paolo Abeni wrote:
> Add the mibs required to cover the few possible fallback causes still
> lacking suck info.
> 
> Move the relevant mib increment into the fallback helper, so that no
> eventual future fallback operation will miss a paired mib increment.
> 
> Additionally track failed fallback via its own mib.

Good idea!

> While at the above, rename an existing helper to reduce long lines
> problems all along.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> still a couple of long lines I could not trim

That's OK. My understanding is that we should try to have lines under 80
chars, but when this condition makes the code harder to read, it is fine
to go over.

> ---
>  net/mptcp/ctrl.c     |  4 ++--
>  net/mptcp/mib.c      |  5 +++++
>  net/mptcp/mib.h      |  7 +++++++
>  net/mptcp/options.c  |  2 +-
>  net/mptcp/protocol.c | 44 ++++++++++++++++++++++++++++++--------------
>  net/mptcp/protocol.h | 31 ++++++++-----------------------
>  net/mptcp/subflow.c  | 10 ++++------
>  7 files changed, 57 insertions(+), 46 deletions(-)
> 
> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index d9290c5bb6c7..fed40dae5583 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
> @@ -533,9 +533,9 @@ void mptcp_active_detect_blackhole(struct sock *ssk, bool expired)
>  	to_max = mptcp_get_pernet(net)->syn_retrans_before_tcp_fallback;
>  
>  	if (timeouts == to_max || (timeouts < to_max && expired)) {
> -		MPTCP_INC_STATS(net, MPTCP_MIB_MPCAPABLEACTIVEDROP);
>  		subflow->mpc_drop = 1;
> -		mptcp_subflow_early_fallback(mptcp_sk(subflow->conn), subflow);
> +		mptcp_early_fallback(mptcp_sk(subflow->conn), subflow,
> +				     MPTCP_MIB_MPCAPABLEACTIVEDROP);
>  	}
>  }
>  
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index 0c24545f0e8d..064f5eaf0595 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -80,6 +80,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>  	SNMP_MIB_ITEM("RcvWndConflict", MPTCP_MIB_RCVWNDCONFLICT),
>  	SNMP_MIB_ITEM("MPCurrEstab", MPTCP_MIB_CURRESTAB),
>  	SNMP_MIB_ITEM("Blackhole", MPTCP_MIB_BLACKHOLE),
> +	SNMP_MIB_ITEM("DataFallback", MPTCP_MIB_DATA_FALLBACK),

Should it not be prefixed MPCapableFallback like others above?

Maybe: MPCapableFallbackData / MPTCP_MIB_MPCAPABLEDATAFALLBACK?

> +	SNMP_MIB_ITEM("MD5sumFallback", MPTCP_MIB_MD5SUM_FALLBACK),

For the name: is it limited to MD5Sum? Or any TCP AO?

> +	SNMP_MIB_ITEM("DssFallback", MPTCP_MIB_DSS_FALLBACK),
> +	SNMP_MIB_ITEM("SimultFallback", MPTCP_MIB_SIMULT_FALLBACK),
> +	SNMP_MIB_ITEM("FallbackFailed", MPTCP_MIB_FALLBACK_FAILED),

Do you think it would be worth it to check in the selftests
(mptcp_join.sh?) if any of these counters are incremented when they
should not be?

>  	SNMP_MIB_SENTINEL
>  };
>  
> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
> index 250c6b77977e..99c8788694c3 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -81,6 +81,13 @@ enum linux_mptcp_mib_field {
>  	MPTCP_MIB_RCVWNDCONFLICT,	/* Conflict with while updating msk rcv wnd */
>  	MPTCP_MIB_CURRESTAB,		/* Current established MPTCP connections */
>  	MPTCP_MIB_BLACKHOLE,		/* A blackhole has been detected */
> +	MPTCP_MIB_DATA_FALLBACK,	/* Missing DSS/MPC+data on first
> +					 * established packet
> +					 */
> +	MPTCP_MIB_MD5SUM_FALLBACK,	/* Conflicting TCP option enabled */
> +	MPTCP_MIB_DSS_FALLBACK,		/* Bad or missing DSS */
> +	MPTCP_MIB_SIMULT_FALLBACK,	/* Simultaneous connect */

Maybe clearer with CONNECT or CONN in the name?

Maybe: SimultConnectFallback / MPTCP_MIB_SIMULT_CONN_FALLBACK?

> +	MPTCP_MIB_FALLBACK_FAILED,	/* Can't fallback due to msk status */
>  	__MPTCP_MIB_MAX
>  };
>  
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 4680d980e605..c9e1f3ec3772 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -978,7 +978,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>  		if (subflow->mp_join)
>  			goto reset;
>  		subflow->mp_capable = 0;
> -		if (mptcp_do_fallback(ssk))
> +		if (mptcp_do_fallback(ssk, MPTCP_MIB_DATA_FALLBACK))
>  			goto reset;
>  		pr_fallback(msk);
>  		return false;
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0dc55a17f274..5f99f02c827c 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -67,6 +67,27 @@ static const struct proto_ops *mptcp_fallback_tcp_ops(const struct sock *sk)
>  	return &inet_stream_ops;
>  }
>  
> +bool __mptcp_do_fallback(struct mptcp_sock *msk, int fb_mib)
> +{
> +	struct net *net = sock_net((struct sock *)msk);
> +
> +	if (__mptcp_check_fallback(msk))
> +		return true;
> +
> +	spin_lock_bh(&msk->fallback_lock);
> +	if (!msk->allow_infinite_fallback) {
> +		__MPTCP_INC_STATS(net, MPTCP_MIB_SIMULT_FALLBACK);

Is the name OK? To me, a counter with this name should be incremented
when __mptcp_check_fallback() returns true just above, no?

Here, we now try to do a fallback, but we cannot, a reset is probably
needed instead, but that's not because we try to do the same fallback in
parallel, no?

I guess you wanted to use MPTCP_MIB_FALLBACK_FAILED, no?

Anyway, even with 'fallback failed', because in the first patch, the
helper is now also used to check if a fallback can happen. It means that
here we can be in a "normal" situation where we just want to fallback if
we can. If not, that's not an error.

In other words, I don't think we can have a generic MIB here. If we do,
this counter will be incremented in parallel to one linked to a subflow
reset, e.g. MPTCP_MIB_DSSCORRUPTIONRESET. I wonder if (1) this helper
should not be renamed to avoid such confusion -- see patch 1 -- and if
(2) another argument should not be passed to increment another counter
linked to a subflow reset or an abort (MPTCP_MIB_FALLBACK_FAILED can be
passed if that's not normal). Or maybe clearer to add one new helper to
do a fallback or a reset + increment the right MIB counter depending on
the situation? (And another helper to do a fallback if possible, and if
not, only increment MPTCP_MIB_FALLBACK_FAILED?)

(also, detail: why not doing that after the spin_unlock?)

> +		spin_unlock_bh(&msk->fallback_lock);
> +		return false;
> +	}
> +
> +	msk->allow_subflows = false;
> +	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
> +	__MPTCP_INC_STATS(net, fb_mib);

(same here, why not doing that after the spin_unlock?)

> +	spin_unlock_bh(&msk->fallback_lock);
> +	return true;
> +}
> +
>  static int __mptcp_socket_create(struct mptcp_sock *msk)
>  {
>  	struct mptcp_subflow_context *subflow;
> @@ -560,10 +581,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
>  
>  static void mptcp_dss_corruption(struct mptcp_sock *msk, struct sock *ssk)
>  {
> -	if (mptcp_do_fallback(ssk)) {
> -		MPTCP_INC_STATS(sock_net(ssk),
> -				MPTCP_MIB_DSSCORRUPTIONFALLBACK);
> -	} else {
> +	if (!mptcp_do_fallback(ssk, MPTCP_MIB_DSSCORRUPTIONFALLBACK)) {
>  		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSCORRUPTIONRESET);
>  		mptcp_subflow_reset(ssk);
>  	}
> @@ -1142,12 +1160,11 @@ static void mptcp_update_infinite_map(struct mptcp_sock *msk,
>  	mpext->infinite_map = 1;
>  	mpext->data_len = 0;
>  
> -	if (!mptcp_do_fallback(ssk)) {
> +	if (!mptcp_do_fallback(ssk, MPTCP_MIB_INFINITEMAPTX)) {
>  		mptcp_subflow_reset(ssk);
>  		return;
>  	}
>  
> -	MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPTX);
>  	mptcp_subflow_ctx(ssk)->send_infinite_map = 0;
>  	pr_fallback(msk);
>  }
> @@ -3717,16 +3734,15 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>  	 * TCP option space.
>  	 */
>  	if (rcu_access_pointer(tcp_sk(ssk)->md5sig_info))
> -		mptcp_subflow_early_fallback(msk, subflow);
> +		mptcp_early_fallback(msk, subflow, MPTCP_MIB_MD5SUM_FALLBACK);
>  #endif
>  	if (subflow->request_mptcp) {
> -		if (mptcp_active_should_disable(sk)) {
> -			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_MPCAPABLEACTIVEDISABLED);
> -			mptcp_subflow_early_fallback(msk, subflow);
> -		} else if (mptcp_token_new_connect(ssk) < 0) {
> -			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_TOKENFALLBACKINIT);
> -			mptcp_subflow_early_fallback(msk, subflow);
> -		}
> +		if (mptcp_active_should_disable(sk))
> +			mptcp_early_fallback(msk, subflow,
> +					    MPTCP_MIB_MPCAPABLEACTIVEDISABLED);
> +		else if (mptcp_token_new_connect(ssk) < 0)
> +			mptcp_early_fallback(msk, subflow,
> +					     MPTCP_MIB_TOKENFALLBACKINIT);
>  	}
>  
>  	WRITE_ONCE(msk->write_seq, subflow->idsn);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 34c0863f1eae..ceb2f2ba2cc7 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1220,24 +1220,6 @@ static inline bool mptcp_check_fallback(const struct sock *sk)
>  	return __mptcp_check_fallback(msk);
>  }
>  
> -static inline bool __mptcp_do_fallback(struct mptcp_sock *msk)
> -{
> -	if (__mptcp_check_fallback(msk)) {
> -		pr_debug("TCP fallback already done (msk=%p)\n", msk);
> -		return true;
> -	}
> -	spin_lock_bh(&msk->fallback_lock);
> -	if (!msk->allow_infinite_fallback) {
> -		spin_unlock_bh(&msk->fallback_lock);
> -		return false;
> -	}
> -
> -	msk->allow_subflows = false;
> -	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
> -	spin_unlock_bh(&msk->fallback_lock);
> -	return true;
> -}
> -
>  static inline bool __mptcp_has_initial_subflow(const struct mptcp_sock *msk)
>  {
>  	struct sock *ssk = READ_ONCE(msk->first);
> @@ -1247,14 +1229,16 @@ static inline bool __mptcp_has_initial_subflow(const struct mptcp_sock *msk)
>  			TCPF_SYN_RECV | TCPF_LISTEN));
>  }
>  
> -static inline bool mptcp_do_fallback(struct sock *ssk)
> +bool __mptcp_do_fallback(struct mptcp_sock *msk, int fb_mib);
> +
> +static inline bool mptcp_do_fallback(struct sock *ssk, int fb_mib)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>  	struct sock *sk = subflow->conn;
>  	struct mptcp_sock *msk;
>  
>  	msk = mptcp_sk(sk);
> -	if (!__mptcp_do_fallback(msk))
> +	if (!__mptcp_do_fallback(msk, fb_mib))
>  		return false;
>  	if (READ_ONCE(msk->snd_data_fin_enable) && !(ssk->sk_shutdown & SEND_SHUTDOWN)) {
>  		gfp_t saved_allocation = ssk->sk_allocation;
> @@ -1272,12 +1256,13 @@ static inline bool mptcp_do_fallback(struct sock *ssk)
>  
>  #define pr_fallback(a) pr_debug("%s:fallback to TCP (msk=%p)\n", __func__, a)
>  
> -static inline void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> -						struct mptcp_subflow_context *subflow)
> +static inline void mptcp_early_fallback(struct mptcp_sock *msk,
> +					struct mptcp_subflow_context *subflow,
> +					int fb_mib)
>  {
>  	pr_fallback(msk);
>  	subflow->request_mptcp = 0;
> -	__mptcp_do_fallback(msk);
> +	__mptcp_do_fallback(msk, fb_mib);
>  }
>  
>  static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 11c5b042c2e1..e140d032eb3e 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -544,11 +544,10 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>  	mptcp_get_options(skb, &mp_opt);
>  	if (subflow->request_mptcp) {
>  		if (!(mp_opt.suboptions & OPTION_MPTCP_MPC_SYNACK)) {
> -			if (!mptcp_do_fallback(sk))
> +			if (!mptcp_do_fallback(sk,
> +					    MPTCP_MIB_MPCAPABLEACTIVEFALLBACK))
>  				goto do_reset;
>  
> -			MPTCP_INC_STATS(sock_net(sk),
> -					MPTCP_MIB_MPCAPABLEACTIVEFALLBACK);
>  			pr_fallback(msk);
>  			goto fallback;
>  		}
> @@ -1406,7 +1405,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  			return true;
>  		}
>  
> -		if (!mptcp_do_fallback(ssk)) {
> +		if (!mptcp_do_fallback(ssk, MPTCP_MIB_DSS_FALLBACK)) {
>  			/* fatal protocol error, close the socket.
>  			 * subflow_error_report() will introduce the appropriate barriers
>  			 */
> @@ -1424,7 +1423,6 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  			WRITE_ONCE(subflow->data_avail, false);
>  			return false;
>  		}
> -

Maybe better to remove this line in patch 1/5?

>  	}
>  
>  	skb = skb_peek(&ssk->sk_receive_queue);
> @@ -1860,7 +1858,7 @@ static void subflow_state_change(struct sock *sk)
>  
>  	msk = mptcp_sk(parent);
>  	if (subflow_simultaneous_connect(sk)) {
> -		mptcp_do_fallback(sk);
> +		mptcp_do_fallback(sk, MPTCP_MIB_SIMULT_FALLBACK);
>  		pr_fallback(msk);
>  		subflow->conn_finished = 1;
>  		mptcp_propagate_state(parent, sk, subflow, NULL);

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

On 7/8/25 5:56 PM, Matthieu Baerts wrote:
> On 05/07/2025 09:24, Paolo Abeni wrote:
>> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
>> index 0c24545f0e8d..064f5eaf0595 100644
>> --- a/net/mptcp/mib.c
>> +++ b/net/mptcp/mib.c
>> @@ -80,6 +80,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>>  	SNMP_MIB_ITEM("RcvWndConflict", MPTCP_MIB_RCVWNDCONFLICT),
>>  	SNMP_MIB_ITEM("MPCurrEstab", MPTCP_MIB_CURRESTAB),
>>  	SNMP_MIB_ITEM("Blackhole", MPTCP_MIB_BLACKHOLE),
>> +	SNMP_MIB_ITEM("DataFallback", MPTCP_MIB_DATA_FALLBACK),
> 
> Should it not be prefixed MPCapableFallback like others above?
> 
> Maybe: MPCapableFallbackData / MPTCP_MIB_MPCAPABLEDATAFALLBACK?

Fine by me

> 
>> +	SNMP_MIB_ITEM("MD5sumFallback", MPTCP_MIB_MD5SUM_FALLBACK),
> 
> For the name: is it limited to MD5Sum? Or any TCP AO?

To be more accurate should be MD5_SIG_FALLBACK, or just MD5_FALLBACK

> 
>> +	SNMP_MIB_ITEM("DssFallback", MPTCP_MIB_DSS_FALLBACK),
>> +	SNMP_MIB_ITEM("SimultFallback", MPTCP_MIB_SIMULT_FALLBACK),
>> +	SNMP_MIB_ITEM("FallbackFailed", MPTCP_MIB_FALLBACK_FAILED),
> 
> Do you think it would be worth it to check in the selftests
> (mptcp_join.sh?) if any of these counters are incremented when they
> should not be?

Could be a nice follow-up! (which I don't plan ATM).

>>  	SNMP_MIB_SENTINEL
>>  };
>>  
>> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
>> index 250c6b77977e..99c8788694c3 100644
>> --- a/net/mptcp/mib.h
>> +++ b/net/mptcp/mib.h
>> @@ -81,6 +81,13 @@ enum linux_mptcp_mib_field {
>>  	MPTCP_MIB_RCVWNDCONFLICT,	/* Conflict with while updating msk rcv wnd */
>>  	MPTCP_MIB_CURRESTAB,		/* Current established MPTCP connections */
>>  	MPTCP_MIB_BLACKHOLE,		/* A blackhole has been detected */
>> +	MPTCP_MIB_DATA_FALLBACK,	/* Missing DSS/MPC+data on first
>> +					 * established packet
>> +					 */
>> +	MPTCP_MIB_MD5SUM_FALLBACK,	/* Conflicting TCP option enabled */
>> +	MPTCP_MIB_DSS_FALLBACK,		/* Bad or missing DSS */
>> +	MPTCP_MIB_SIMULT_FALLBACK,	/* Simultaneous connect */
> 
> Maybe clearer with CONNECT or CONN in the name?
> 
> Maybe: SimultConnectFallback / MPTCP_MIB_SIMULT_CONN_FALLBACK?

Fine by me.

> 
>> +	MPTCP_MIB_FALLBACK_FAILED,	/* Can't fallback due to msk status */
>>  	__MPTCP_MIB_MAX
>>  };
>>  
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 4680d980e605..c9e1f3ec3772 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -978,7 +978,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>>  		if (subflow->mp_join)
>>  			goto reset;
>>  		subflow->mp_capable = 0;
>> -		if (mptcp_do_fallback(ssk))
>> +		if (mptcp_do_fallback(ssk, MPTCP_MIB_DATA_FALLBACK))
>>  			goto reset;
>>  		pr_fallback(msk);
>>  		return false;
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 0dc55a17f274..5f99f02c827c 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -67,6 +67,27 @@ static const struct proto_ops *mptcp_fallback_tcp_ops(const struct sock *sk)
>>  	return &inet_stream_ops;
>>  }
>>  
>> +bool __mptcp_do_fallback(struct mptcp_sock *msk, int fb_mib)
>> +{
>> +	struct net *net = sock_net((struct sock *)msk);
>> +
>> +	if (__mptcp_check_fallback(msk))
>> +		return true;
>> +
>> +	spin_lock_bh(&msk->fallback_lock);
>> +	if (!msk->allow_infinite_fallback) {
>> +		__MPTCP_INC_STATS(net, MPTCP_MIB_SIMULT_FALLBACK);
> 
> Is the name OK? To me, a counter with this name should be incremented
> when __mptcp_check_fallback() returns true just above, no?

Typo above should be MPTCP_MIB_FALLBACK_FAILED.

> Here, we now try to do a fallback, but we cannot, a reset is probably
> needed instead, but that's not because we try to do the same fallback in
> parallel, no?
> 
> I guess you wanted to use MPTCP_MIB_FALLBACK_FAILED, no?
> 
> Anyway, even with 'fallback failed', because in the first patch, the
> helper is now also used to check if a fallback can happen. It means that
> here we can be in a "normal" situation where we just want to fallback if
> we can. If not, that's not an error.

Modulo bugs it's either:

- mptcp_do_fallback() returns false and we reset the subflow
- It's an early fallback and mptcp_do_fallback() should not return
false. Corrently the code does not even check for `false` return value
in this case, but we could/should add a WARN.

In any case it's not an error, but it's a fallback failure: the code
path should have caused a fallback, but instead the socket has been
reset. I think we should track the exceptional event with a MIB, and
FALLBACK_FAILED still sounds like a valid name to me.

> In other words, I don't think we can have a generic MIB here. If we do,
> this counter will be incremented in parallel to one linked to a subflow
> reset, e.g. MPTCP_MIB_DSSCORRUPTIONRESET.

AFAICS some reset i.e. in option.c and in subflow_finish_connect() don't
get their own MIB incremented. Even what that happens I think it's good:
having more mibs incremented allow understanding what happened exactly.
Not that there are already other exceptional events where multiple mibs
are incremented (IIRC even in the MPTCP code).

I wonder if (1) this helper
> should not be renamed to avoid such confusion -- see patch 1 -- and if
> (2) another argument should not be passed to increment another counter
> linked to a subflow reset or an abort (MPTCP_MIB_FALLBACK_FAILED can be
> passed if that's not normal). Or maybe clearer to add one new helper to
> do a fallback or a reset + increment the right MIB counter depending on
> the situation? (And another helper to do a fallback if possible, and if
> not, only increment MPTCP_MIB_FALLBACK_FAILED?)

I would not over-complicate the code here. The reset depends on the
caller, having that separate AFAICS makes the code simpler.

I'm ok to rename [__]mptcp_do_fallback() to [__]mptcp_try_fallback(),
but I would not do that in patch 1, for backport's sake.

> (also, detail: why not doing that after the spin_unlock?)

So we don't need to lock/unlock BH again to update the MIB>
  			 */
>> @@ -1424,7 +1423,6 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>  			WRITE_ONCE(subflow->data_avail, false);
>>  			return false;
>>  		}
>> -
> 
> Maybe better to remove this line in patch 1/5?

Fine by me.

/P

On 7/9/25 11:55 AM, Paolo Abeni wrote:
> I'm ok to rename [__]mptcp_do_fallback() to [__]mptcp_try_fallback(),
> but I would not do that in patch 1, for backport's sake.

Actually patch 1 already touches almost all the the places where
[__]mptcp_do_fallback is used, so it should be fine to do the rename is
such patch.

For consistency with other MIBs I'll additionally trim all the '_' in
the MPTCP-specific part of the newly introduced MIB name i.e.

MPTCP_MIB_FALLBACK_FAILED -> MPTCP_MIB_FALLBACKFAILED

/P

On 09/07/2025 12:23, Paolo Abeni wrote:
> On 7/9/25 11:55 AM, Paolo Abeni wrote:
>> I'm ok to rename [__]mptcp_do_fallback() to [__]mptcp_try_fallback(),
>> but I would not do that in patch 1, for backport's sake.
> 
> Actually patch 1 already touches almost all the the places where
> [__]mptcp_do_fallback is used, so it should be fine to do the rename is
> such patch.

Great, thank you!

Just to come back on your "for backport's sake": if the behaviour of a
function is modified, I think it is safer to rename it → so during the
backports, we can easily find extra calls that are only in stable
versions, but not in the dev branch, and do extra adaptations if needed.

> For consistency with other MIBs I'll additionally trim all the '_' in
> the MPTCP-specific part of the newly introduced MIB name i.e.
> 
> MPTCP_MIB_FALLBACK_FAILED -> MPTCP_MIB_FALLBACKFAILED

Good point!
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

On 09/07/2025 11:55, Paolo Abeni wrote:
> On 7/8/25 5:56 PM, Matthieu Baerts wrote:
>> On 05/07/2025 09:24, Paolo Abeni wrote:
>>> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
>>> index 0c24545f0e8d..064f5eaf0595 100644
>>> --- a/net/mptcp/mib.c
>>> +++ b/net/mptcp/mib.c
>>> @@ -80,6 +80,11 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>>>  	SNMP_MIB_ITEM("RcvWndConflict", MPTCP_MIB_RCVWNDCONFLICT),
>>>  	SNMP_MIB_ITEM("MPCurrEstab", MPTCP_MIB_CURRESTAB),
>>>  	SNMP_MIB_ITEM("Blackhole", MPTCP_MIB_BLACKHOLE),
>>> +	SNMP_MIB_ITEM("DataFallback", MPTCP_MIB_DATA_FALLBACK),
>>
>> Should it not be prefixed MPCapableFallback like others above?
>>
>> Maybe: MPCapableFallbackData / MPTCP_MIB_MPCAPABLEDATAFALLBACK?
> 
> Fine by me
> 
>>
>>> +	SNMP_MIB_ITEM("MD5sumFallback", MPTCP_MIB_MD5SUM_FALLBACK),
>>
>> For the name: is it limited to MD5Sum? Or any TCP AO?
> 
> To be more accurate should be MD5_SIG_FALLBACK, or just MD5_FALLBACK

Do we fallback to TCP when TCP AO is used? Or maybe it can be used in
parallel? I don't remember.

>>> +	SNMP_MIB_ITEM("DssFallback", MPTCP_MIB_DSS_FALLBACK),
>>> +	SNMP_MIB_ITEM("SimultFallback", MPTCP_MIB_SIMULT_FALLBACK),
>>> +	SNMP_MIB_ITEM("FallbackFailed", MPTCP_MIB_FALLBACK_FAILED),
>>
>> Do you think it would be worth it to check in the selftests
>> (mptcp_join.sh?) if any of these counters are incremented when they
>> should not be?
> 
> Could be a nice follow-up! (which I don't plan ATM).

I just created this:

  https://github.com/multipath-tcp/mptcp_net-next/issues/571


>>>  	SNMP_MIB_SENTINEL
>>>  };
>>>  
>>> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
>>> index 250c6b77977e..99c8788694c3 100644
>>> --- a/net/mptcp/mib.h
>>> +++ b/net/mptcp/mib.h
>>> @@ -81,6 +81,13 @@ enum linux_mptcp_mib_field {
>>>  	MPTCP_MIB_RCVWNDCONFLICT,	/* Conflict with while updating msk rcv wnd */
>>>  	MPTCP_MIB_CURRESTAB,		/* Current established MPTCP connections */
>>>  	MPTCP_MIB_BLACKHOLE,		/* A blackhole has been detected */
>>> +	MPTCP_MIB_DATA_FALLBACK,	/* Missing DSS/MPC+data on first
>>> +					 * established packet
>>> +					 */
>>> +	MPTCP_MIB_MD5SUM_FALLBACK,	/* Conflicting TCP option enabled */
>>> +	MPTCP_MIB_DSS_FALLBACK,		/* Bad or missing DSS */
>>> +	MPTCP_MIB_SIMULT_FALLBACK,	/* Simultaneous connect */
>>
>> Maybe clearer with CONNECT or CONN in the name?
>>
>> Maybe: SimultConnectFallback / MPTCP_MIB_SIMULT_CONN_FALLBACK?
> 
> Fine by me.
> 
>>
>>> +	MPTCP_MIB_FALLBACK_FAILED,	/* Can't fallback due to msk status */
>>>  	__MPTCP_MIB_MAX
>>>  };
>>>  
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index 4680d980e605..c9e1f3ec3772 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -978,7 +978,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>>>  		if (subflow->mp_join)
>>>  			goto reset;
>>>  		subflow->mp_capable = 0;
>>> -		if (mptcp_do_fallback(ssk))
>>> +		if (mptcp_do_fallback(ssk, MPTCP_MIB_DATA_FALLBACK))
>>>  			goto reset;
>>>  		pr_fallback(msk);
>>>  		return false;
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 0dc55a17f274..5f99f02c827c 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -67,6 +67,27 @@ static const struct proto_ops *mptcp_fallback_tcp_ops(const struct sock *sk)
>>>  	return &inet_stream_ops;
>>>  }
>>>  
>>> +bool __mptcp_do_fallback(struct mptcp_sock *msk, int fb_mib)
>>> +{
>>> +	struct net *net = sock_net((struct sock *)msk);
>>> +
>>> +	if (__mptcp_check_fallback(msk))
>>> +		return true;
>>> +
>>> +	spin_lock_bh(&msk->fallback_lock);
>>> +	if (!msk->allow_infinite_fallback) {
>>> +		__MPTCP_INC_STATS(net, MPTCP_MIB_SIMULT_FALLBACK);
>>
>> Is the name OK? To me, a counter with this name should be incremented
>> when __mptcp_check_fallback() returns true just above, no?
> 
> Typo above should be MPTCP_MIB_FALLBACK_FAILED.
> 
>> Here, we now try to do a fallback, but we cannot, a reset is probably
>> needed instead, but that's not because we try to do the same fallback in
>> parallel, no?
>>
>> I guess you wanted to use MPTCP_MIB_FALLBACK_FAILED, no?
>>
>> Anyway, even with 'fallback failed', because in the first patch, the
>> helper is now also used to check if a fallback can happen. It means that
>> here we can be in a "normal" situation where we just want to fallback if
>> we can. If not, that's not an error.
> 
> Modulo bugs it's either:
> 
> - mptcp_do_fallback() returns false and we reset the subflow
> - It's an early fallback and mptcp_do_fallback() should not return
> false. Corrently the code does not even check for `false` return value
> in this case, but we could/should add a WARN.

Good idea :)
> In any case it's not an error, but it's a fallback failure: the code
> path should have caused a fallback, but instead the socket has been
> reset. I think we should track the exceptional event with a MIB, and
> FALLBACK_FAILED still sounds like a valid name to me.

I don't think it is always an error, see when you set
MPTCP_MIB_DSSCORRUPTIONFALLBACK (mptcp_dss_corruption) or
MPTCP_MIB_DSS_FALLBACK (subflow_check_data_avail), no?

>> In other words, I don't think we can have a generic MIB here. If we do,
>> this counter will be incremented in parallel to one linked to a subflow
>> reset, e.g. MPTCP_MIB_DSSCORRUPTIONRESET.
> 
> AFAICS some reset i.e. in option.c and in subflow_finish_connect() don't
> get their own MIB incremented. Even what that happens I think it's good:
> having more mibs incremented allow understanding what happened exactly.
> Not that there are already other exceptional events where multiple mibs
> are incremented (IIRC even in the MPTCP code).

Sure but it might be confusing to see that the "fallback failed" when it
was not supposed to even try to do a fallback at that stage of the
connection.

>> I wonder if (1) this helper
>> should not be renamed to avoid such confusion -- see patch 1 -- and if
>> (2) another argument should not be passed to increment another counter
>> linked to a subflow reset or an abort (MPTCP_MIB_FALLBACK_FAILED can be
>> passed if that's not normal). Or maybe clearer to add one new helper to
>> do a fallback or a reset + increment the right MIB counter depending on
>> the situation? (And another helper to do a fallback if possible, and if
>> not, only increment MPTCP_MIB_FALLBACK_FAILED?)
> 
> I would not over-complicate the code here. The reset depends on the
> caller, having that separate AFAICS makes the code simpler.

Fine by me.

> I'm ok to rename [__]mptcp_do_fallback() to [__]mptcp_try_fallback(),
> but I would not do that in patch 1, for backport's sake.
> 
>> (also, detail: why not doing that after the spin_unlock?)
> 
> So we don't need to lock/unlock BH again to update the MIB>

Ah yes, you use the "__" version of MPTCP_INC_STATS, missed that.

>   			 */
>>> @@ -1424,7 +1423,6 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>>  			WRITE_ONCE(subflow->data_avail, false);
>>>  			return false;
>>>  		}
>>> -
>>
>> Maybe better to remove this line in patch 1/5?
> 
> Fine by me.
> 
> /P
> 

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