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

[Eric Dumazet]

Le lun. 26 oct. 2026 à 13:04, Cen Zhang <zzzccc427@xxxxxxxxx> a écrit :
>
> 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
>
> Take the MPTCP data lock around the listener state check and the
> backlog reads, and only follow msk->first while no process owns the
> socket. Every path that disposes of the first subflow runs with the
> socket owned, and lock_sock() has to take the same spinlock to become
> the owner, so the reads cannot overlap with the release. Waiting for
> the owner is not an option: mptcp_diag_dump_listeners() calls this
> function with the listener hash bucket lock held, which the owner needs
> to unhash that very listener. While a process owns the socket, the dump
> reports the MPTCP socket's own queue values instead.
>
> BUG: KASAN: slab-use-after-free in mptcp_diag_get_info+0x415/0x490
> Read of size 4 at addr ffff888111763818 by task python3/499
>
> Call Trace:

Adding these stack traces without symbols is not really nice.

Time to use scripts/decode_stacktrace.sh

>
> Fixes: 4fa39b701ce9 ("mptcp: listen diag dump support")
> Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> Assisted-by: LLM
> ---
>  net/mptcp/mptcp_diag.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
> index 136c2d05c0ee..a71bcaee15c8 100644
> --- a/net/mptcp/mptcp_diag.c
> +++ b/net/mptcp/mptcp_diag.c
> @@ -199,7 +199,12 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
>                           READ_ONCE(mptcp_sk(sk)->backlog_len);
>         r->idiag_wqueue = sk_wmem_alloc_get(sk);
>
> -       if (inet_sk_state_load(sk) == TCP_LISTEN) {
> +       /* The listener walk holds the listen hash lock: do not wait for a
> +        * socket owner that could be unhashing the first subflow.
> +        */
> +       mptcp_data_lock(sk);
> +       if (!sock_owned_by_user(sk) &&
> +           inet_sk_state_load(sk) == TCP_LISTEN) {
>                 struct sock *lsk = READ_ONCE(msk->first);
>
>                 if (lsk) {
> @@ -210,6 +215,7 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
>                         r->idiag_wqueue = READ_ONCE(lsk->sk_max_ack_backlog);
>                 }
>         }
> +       mptcp_data_unlock(sk);
>
>         if (!info)
>                 return;

This seems overkill, and adds a new msk slock -> ilb->lock nesting from
mptcp_diag_dump_listeners(). It also makes ss randomly report msk
rmem/wmem instead of the accept queue while the listener is owned.

msk->first is a TCP socket: TCP listeners have SOCK_RCU_FREE, and
tcp_prot / tcpv6_prot use SLAB_TYPESAFE_BY_RCU. Reading two integers
under rcu_read_lock() is safe, and stale values are fine for diag.

mptcp_diag_dump_listeners() already holds rcu_read_lock(), only the
token dump and mptcp_diag_dump_one() paths are missing it.

Something like:

diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
index 136c2d05c0eeb8f7050344a63396396d8718206c..28a42eee665129fe56c47dc2dd5b265ba18b2e95
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)


pw-bot: cr