Re: [PATCH net v2 2/2] mptcp: diag: bound listener bucket lock hold

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Batch-relative offset used as an absolute skip count corrupts the resumption state (`diag_ctx->l_num`), causing massive duplicate dumps or infinite loops.
- [High] Dereferencing `ctx->conn` before verifying the socket ULP type leads to an out-of-bounds read for non-MPTCP sockets.
- [High] Dropping the lock and missing re-check of `icsk_ulp_data` after taking a reference on the `SLAB_TYPESAFE_BY_RCU` MPTCP socket causes a TOCTOU race.
--

commit 18ad84befba3a7e705432d2c50d6139e9f7b55e6
Author: Zihan Xi <zihanx@nebusec.ai>
Date:   Tue Sep 1 12:53:47 2026 +0000

    mptcp: diag: bound listener bucket lock hold

This commit modifies the MPTCP listener diag dump to collect referenced 
sockets while holding the bucket lock, and then executes sk_diag_dump()
after releasing the lock. It aims to reduce the lock hold time and tracks
resumption using a referenced cursor.

Link: https://lore.kernel.org/all/cover.1785307984.git.zihanx@nebusec.ai/

> diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
> index 136c2d05c0eeb..37b33ea5d79de 100644
> --- a/net/mptcp/mptcp_diag.c
> +++ b/net/mptcp/mptcp_diag.c
[ ... ]
> @@ -84,64 +98,102 @@ static void mptcp_diag_dump_listeners(struct sk_buff *skb, struct netlink_callba
>  	hinfo = net->ipv4.tcp_death_row.hashinfo;
>  
>  	for (i = diag_ctx->l_slot; i <= hinfo->lhash2_mask; i++) {
> +		struct sock *tmp, *sk, *sk_arr[MPTCP_DIAG_BULK_SZ];
>  		struct inet_listen_hashbucket *ilb;
> +		int num_arr[MPTCP_DIAG_BULK_SZ];
>  		struct hlist_nulls_node *node;
[ ... ]
> +		for (idx = 0; idx < accum; idx++) {
> +			const struct mptcp_subflow_context *ctx;
> +			const struct tcp_ulp_ops *ulp_ops;
> +			struct inet_sock *inet;
> +
> +			sk = sk_arr[idx];
> +			rcu_read_lock();
> +			ctx = rcu_dereference(inet_csk(sk)->icsk_ulp_data);
> +			ulp_ops = READ_ONCE(inet_csk(sk)->icsk_ulp_ops);
> +			inet = inet_sk(sk);
> +			tmp = ctx ? ctx->conn : NULL;

[Severity: High]
Can dereferencing `ctx->conn` here before verifying the ULP protocol name 
lead to an out-of-bounds read? 

If another protocol (like TLS or SMC) attaches a smaller ULP context to a
TCP listener, `ctx` would point to that smaller structure, and reading
`ctx->conn` would access memory past the end of the ULP object. Should the
ULP type be checked before accessing `ctx->conn`?

> +			if (!ctx || !ulp_ops || strcmp(ulp_ops->name, "mptcp") ||
> +			    !tmp || !net_eq(sock_net(tmp), net) ||
> +			    (r->sdiag_family != AF_UNSPEC &&
> +			     tmp->sk_family != r->sdiag_family) ||
> +			    (r->id.idiag_sport != inet->inet_sport &&
> +			     r->id.idiag_sport) ||
> +			    !refcount_inc_not_zero(&tmp->sk_refcnt)) {
> +				rcu_read_unlock();
> +				goto processed_listener_sk;
> +			}
> +			rcu_read_unlock();

[Severity: High]
Since sockets are allocated with SLAB_TYPESAFE_BY_RCU, and the listener hash
bucket lock has been dropped at this point, is it possible for the `tmp`
socket to be freed and reallocated as an unrelated socket between finding it
and successfully incrementing the refcount? 

Does the code need to re-check `rcu_dereference(inet_csk(sk)->icsk_ulp_data)`
against `ctx` after taking the refcount to ensure the object was not
reallocated during the lockless window?

> +			if (ret >= 0) {
> +				ret = sk_diag_dump(tmp, skb, cb, r, net_admin);
> +				if (ret < 0)
> +					num = num_arr[idx];
> +			}
> +			sock_put(tmp);
> +processed_listener_sk:
> +			if (ret >= 0)
> +				mptcp_diag_save_cursor(cb_data, i, sk);
> +			sock_put(sk);
> +		}
> +
> +		if (ret < 0) {
> +			diag_ctx->l_slot = i;
> +			diag_ctx->l_num = num;
> +			return;
> +		}

[Severity: High]
`diag_ctx->l_num` tracks the absolute position in the bucket, but `num` was
reset to 0 at the start of `resume_listen_walk`. 

If the dump fails (e.g. skb is full), doesn't saving this batch-relative `num`
into `diag_ctx->l_num` corrupt the absolute progress state? Because it saves a 
small relative number, if the cursor is invalidated later, the dump would fall 
back to skipping elements from the start, causing massive duplicate dumps or 
infinite loops.

> +
> +		cond_resched();
> +
> +		if (accum == MPTCP_DIAG_BULK_SZ) {
> +			diag_ctx->l_num = 0;
> +			goto resume_listen_walk;
> +		}

[Severity: High]
Similarly, on a successful batch, `diag_ctx->l_num` is explicitly set to 0.
Does this intentionally destroy the absolute index between batches? 

If the cursor gets lost later, this would guarantee a total rewind to the 
beginning of the bucket.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788248601.git.zihanx@nebusec.ai?part=2