Re: [PATCH] mptcp: add per-reason MIB counters for MPTCP_RST_EMPTCP resets

[Matthieu Baerts <matttbe@kernel.org>]

Hi Shardul,

(-cc: Netdev)

On 21/04/2026 11:56, Shardul Bankar wrote:
> MPTCP_RST_EMPTCP (reset reason 1) is used as a catch-all for seven
> distinct error conditions across subflow setup, authentication, and
> data path validation.  The existing MPRstTx/MPRstRx counters only
> track aggregate reset volume, making it difficult to diagnose which
> code path is triggering subflow resets in production.
> 
> Add per-reason MIB counters for each MPTCP_RST_EMPTCP use site:
> 
>   MPRstMD5SIG         - MD5SIG on listener, incompatible with MPTCP
>   MPRstNoToken        - JOIN token lookup failed, no matching conn
>   MPRstNoMPJ          - SYN/ACK missing MP_JOIN option
>   MPRstHMAC           - HMAC validation failed during subflow join
>   MPRstFatalFallback  - fatal fallback during child socket creation
>   MPRstBadMap         - invalid data mapping on established subflow
>   MPRstNotEstablished - JOIN on not-fully-established msk

When looking at the other MIB counters we have where RST are sent in
some cases, it might be better to link them to the event (e.g. MPJoin)
or the issue (DSS issue), rather than the action (MPRst) I think. See below.
> These counters are incremented alongside the existing reset_reason
> assignment and are visible via nstat as MPTcpExtMPRst*.  The
> aggregate MPRstTx/MPRstRx counters remain unchanged.

Do you mind also modifying the selftests in a separated patch to check
these counters (if available -- if not, they can be ignored), please?

e.g. by checking them in mptcp_join.sh, in chk_rst_nr(), assuming they
are all at 0 except set → check how chk_join_nr() is used where specific
errors need to be set explicitly:

  join_syn_rej=1 \
      chk_join_nr (...)

But I guess most/all these errors are not produced in the mptcp_join.sh
selftest, right?. But I guess some can be seen in packetdrill tests:


https://github.com/multipath-tcp/packetdrill/tree/mptcp-net-next/gtests/net/mptcp

It would be good to check these counters are incremented there when
expected, see how MPTcpExtMPCapableFallbackACK is checked for example.

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/511
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  net/mptcp/mib.c      |  7 +++++++
>  net/mptcp/mib.h      |  7 +++++++
>  net/mptcp/protocol.c |  1 +
>  net/mptcp/subflow.c  | 14 +++++++++++---
>  4 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index f23fda0c55a7..ec42583c59cf 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -71,6 +71,13 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>  	SNMP_MIB_ITEM("MPFastcloseRx", MPTCP_MIB_MPFASTCLOSERX),
>  	SNMP_MIB_ITEM("MPRstTx", MPTCP_MIB_MPRSTTX),
>  	SNMP_MIB_ITEM("MPRstRx", MPTCP_MIB_MPRSTRX),
> +	SNMP_MIB_ITEM("MPRstMD5SIG", MPTCP_MIB_MPRSTMD5SIG),

We already have MD5SigFallback, should we have MD5SigReset?

> +	SNMP_MIB_ITEM("MPRstNoToken", MPTCP_MIB_MPRSTNOTOKEN),

Maybe clearer if you add 'Found'. But this counter doesn't seem to be
needed, see below.

> +	SNMP_MIB_ITEM("MPRstNoMPJ", MPTCP_MIB_MPRSTNOMPJ),

It doesn't seem very clear. Perhaps clearer to add that one with the
other MPJoin? Possibly MPJoinSynAckNoMPJoin, or is it confusing?

> +	SNMP_MIB_ITEM("MPRstHMAC", MPTCP_MIB_MPRSTHMAC),

Not needed I think, see below.

> +	SNMP_MIB_ITEM("MPRstFatalFallback", MPTCP_MIB_MPRSTFATALFALLBACK),

That's a bit vague. See below.

> +	SNMP_MIB_ITEM("MPRstBadMap", MPTCP_MIB_MPRSTBADMAP),

Should it be linked to DSS? We have DssFallback, DSSCorruptionReset and
DSSCorruptionFallback. Should we have DssReset for this one?

> +	SNMP_MIB_ITEM("MPRstNotEstablished", MPTCP_MIB_MPRSTNOTESTABLISHED),

Here, it is a bit to vague: see below.

