Re: [PATCH net] net/smc: prevent TCP from retaining freed address-family ops

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 (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] smc_listen() still installs &smc->af_ops on a clcsock that is
  already a live TCP listener, so an accepted child can keep the…
- [High] The new MPTCP gate in subflow_ulp_init() rejects exposed sockets
  only if it sees `sk->sk_socket->file != NULL`.

Pre-existing issues:
- [High] The problem wasn't introduced by this patch, but smc_listen()
  publishes &smc->af_ops with WRITE_ONCE() after plain stores to…

--

net/smc: prevent TCP from retaining freed address-family ops

This patch restores the original icsk_af_ops when smc_listen() fails and
in smc_clcsock_release(). Installing and restoring the ops is serialized
with the clcsock lock, and SOCK_RCU_FREE is now set before the listener
callbacks are published.

It also compares a child's ops against the embedded table in
smc_tcp_syn_recv_sock(). The MPTCP ULP can no longer be attached to a
kernel socket that has been exposed through a file.

Closes: https://lore.kernel.org/netdev/20260713085238.16780-1-samsun1006219@gmail.com/

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index f0a6725d2c37..da7ac71344a9 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1984,10 +1984,11 @@ static int subflow_ulp_init(struct sock *sk)
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	int err = 0;
>  
> -	/* disallow attaching ULP to a socket unless it has been
> -	 * created with sock_create_kern()
> +	/* Only attach to a kernel-created socket that has not been
> +	 * exposed through a file.
>  	 */
> -	if (!sk->sk_kern_sock) {
> +	if (!sk->sk_kern_sock ||
> +	    (sk->sk_socket && READ_ONCE(sk->sk_socket->file))) {

[Severity: High]

Does this check hold up against a concurrent smc_switch_to_fallback()?

smc_switch_to_fallback() exposes the clcsock with two plain stores and no
barrier between them:

net/smc/af_smc.c:smc_switch_to_fallback() {
    ...
	if (smc->sk.sk_socket && smc->sk.sk_socket->file) {
		smc->clcsock->file = smc->sk.sk_socket->file;
		smc->clcsock->file->private_data = smc->clcsock;
    ...
}

By then the file is already in the fd table. Another thread calling
setsockopt(fd, SOL_TCP, TCP_ULP, "mptcp") finds the clcsock through
sock_from_file(), which does a plain load of file->private_data. That
thread holds neither clcsock_release_lock nor the SMC sock lock.

The compiler may reorder those two stores, and so may arm64 or POWER. In
that case, can the second thread see private_data == clcsock while
clcsock->file still reads NULL?

The clcsock came from sock_create_kern(), so sk_kern_sock is 1 and both
checks would pass.

The MPTCP ULP would then attach to a TCP socket that userspace can reach
but that has no MPTCP parent, so ctx->conn is NULL. subflow_data_ready()
starts with:

	struct sock *parent = subflow->conn;

and subflow_state_change() also uses the parent. ctx->icsk_af_ops could
also capture the SMC-embedded table.

The commit message says:

    Reject attaching the MPTCP ULP to such a socket: otherwise userspace
    can bypass smc_setsockopt() and MPTCP can retain the embedded table in
    its subflow context.

Is that path still reachable through this window?

>  		err = -EOPNOTSUPP;
>  		goto out;
>  	}
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index e9f93b3ab435..dbbe7d6574e4 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c

[ ... ]

> @@ -2682,18 +2684,25 @@ int smc_listen(struct socket *sock, int backlog)
>  	write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
>  
>  	/* save original ops */
> +	lock_sock(smc->clcsock->sk);
>  	smc->ori_af_ops = inet_csk(smc->clcsock->sk)->icsk_af_ops;
>  
>  	smc->af_ops = *smc->ori_af_ops;
>  	smc->af_ops.syn_recv_sock = smc_tcp_syn_recv_sock;
>  
> -	inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
> +	WRITE_ONCE(inet_csk(smc->clcsock->sk)->icsk_af_ops, &smc->af_ops);
> +	release_sock(smc->clcsock->sk);

[Severity: High]

Can this still install &smc->af_ops on a clcsock that is already a live
TCP listener?

Here is one possible interleaving. Three threads share an AF_SMC fd that
is in SMC_INIT:

T1: listen(fd)
    smc_listen()
        lock_sock(sk)            <- waits

T2: setsockopt(fd, SOL_TCP, TCP_FASTOPEN, ...)
    smc_setsockopt()
        smc_switch_to_fallback()
            file->private_data = clcsock
    (sk_state stays SMC_INIT)

T3: listen(fd)
    inet_listen() on the clcsock, which is now a plain TCP listener

T1 then resumes and passes the state checks in smc_listen(), then
installs &smc->af_ops. kernel_listen() returns 0 because
__inet_listen_sk() only adjusts the backlog when the socket is already in
TCP_LISTEN.

The listener receive path runs without the socket lock:

tcp_v4_rcv()
    tcp_v4_do_rcv()          (TCP_LISTEN)
    tcp_check_req()          (TCP_NEW_SYN_RECV)
        inet_csk(sk)->icsk_af_ops->syn_recv_sock()

A softirq can load the original tcp_v4_syn_recv_sock() before the
WRITE_ONCE() above. It can then clone the listener in
tcp_create_openreq_child() after it.

That child inherits &smc->af_ops but never goes through
smc_tcp_syn_recv_sock(). So the new check there never runs for it:

    if (inet_csk(child)->icsk_af_ops == &smc->af_ops)

Suppose userspace then accepts that child from the clcsock and closes the
SMC fd. smc_clcsock_release() restores only the listener's ops.

Once the RCU grace period ends and the smc_sock is freed, would the child
keep calling through the freed table (net_header_len, queue_xmit, and so
on)?

Taking lock_sock() on the clcsock serializes install against restore, but
it doesn't seem to exclude lockless listener processing. Should the
install be refused when the clcsock is already in TCP_LISTEN?

[Severity: High]

This problem wasn't introduced by this patch, but is the publication of
&smc->af_ops ordered after the stores that initialize it?

smc->ori_af_ops and the struct copy into smc->af_ops are plain stores.
The WRITE_ONCE() that publishes &smc->af_ops has no release semantics.

In the already-listening case above, the lockless reader is:

tcp_v4_rcv()
    tcp_check_req()
        inet_csk(sk)->icsk_af_ops->syn_recv_sock(...)

The compiler may reorder the plain stores, and so may arm64 or POWER.
Could the reader then see the new pointer before the struct copy is
visible?

sk_alloc() zeroes smc->af_ops, so the softirq could call a NULL
syn_recv_sock. It could also enter smc_tcp_syn_recv_sock() and
dereference a NULL smc->ori_af_ops here:

	child = smc->ori_af_ops->syn_recv_sock(sk, skb, req, dst, req_unhash,

The added lock_sock() does not synchronize with this reader, which never
takes sk_lock. Refusing to install on a socket that is already listening,
or publishing with smp_store_release(), would avoid this.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928213153.36141-1-kylebot%40openai.com