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