Re: [PATCH net 2/2] mptcp: release the parent socket reference with the subflow context

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Critical] The new sock_put(ctx->conn) sits in subflow_ctx_destruct(),
  and a TCP child cloned from a retired MPTCP listener subflow can…
- [High] The fix is incomplete.
- [Medium] The commit message presents the delegated-action use-after-
  free as a live bug that this patch fixes: the second interleaving, the…
- [Low] The comment in subflow_ulp_release()
  (net/mptcp/subflow.c:2115-2118) says the context 'will be freed by…

--

I'm sending this review as a reply to the patch on the list. It has four inline comments:

- **Medium:** the commit message credits this patch with a fix that the previous commit in the series already makes.
- **High:** mptcp_subflow_drop_ctx() still drops the parent reference immediately, so the fix is incomplete.
- **Critical:** a possible double free of the subflow context when a child is cloned from a listener that is being closed.
- **Low:** a stale comment.

mptcp: release the parent socket reference with the subflow context

This commit moves the sock_put() on the MPTCP parent socket from
subflow_ulp_release() into subflow_ctx_destruct(). The parent reference
then lives as long as the subflow socket. The aim is to fix use-after-free
in bpf_skc_to_mptcp_sock() readers and in delegated action processing,
building on the previous commit that keeps a retired context attached
until sk_destruct.

> A delegated action pins the TCP subflow socket, but
> mptcp_subflow_process_delegated() locks and updates ctx->conn. A peer
> reset followed by the application closing the MPTCP socket releases
> the parent before mptcp_napi_poll() takes its data lock:
>
>   Delegation / NAPI                 Peer reset / application close
>   queue SEND or SNDBUF
>     sock_hold(ssk)
>                                     subflow_ulp_release(ssk)
>                                       sock_put(parent)
>                                     mptcp_close(parent)
>                                       last sock_put(parent)
>   mptcp_napi_poll()
>     mptcp_subflow_process_delegated()
>       mptcp_data_lock(parent)

[ ... ]

> Fixes: b19bc2945b40 ("mptcp: implement delegated actions")

[Severity: Medium]
Can this delegated action sequence still happen on the tree this patch
applies to?

The previous commit in the series, "mptcp: keep delegated subflow contexts
alive until socket release", added this guard to mptcp_napi_poll() in
net/mptcp/protocol.c:

	bh_lock_sock_nested(ssk);
	if (unlikely(!READ_ONCE(inet_csk(ssk)->icsk_ulp_ops))) {
		...
		goto unlock;
	}

Before this patch, the parent sock_put() ran in subflow_ulp_release().
That function is called from tcp_cleanup_ulp() right before:

	icsk->icsk_ulp_ops = NULL;

At that point the subflow is owned by the user or bh-locked. So
mptcp_napi_poll() sees one of three states:

- An attached ULP, where ctx->conn still pins the parent.
- An owned socket, where it only clears the scheduled bit.
- NULL ops, where it skips the entry.

Also, tcp_release_cb_override() isn't reached after the ULP release,
because mptcp_subflow_ops_undo_override() restores sk_prot.

If that's right, this patch only fixes the bpf_skc_to_mptcp_sock() case.
For delegated actions it is defence in depth.

Should the second interleaving, the second KASAN report and the
b19bc2945b40 Fixes tag move to the previous commit instead?

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8dc0677dc5514..ebcface15661a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c

[ ... ]

> @@ -820,6 +822,8 @@ static void subflow_ctx_destruct(struct sock *ssk)
>
>  	rcu_assign_pointer(inet_csk(ssk)->icsk_ulp_data, NULL);
>  	ctx->tcp_sock_destruct(ssk);
> +	if (ctx->conn)
> +		sock_put(ctx->conn);
>  	kfree_rcu(ctx, rcu);
>  }

[Severity: High]
The commit message says "Every reference on the subflow socket then also
keeps the parent alive". Does that hold for mptcp_subflow_drop_ctx()?
That function still drops the parent reference right away when the ULP
is attached:

