[PATCH net v2] mptcp: protect listener backlog reads in diagnostics

[PATCH net v2] mptcp: protect listener backlog reads in diagnostics

[Cen Zhang]

mptcp_diag_get_info() reports the accept queue of a listening MPTCP
socket by reading msk->first. The token based dump and the exact token
query only hold a reference on the MPTCP socket, which pins neither the
first subflow nor the listener hash bucket, and __mptcp_close_ssk()
drops the last reference on the first subflow before clearing
msk->first.

An application can disconnect a connected MPTCP socket with AF_UNSPEC,
which removes the token but retains the first subflow, listen on it
again and close it. A dump that obtained the MPTCP socket before the
disconnect then reads the backlog of a freed TCP listener:

  Token diagnostic                 Application
  mptcp_token_iter_next()
    retain MPTCP socket
                                   connect(AF_UNSPEC)
                                     remove token, retain first
                                   listen()
                                     reuse first
  mptcp_diag_get_info()
    observe TCP_LISTEN
    lsk = msk->first
                                   close()
                                     mptcp_destroy()
                                       __mptcp_close_ssk()
                                         release first
    read lsk->sk_ack_backlog

The report below was decoded with scripts/decode_stacktrace.sh against
the 7.2.0-rc5 based test kernel that produced it, with the line numbers
mapped to this tree; only the relevant frames are kept:

  BUG: KASAN: slab-use-after-free in mptcp_diag_get_info (net/mptcp/mptcp_diag.c:209)
  Read of size 4 at addr ffff888111763818 by task python3/499

  Call Trace:
   mptcp_diag_get_info (net/mptcp/mptcp_diag.c:209)
   inet_sk_diag_fill (net/ipv4/inet_diag.c:330)
   mptcp_diag_dump (net/mptcp/mptcp_diag.c:23 net/mptcp/mptcp_diag.c:177)
   __inet_diag_dump (net/ipv4/inet_diag.c:823)
   netlink_dump (net/netlink/af_netlink.c:2351)

  Allocated by task 497:
   sk_alloc (net/core/sock.c:2337)
   inet_create (net/ipv4/af_inet.c:333)
   __sock_create (net/socket.c:1679)
   mptcp_subflow_create_socket (net/mptcp/subflow.c:1836)
   __mptcp_nmpc_sk (net/mptcp/protocol.c:111 net/mptcp/protocol.c:143)
   mptcp_setsockopt (net/mptcp/sockopt.c:313 net/mptcp/sockopt.c:916)

  Freed by task 0:
   slab_free_after_rcu_debug (mm/slub.c:6571)
   rcu_core (kernel/rcu/tree.c:2919)

  Second to last potentially related work creation:
   __call_rcu_common.constprop.0 (kernel/rcu/tree.c:3287)
   sk_destruct (net/core/sock.c:2446)
   __sk_free (net/core/sock.c:2459)
   sk_free (net/core/sock.c:2470)
   __mptcp_close_ssk (include/net/sock.h:2024 net/mptcp/protocol.c:2721)
   mptcp_destroy_common (net/mptcp/protocol.c:3579)
   mptcp_destroy (net/mptcp/protocol.c:3815)
   __mptcp_destroy_sock (net/mptcp/protocol.c:3421)
   __mptcp_close (net/mptcp/protocol.c:3523)
   mptcp_close (net/mptcp/protocol.c:3538)
   inet_release (net/ipv4/af_inet.c:442)

The first subflow is a TCP socket: TCP listeners carry SOCK_RCU_FREE
and tcp_prot/tcpv6_prot sockets live in SLAB_TYPESAFE_BY_RCU caches, so
reading the two backlog counters under rcu_read_lock() is safe, and a
stale value is acceptable for diagnostics. mptcp_diag_dump_listeners()
already runs the formatter inside an RCU read-side section; only the
token dump and mptcp_diag_dump_one() reach it without one. Take
rcu_read_lock() around the msk->first snapshot and the backlog reads.

Fixes: 4fa39b701ce9 ("mptcp: listen diag dump support")
Suggested-by: Eric Dumazet <edumazet@kernel.org>
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
Assisted-by: LLM
---
Changes in v2:
- Take rcu_read_lock() around the first subflow reads instead of the MPTCP
  data lock plus an owner check, as suggested by Eric Dumazet; this avoids
  the new msk slock -> listener hash lock nesting and keeps ss reporting the
  accept queue while the socket is owned.
- Decode the stack trace with scripts/decode_stacktrace.sh, map the line
  numbers to this tree and keep only the relevant frames.

Link to v1: https://lore.kernel.org/r/pm-mptcp-objects-candidate-0066-v3-73df72b7a543d4cada0e@gmail.com

 net/mptcp/mptcp_diag.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
