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

[netdev-bot+sashiko]

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

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

- [High] Use-after-free introduced by the new deferred processing loop in
  mptcp_diag_dump_listeners().
- [Medium] The patch silently changes which MPTCP listeners the dump
  reports, and the commit message does not mention it.
- [Medium] mptcp_diag_dump_listeners() mixes two incompatible counting
  bases for the resume index diag_ctx->l_num.
- [Medium] mptcp_diag_dump_listeners() performs all
  identity/authorization checks on the parent MPTCP socket…
- [Low] Commit message of "mptcp: diag: bound listener bucket lock hold"
  carries Fixes: 4fa39b701ce9 + Cc: stable + Reported-by:, but never…
- [Low] In the new post-lock loop of mptcp_diag_dump_listeners()
  (net/mptcp/mptcp_diag.c), `tmp = ctx ?

--

mptcp: diag: bound listener bucket lock hold

This reworks mptcp_diag_dump_listeners() so that the listener bucket lock is
only held while collecting up to MPTCP_DIAG_BULK_SZ referenced listener
sockets, with the ULP/property re-checks, the parent msk reference and
sk_diag_dump() moved outside the lock.  A referenced dump cursor is kept in
cb->data so later batches resume after the previous listener instead of
rescanning the bucket prefix.

v1 Link: https://lore.kernel.org/all/cover.1785307984.git.zihanx@xxxxxxxxxx/

> diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
> index 6ccd32bc48f91..4859e77a28c7a 100644
> --- a/include/linux/inet_diag.h
> +++ b/include/linux/inet_diag.h
> @@ -39,6 +39,7 @@ enum inet_diag_dump_cursor_type {
>  	INET_DIAG_DUMP_CURSOR_TCP_LISTEN,
>  	INET_DIAG_DUMP_CURSOR_TCP_BIND,
>  	INET_DIAG_DUMP_CURSOR_TCP_EHASH,
> +	INET_DIAG_DUMP_CURSOR_MPTCP_LISTEN,
>  };

[Severity: Low]
Two questions about the changelog for this one.

First, the message says the lock hold time "can therefore grow with the
number of sockets visited", but it never states the observed symptom.  Was
this a soft lockup, an RCU stall, or measured contention on ilb->lock?  Given
the Fixes: 4fa39b701ce9 and Cc: stable tags plus the Reported-by:, could the
symptom be spelled out?

Second, could the backport dependency be mentioned?  enum
inet_diag_dump_cursor_type does not exist in the tree this applies on top of:

  git grep inet_diag_dump_cursor_type <base>   -> no match

The whole enum, cb_data->dump_cursor, dump_cursor_slot, dump_cursor_type and
inet_diag_dump_clear_cursor() come from the immediately preceding patch of
this series, "tcp: diag: bound bucket lock hold in tcp_diag_dump()".  Stable
trees carrying 4fa39b701ce9 cannot take this patch on its own, and nothing in
the message says so.

> 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
> @@ -12,6 +12,19 @@
>  #include <net/netlink.h>
>  #include "protocol.h"
>  
> +/* Process a bounded number of listeners per bucket lock hold. */
> +#define MPTCP_DIAG_BULK_SZ 16
> +
> +static void mptcp_diag_save_cursor(struct inet_diag_dump_data *cb_data,
> +				   unsigned int slot, struct sock *sk)
> +{
> +	sock_hold(sk);
> +	inet_diag_dump_clear_cursor(cb_data);
> +	cb_data->dump_cursor = sk;
> +	cb_data->dump_cursor_slot = slot;
> +	cb_data->dump_cursor_type = INET_DIAG_DUMP_CURSOR_MPTCP_LISTEN;
> +}
> +

[ ... ]

> @@ -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;
> -		struct sock *sk;
> -		int num = 0;
> +		int accum, idx, num, ret;
> +		struct sock *cursor;
> +		bool use_cursor;
>  
> +resume_listen_walk:
> +		num = 0;
> +		accum = 0;
>  		ilb = &hinfo->lhash2[i];
> +		ret = 0;
>  
>  		rcu_read_lock();
>  		spin_lock(&ilb->lock);
> -		sk_nulls_for_each(sk, node, &ilb->nulls_head) {
> -			const struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(sk);
> -			struct inet_sock *inet = inet_sk(sk);
> -			int ret;
> -
> -			if (num < diag_ctx->l_num)
> -				goto next_listen;
> -
> -			if (!ctx || strcmp(inet_csk(sk)->icsk_ulp_ops->name, "mptcp"))
> -				goto next_listen;
> -
> -			sk = ctx->conn;
> -			if (!sk || !net_eq(sock_net(sk), net))
> -				goto next_listen;
> -
> -			if (r->sdiag_family != AF_UNSPEC &&
> -			    sk->sk_family != r->sdiag_family)
> -				goto next_listen;
> -
> -			if (r->id.idiag_sport != inet->inet_sport &&
> -			    r->id.idiag_sport)
> +		cursor = cb_data->dump_cursor;
> +		use_cursor = cursor &&
> +			     cb_data->dump_cursor_type ==
> +			     INET_DIAG_DUMP_CURSOR_MPTCP_LISTEN &&
> +			     cb_data->dump_cursor_slot == i &&
> +			     inet_sk_state_load(cursor) == TCP_LISTEN &&
> +			     !hlist_nulls_unhashed(&cursor->sk_nulls_node) &&
> +			     cursor->sk_nulls_node.pprev != LIST_POISON2 &&
> +			     inet_lhash2_bucket_sk(hinfo, cursor) == ilb;
> +		node = use_cursor ? cursor->sk_nulls_node.next :
> +				    ilb->nulls_head.first;
> +		hlist_nulls_for_each_entry_from(sk, node, sk_nulls_node) {
> +			if (!use_cursor && num < diag_ctx->l_num)
>  				goto next_listen;

[Severity: Medium]
Are the two resume mechanisms here using the same counting base for
diag_ctx->l_num?

This test counts entries from the bucket head, but at resume_listen_walk num
is reset to 0 and, when use_cursor is true, the walk starts at
cursor->sk_nulls_node.next, i.e. mid-bucket.  The values stashed in num_arr[]
and later stored are therefore cursor-relative and bounded by 0..15:

	if (ret >= 0) {
		ret = sk_diag_dump(tmp, skb, cb, r, net_admin);
		if (ret < 0)
			num = num_arr[idx];
	}
	...
	if (ret < 0) {
		diag_ctx->l_slot = i;
		diag_ctx->l_num = num;
		return;
	}

Walking a bucket with more than 16 matching listeners:

  batch 1: 16 listeners dumped, cursor saved on e15
  goto resume_listen_walk with diag_ctx->l_num = 0, use_cursor true, so num
    now counts from e16
  skb fills at idx 5, so diag_ctx->l_num = 5 while the real bucket offset
    is 21
  the cursor socket is closed before the next recvmsg(), use_cursor becomes
    false, and the walk resumes at bucket position 5

Does that re-emit e5..e19, and, if entries were removed in the meantime and
positions shifted, silently drop listeners that were never reported?

The peer implementation of the same cursor protocol from the preceding patch,
tcp_diag_dump(), zeroes the positional counter when the cursor is unusable:

	node = use_cursor ? cursor->sk_nulls_node.next : ilb->nulls_head.first;
	if (!use_cursor)
		s_num = 0;

Should mptcp_diag_dump_listeners() do the same so the two bases are never
mixed?  The intra-call restart also forces diag_ctx->l_num = 0 before goto
resume_listen_walk, so a cursor invalidated between lock drops restarts the
bucket at the head within a single dump call.

>  			if (!refcount_inc_not_zero(&sk->sk_refcnt))
>  				goto next_listen;
>  
> -			ret = sk_diag_dump(sk, skb, cb, r, net_admin);
> -
> -			sock_put(sk);
> -
> -			if (ret < 0) {
> -				spin_unlock(&ilb->lock);
> -				rcu_read_unlock();
> -				diag_ctx->l_slot = i;
> -				diag_ctx->l_num = num;
> -				return;
> -			}
> -			diag_ctx->l_num = num + 1;
> -			num = 0;

[Severity: Medium]
Removing this "l_num = num + 1; num = 0;" pair changes which listeners the
dump reports, and the changelog does not mention it.

In the pre-patch loop the skip test was "if (num < diag_ctx->l_num)" while
num was reset to 0 after every successful dump and l_num kept growing, so for
a bucket e0..e6 the emitted set was e0, e1, e3, e6:

  e0 dumped -> l_num = 1, num = 0 then ++num = 1
  e1 dumped -> l_num = 2, num = 1
  e2 hits 1 < 2 and is skipped
  e3 dumped -> l_num = 3
  e4, e5 skipped

The new code keeps num monotonic and applies the skip test only in the
non-cursor fallback, so every collected listener is processed.  Is that the
actual user-visible defect being fixed here?  ss -Ml output changes (more
listeners reported) on any host with three or more MPTCP listeners hashing
into the same lhash2 bucket, and this seems worth stating in the message,
along with whether it belongs in the same patch as the lock-hold change.

> +			num_arr[accum] = num;
> +			sk_arr[accum] = sk;
> +			if (++accum == MPTCP_DIAG_BULK_SZ)
> +				break;
>  next_listen:
>  			++num;
>  		}
>  		spin_unlock(&ilb->lock);
>  		rcu_read_unlock();
>  
> +		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 this read freed memory?  The rcu_read_lock() here is entered after
spin_unlock(&ilb->lock) and rcu_read_unlock() above, so it cannot extend a
grace period that already started.

The collection loop only took sk_refcnt on the listener subflow, which does
not keep the ULP context alive:

  __tcp_close()
    tcp_set_state(sk, TCP_CLOSE)
    inet_unhash(sk)               /* takes ilb->lock, free once diag drops it */
    inet_csk_destroy_sock()
      sk->sk_prot->destroy(sk)
        tcp_v4_destroy_sock()
          tcp_cleanup_ulp()
            subflow_ulp_release()

and net/mptcp/subflow.c:subflow_ulp_release() drops the msk reference and
frees the context:

	sk = ctx->conn;
	if (sk) {
		...
		sock_put(sk);
	}
	mptcp_subflow_ops_undo_override(ssk);
	if (release)
		kfree_rcu(ctx, rcu);

net/ipv4/tcp_ulp.c:tcp_cleanup_ulp() clears only icsk_ulp_ops:

	if (icsk->icsk_ulp_ops->release)
		icsk->icsk_ulp_ops->release(sk);
	module_put(icsk->icsk_ulp_ops->owner);

	icsk->icsk_ulp_ops = NULL;

icsk_ulp_data is never unpublished on this path (subflow_ulp_fallback() does
rcu_assign_pointer(icsk->icsk_ulp_data, NULL), this path does not).  If
kfree_rcu(ctx) was queued before this loop iteration re-entered
rcu_read_lock(), the grace period does not cover this reader, so
rcu_dereference(inet_csk(sk)->icsk_ulp_data) can return a freed and reused
slab object, and ctx->conn is dereferenced before the !ulp_ops guard is even
evaluated.

The loop is also preemptible for up to 15 preceding sk_diag_dump() /
inet_diag_bc_sk() invocations with request-supplied bytecode, which widens the
window.  Does the commit message claim ("After dropping the lock, read
icsk_ulp_data with rcu_dereference()") hold here, or do ctx and the parent msk
reference have to be acquired while ilb->lock is still held, as the pre-patch
code did?

[Severity: Low]
Separately, is it intentional that ctx->conn is loaded before the check that
the ULP is actually "mptcp"?  The collection loop under ilb->lock no longer
filters by ULP at all, so listeners with a foreign ULP context now reach this
loop, and the read happens at offsetof(struct mptcp_subflow_context, conn)
inside an object of an unrelated type.

net/xfrm/espintcp.c:espintcp_init_sk() publishes its own object with
rcu_assign_pointer(icsk->icsk_ulp_data, ctx) and, unlike tls_init(), has no
TCP-state restriction, so an unprivileged setsockopt(TCP_ULP, "espintcp")
followed by listen() puts such a socket into lhash2.

struct espintcp_ctx is larger than that offset today, so the read stays inside
the allocation and the loaded value is discarded by the failing strcmp().
Still, the pre-patch code did the strcmp() first and never touched a foreign
context.  Could the ctx->conn load be moved after the ULP name check?

> +			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();
> +			if (ret >= 0) {
> +				ret = sk_diag_dump(tmp, skb, cb, r, net_admin);
> +				if (ret < 0)
> +					num = num_arr[idx];
> +			}
> +			sock_put(tmp);

[Severity: Medium]
Does this need a re-validation after refcount_inc_not_zero(&tmp->sk_refcnt)?

All the identity and scoping checks on the parent msk (net_eq(sock_net(tmp),
net), sdiag_family, idiag_sport) run before the reference is taken, and
nothing is re-checked afterwards.  mptcp_prot uses SLAB_TYPESAFE_BY_RCU:

net/mptcp/protocol.c:
	.slab_flags	= SLAB_TYPESAFE_BY_RCU,

so rcu_read_lock() only guarantees type stability, not identity.  If
subflow_ulp_release() already dropped the last reference via
sock_put(ctx->conn), the object can be recycled as a different MPTCP socket
inside this reader's RCU section, refcount_inc_not_zero() then succeeds, and
sk_diag_dump(tmp, ...) serializes that socket's addresses, ports, uid, cookie
and state into the reply, possibly from another namespace.

The in-tree pattern for this slab, net/mptcp/token.c:mptcp_token_get_sock(),
re-checks after the acquisition:

	if (!refcount_inc_not_zero(&sk->sk_refcnt))
		goto not_found;

	if (READ_ONCE(msk->token) != token ||
	    !net_eq(sock_net(sk), net)) {

Should the same re-check be done here?  Pre-patch the conn read and the msk
refcount acquisition both happened under ilb->lock with the subflow still
hashed, so the reference held by ctx was still alive.

> +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;
> +		}
> +
>  		cond_resched();
> +
> +		if (accum == MPTCP_DIAG_BULK_SZ) {
> +			diag_ctx->l_num = 0;
> +			goto resume_listen_walk;
> +		}
> +
> +		inet_diag_dump_clear_cursor(cb_data);
>  		diag_ctx->l_num = 0;
>  	}
>  
> +	inet_diag_dump_clear_cursor(cb_data);
>  	diag_ctx->l_num = 0;
>  	diag_ctx->l_slot = i;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788248601.git.zihanx%40nebusec.ai