net/mptcp/subflow.c:mptcp_subflow_drop_ctx() {
	...
	if (inet_csk(ssk)->icsk_ulp_ops) {
		subflow_ulp_fallback(ssk, ctx);
		if (ctx->conn)
			sock_put(ctx->conn);
	...
}

In that branch the TCP socket stays hashed as plain TCP, so a reference
on it no longer pins the msk. bpf_mptcp_sock_from_subflow() returns
ctx->conn without taking a reference of its own.

One way to reach it is closing a listener that has an unaccepted
connection:

mptcp_check_listen_stop()
  mptcp_subflow_queue_clean()
    __mptcp_unaccepted_force_close(msk)
      mptcp_do_fastclose(msk)        <- only sends RST, child stays in ehash
      __mptcp_destroy_sock(msk)
        mptcp_destroy()              <- sets free_first
          mptcp_destroy_common()
            __mptcp_close_ssk(first) <- in_accept_queue branch
              mptcp_subflow_drop_ctx(first)
                sock_put(msk)
        sock_put(msk)
    mptcp_cancel_work(msk)
    sock_put(msk)                    <- msk freed

Suppose a TC or SK_SKB program on another CPU called bpf_sk_lookup_tcp()
on that 4-tuple, then bpf_skc_to_mptcp_sock(), before the fallback. It
holds only the TCP reference. Wouldn't it then read the freed msk, the
same way as in the first KASAN report?

The dispose_child label in subflow_syn_recv_sock() looks like a second
path. It is reached after:

	ctx->conn = (struct sock *)owner;

and by then tcp_v4_syn_recv_sock() has already hashed the child.

Does this put also need to be deferred, for example past an RCU grace
period, or otherwise tied to the TCP socket's lifetime?

[Severity: Critical]
Can a TCP child cloned from a retired MPTCP listener subflow inherit
subflow_ctx_destruct()? If so, it would run this new sock_put() on the
listener's ctx.

The comment on mptcp_subflow_ctx_release() from the previous commit says
"TCP never clones such a socket". However, tcp_v4_rcv() checks the
listener state without holding the listener lock:

CPU1 (softirq)
tcp_v4_rcv()
  if (unlikely(sk->sk_state != TCP_LISTEN)) { ... }   <- passes
  sock_hold(sk)
  tcp_check_req()
    ->syn_recv_sock()
      inet_csk_clone_lock()
        sk_clone_lock()

CPU2
mptcp_close()
  mptcp_check_listen_stop()
  __mptcp_destroy_sock()
    mptcp_destroy()                  <- free_first = 1
      __mptcp_close_ssk(listener ssk)
        __tcp_close()
          inet_csk_destroy_sock()
            tcp_cleanup_ulp()
              subflow_ulp_release()  <- release == true
                mptcp_subflow_ctx_release()
                  ssk->sk_destruct = subflow_ctx_destruct;
              icsk->icsk_ulp_ops = NULL;

tcp_cleanup_ulp() leaves icsk_ulp_data pointing at the listener's ctx.
Suppose CPU2 gets this far before sk_clone_lock() copies the listener:

- sk_clone_lock() keeps the inherited sk_destruct on success. It only
  clears it on the failure path.
- inet_clone_ulp() returns early because icsk_ulp_ops is NULL.

The child then has sk_destruct == subflow_ctx_destruct, icsk_ulp_data
pointing at the listener's ctx, and is_mptcp set.

inet_csk_reqsk_queue_add() then sees that the listener is no longer
TCP_LISTEN and calls inet_child_forget(). When the child is freed,
subflow_ctx_destruct(child) runs ctx->tcp_sock_destruct, then
sock_put(ctx->conn) on the listener msk, then kfree_rcu(ctx).

Later the listener ssk is destroyed, and subflow_ctx_destruct() runs
again on the same ctx. Wouldn't that call ctx->tcp_sock_destruct through
a pointer read from memory that may already be freed? It would also put
the listener msk a second time and kfree_rcu() the ctx twice.

The destructor inheritance comes from the previous commit in this series.
This patch adds the extra put on the listener msk on top of it.

[ ... ]

> @@ -2120,7 +2124,6 @@ static void subflow_ulp_release(struct sock *ssk)
>  		if (!release && !test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW,
>  						  &mptcp_sk(sk)->flags))
>  			mptcp_schedule_work(sk);
> -		sock_put(sk);
>  	}
>
>  	mptcp_subflow_ops_undo_override(ssk);

[Severity: Low]
This isn't a bug, but the comment a few lines above in
subflow_ulp_release() still says:

	/* if the msk has been orphaned, keep the ctx
	 * alive, will be freed by __mptcp_close_ssk(),
	 * when the subflow is still unaccepted
	 */

After this series, __mptcp_close_ssk() only retires the context through
mptcp_subflow_ctx_release(). The context is freed in subflow_ctx_destruct()
when the subflow socket is destroyed, and with this patch the parent
reference is dropped there too.

Should this comment be updated?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/pm-mptcp-objects-candidate-0010-v3-14c229ea28a7d31f60a0-0%40gmail.com