index 136c2d05c0ee..28a42eee6651 100644
--- a/net/mptcp/mptcp_diag.c
+++ b/net/mptcp/mptcp_diag.c
@@ -200,8 +200,10 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
 	r->idiag_wqueue = sk_wmem_alloc_get(sk);
 
 	if (inet_sk_state_load(sk) == TCP_LISTEN) {
-		struct sock *lsk = READ_ONCE(msk->first);
+		struct sock *lsk;
 
+		rcu_read_lock();
+		lsk = READ_ONCE(msk->first);
 		if (lsk) {
 			/* override with settings from tcp listener,
 			 * so Send-Q will show accept queue.
@@ -209,6 +211,7 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
 			r->idiag_rqueue = READ_ONCE(lsk->sk_ack_backlog);
 			r->idiag_wqueue = READ_ONCE(lsk->sk_max_ack_backlog);
 		}
+		rcu_read_unlock();
 	}
 
 	if (!info)

Re: [PATCH net v2] mptcp: protect listener backlog reads in diagnostics

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2] mptcp: protect listener backlog reads in diagnostics

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch turns msk->first into an RCU-protected pointer for
  diag readers, but the code that frees the first subflow still frees it…

--

mptcp: protect listener backlog reads in diagnostics

This patch puts the msk->first load and the listener backlog reads in
mptcp_diag_get_info() under rcu_read_lock(). The goal is to fix a KASAN
slab-use-after-free that happens when a token dump or
mptcp_diag_dump_one() races with close() on an MPTCP socket that was
put back into listen after connect(AF_UNSPEC). The fix depends on
SOCK_RCU_FREE and the SLAB_TYPESAFE_BY_RCU TCP slab caches to keep the
first subflow readable.

> diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
> index 136c2d05c0ee..28a42eee6651 100644
> --- a/net/mptcp/mptcp_diag.c
> +++ b/net/mptcp/mptcp_diag.c
> @@ -200,8 +200,10 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
>  	r->idiag_wqueue = sk_wmem_alloc_get(sk);
>  
>  	if (inet_sk_state_load(sk) == TCP_LISTEN) {
> -		struct sock *lsk = READ_ONCE(msk->first);
> +		struct sock *lsk;
>  
> +		rcu_read_lock();
> +		lsk = READ_ONCE(msk->first);

[Severity: High]
Is rcu_read_lock() here enough by itself? The writer in
__mptcp_close_ssk() frees the subflow first and only then clears
msk->first:

net/mptcp/protocol.c:__mptcp_close_ssk() {
    ...
	release_sock(ssk);

	sock_put(ssk);

	if (ssk == msk->first)
		WRITE_ONCE(msk->first, NULL);
    ...
}

On the path close() -> __mptcp_close() -> __mptcp_destroy_sock() ->
mptcp_destroy() -> mptcp_destroy_common(), with free_first set, this
sock_put() drops the last reference. The KASAN trace in the changelog
shows exactly that. For a SOCK_RCU_FREE listener, sk_destruct() then
queues call_rcu(__sk_destruct). msk->first is still published until the
WRITE_ONCE() that follows.

A grace period only covers readers that were already inside a read-side
section when call_rcu() was called. The closer runs in sleepable process
context, outside RCU. On a PREEMPT_RCU kernel, could it be preempted
between sock_put() and WRITE_ONCE() long enough for both the
__sk_destruct grace period and the later SLAB_TYPESAFE_BY_RCU (or
SLUB_RCU_DEBUG) grace period to finish?

The reader side has a matching gap. The TCP_LISTEN check above runs
outside RCU, and both mptcp_diag_dump() and mptcp_diag_dump_one() can
sleep. A reader could see TCP_LISTEN, get preempted, and enter
rcu_read_lock() only after those grace periods. It would then load the
stale msk->first and read sk_ack_backlog and sk_max_ack_backlog from
freed or reused memory into idiag_rqueue/idiag_wqueue.

Wouldn't that be the same slab-use-after-free shown in the KASAN report?

There also seems to be a second path through mptcp_listen():

	mptcp_set_state(sk, TCP_LISTEN);
	sock_set_flag(sk, SOCK_RCU_FREE);

	lock_sock(ssk);
	err = __inet_listen_sk(ssk, backlog);

The msk becomes TCP_LISTEN before the first subflow is hashed. The
subflow only gets SOCK_RCU_FREE in __inet_hash(), after hashing
succeeds. If listen fails, the subflow is later freed synchronously
through __sk_destruct(), and again msk->first is cleared only after
that.

The changelog says SOCK_RCU_FREE plus the typesafe slab make this read
safe. Doesn't that hold only if msk->first is unpublished before
reclamation starts? mptcp_sk_clone_init() already uses that order:

	list_del_init(&subflow->node);
	WRITE_ONCE(msk->first, NULL);
	sock_put(ssk);

Would it close the remaining window to clear msk->first in
__mptcp_close_ssk() before the final sock_put(ssk), and keep the
rcu_read_lock() added here?

>  		if (lsk) {
>  			/* override with settings from tcp listener,
>  			 * so Send-Q will show accept queue.
> @@ -209,6 +211,7 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
>  			r->idiag_rqueue = READ_ONCE(lsk->sk_ack_backlog);
>  			r->idiag_wqueue = READ_ONCE(lsk->sk_max_ack_backlog);
>  		}
> +		rcu_read_unlock();
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/pm-mptcp-objects-candidate-0066-v5-4d1b83d62fcde2d57e8d%40gmail.com