Re: [PATCH net v5 2/2] mptcp: fix MP_CAPABLE token migration when cloning reqsk

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The new non-OOM failure path in mptcp_sk_clone_init()
  (net/mptcp/protocol.c:3780-3786) downgrades an already negotiated…

--

mptcp: fix MP_CAPABLE token migration when cloning reqsk

This moves MP_CAPABLE token ownership from an original request socket to
its inet_reqsk_clone() copy under the token bucket lock, and makes
mptcp_token_accept() and mptcp_token_destroy_request() re-check
token_node under that lock instead of warning.  mptcp_token_accept() now
returns a bool, and mptcp_sk_clone_init() destroys the provisional msk
and returns NULL when the token cannot be claimed.

Closes: https://sashiko.dev/#/patchset/86e2514b533bf4d55d4aa2fdbf1404022e8c9430.1776149210.git.caoruide123%40gmail.com

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e1f08f71cdb16..30067f2a7ca8f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -3764,6 +3777,14 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
>  	mptcp_rcv_space_init(msk, ssk);
>  	msk->rcvq_space.time = mptcp_stamp();
>  
> +	if (!mptcp_token_accept(subflow_req, msk)) {
> +		list_del_init(&subflow->node);
> +		WRITE_ONCE(msk->first, NULL);
> +		sock_put(ssk);
> +		mptcp_sk_clone_destroy(nsk);
> +		return NULL;
> +	}
> +

[Severity: Medium]
Is this new failure path completely silent from a user's point of view?

In subflow_syn_recv_sock() the NULL return is turned into a fallback:

net/mptcp/subflow.c:subflow_syn_recv_sock() {
	...
	if (ctx->mp_capable) {
		ctx->conn = mptcp_sk_clone_init(listener->conn, &mp_opt, child, req);
		if (!ctx->conn)
			goto fallback;
	...
}

but the fallback label only bumps a counter when the local fallback bool
is set:

net/mptcp/subflow.c:subflow_syn_recv_sock() {
	...
fallback:
	if (fallback)
		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK);
	mptcp_subflow_drop_ctx(child);
	return child;
}

Reaching the ctx->mp_capable block requires having already passed:

	if (!ctx || fallback) {
		...
		goto fallback;
	}

so on the token_accept failure path fallback is false and nothing is
counted.  Every sibling failure site in the same function increments
something (MPTCP_MIB_MPJOINACKNOCTX, MPTCP_MIB_MPJOINACKNOMPJOIN,
MPTCP_MIB_JOINACKMAC, MPTCP_MIB_JOINREJECTED).

At the same time the two WARN_ON_ONCE() calls that used to fire for
exactly this state are dropped in this patch, in mptcp_token_accept():

-	/* pedantic lookup check for the moved token */
 	pos = __token_lookup_req(bucket, req->token);
-	if (!WARN_ON_ONCE(pos != req))
-		hlist_nulls_del_init_rcu(&req->token_node);

and in mptcp_token_destroy_request():

 	pos = __token_lookup_req(bucket, subflow_req->token);
-	if (!WARN_ON_ONCE(pos != subflow_req)) {
+	if (pos == subflow_req) {

The changelog describes this as trading "an exceptionally rare fallback
for eliminating the warning", but with no dmesg output and no MPTcpExt
counter, is there any way left to observe that a negotiated passive
MP_CAPABLE connection was downgraded to plain TCP, either in the field
or from the mptcp selftests?

Would it make sense to increment MPTCP_MIB_MPCAPABLEPASSIVEFALLBACK
(exported as MPCapableFallbackACK, documented in mib.h as "Server-side
fallback during 3-way handshake"), or MPTCP_MIB_TOKENFALLBACKINIT, on
this path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788202924.git.caoruide123%40gmail.com