>  	SNMP_MIB_ITEM("SubflowStale", MPTCP_MIB_SUBFLOWSTALE),
>  	SNMP_MIB_ITEM("SubflowRecover", MPTCP_MIB_SUBFLOWRECOVER),
>  	SNMP_MIB_ITEM("SndWndShared", MPTCP_MIB_SNDWNDSHARED),
> diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
> index 812218b5ed2b..0ac109b52d35 100644
> --- a/net/mptcp/mib.h
> +++ b/net/mptcp/mib.h
> @@ -70,6 +70,13 @@ enum linux_mptcp_mib_field {
>  	MPTCP_MIB_MPFASTCLOSERX,	/* Received a MP_FASTCLOSE */
>  	MPTCP_MIB_MPRSTTX,		/* Transmit a MP_RST */
>  	MPTCP_MIB_MPRSTRX,		/* Received a MP_RST */
> +	MPTCP_MIB_MPRSTMD5SIG,		/* MP_RST: MD5SIG enabled on listener */
> +	MPTCP_MIB_MPRSTNOTOKEN,		/* MP_RST: JOIN token not found */
> +	MPTCP_MIB_MPRSTNOMPJ,		/* MP_RST: missing MPJ in SYN/ACK */
> +	MPTCP_MIB_MPRSTHMAC,		/* MP_RST: HMAC validation failed */
> +	MPTCP_MIB_MPRSTFATALFALLBACK,	/* MP_RST: fatal fallback on child */
> +	MPTCP_MIB_MPRSTBADMAP,		/* MP_RST: bad data mapping */
> +	MPTCP_MIB_MPRSTNOTESTABLISHED,	/* MP_RST: JOIN on not-established msk */
>  	MPTCP_MIB_SUBFLOWSTALE,		/* Subflows entered 'stale' status */
>  	MPTCP_MIB_SUBFLOWRECOVER,	/* Subflows returned to active status after being stale */
>  	MPTCP_MIB_SNDWNDSHARED,		/* Subflow snd wnd is overridden by msk's one */
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 17b9a8c13ebf..b06cbcf983f4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3836,6 +3836,7 @@ bool mptcp_finish_join(struct sock *ssk)
>  
>  	/* mptcp socket already closing? */
>  	if (!mptcp_is_fully_established(parent)) {
> +		MPTCP_INC_STATS(sock_net(parent), MPTCP_MIB_MPRSTNOTESTABLISHED);

As you mentioned above in a comment, it is linked to a Join: then I
think this should be reflected in the name. It might be better with
something like: MPJoinSynAckClose? (or with the Rst suffix?)

>  		subflow->reset_reason = MPTCP_RST_EMPTCP;
>  		return false;
>  	}
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index e2cb9d23e4a0..71bffcda0138 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -160,6 +160,7 @@ static int subflow_check_req(struct request_sock *req,
>  	 * TCP option space.
>  	 */
>  	if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) {
> +		MPTCP_INC_STATS(sock_net(sk_listener), MPTCP_MIB_MPRSTMD5SIG);

MD5SigReset?

>  		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
>  		return -EINVAL;
>  	}
> @@ -227,6 +228,7 @@ static int subflow_check_req(struct request_sock *req,
>  
>  		/* Can't fall back to TCP in this case. */
>  		if (!subflow_req->msk) {
> +			MPTCP_INC_STATS(sock_net(sk_listener), MPTCP_MIB_MPRSTNOTOKEN);

Do we not already have MPTCP_MIB_JOINNOTOKEN sent in this case?

I was initially going to say that NoToken was confusing, NoTokenFound
would be clearer, but it looks like we already have one for that.

>  			subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
>  			return -EPERM;
>  		}
> @@ -568,6 +570,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>  		u8 hmac[SHA256_DIGEST_SIZE];
>  
>  		if (!(mp_opt.suboptions & OPTION_MPTCP_MPJ_SYNACK)) {
> +			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPRSTNOMPJ);

MPJoinSynAckNoMPJoin?

>  			subflow->reset_reason = MPTCP_RST_EMPTCP;
>  			goto do_reset;
>  		}
> @@ -582,6 +585,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>  
>  		if (!subflow_thmac_valid(subflow)) {
>  			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINACKMAC);
> +			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPRSTHMAC);

Do we need this one if we already have MPJoinAckHMacFailure just above?
This other MIB counter is only used in case of HMAC issues and an MP_RST
will be sent.

Note that I see that at the other place where it is used, the reset
reason looks wrong (Administratively prohibited), it should also be an
MPTCP-specific error like here. Do you mind creating another patch
fixing that please? That's not urgent, so also only to the MPTCP ML,
with the 'mptcp-net' prefix this time, and a Fixes tag, please?

Also, here, should it not be MPJoinSynAckHMacFailure instead (SynAck,
not Ack)? If yes, we will need another patch with probably another Fixes
tag.

>  			subflow->reset_reason = MPTCP_RST_EMPTCP;
>  			goto do_reset;
>  		}
> @@ -870,6 +874,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>  		 */
>  		if (!ctx || fallback) {
>  			if (fallback_is_fatal) {
> +				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPRSTFATALFALLBACK);

Here, we should send a reset only for the MPJoin, so probably best to
link it to that. Then there are two conditions:

- fallback == true: MPJoin options are missing → MPJoinAckNoMPJoin?
- !ctx == no ULP: so I guess alloc failure (or a fallback happened
before? Not sure...) → MPJoinAckNoCtx?

>  				subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
>  				goto dispose_child;
>  			}
> @@ -1421,9 +1426,12 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  			 * subflow_error_report() will introduce the appropriate barriers
>  			 */
>  			subflow->reset_transient = 0;
> -			subflow->reset_reason = status == MAPPING_NODSS ?
> -						MPTCP_RST_EMIDDLEBOX :
> -						MPTCP_RST_EMPTCP;
> +			if (status == MAPPING_NODSS) {
> +				subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
> +			} else {
> +				MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_MPRSTBADMAP);
> +				subflow->reset_reason = MPTCP_RST_EMPTCP;
> +			}

It sounds better to increment the MIB counter (DssReset?) even if it is
due to a middlebox, no?

>  reset:
>  			WRITE_ONCE(ssk->sk_err, EBADMSG);

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