Re: [PATCH ipsec] xfrm: espintcp: reorder operations within espintcp_init_sk

[netdev-bot+sashiko@kernel.org]

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 does not fully close the race it targets.

--

xfrm: espintcp: reorder operations within espintcp_init_sk

This commit reorders espintcp_init_sk() so the espintcp_ctx is fully set
up and published in icsk_ulp_data before the socket callbacks, sk_prot and
socket ops are switched to the espintcp versions. It relies on the barrier
in rcu_assign_pointer() so that callers always find a valid ctx.

Link: https://lore.kernel.org/all/c30b645074a1b379e0f7fe297f917c66137d9964.1778464688.git.zylzyl2333@gmail.com/
Link: https://lore.kernel.org/all/20260819155349.3555804-1-jjy600901@snu.ac.kr

> diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
> index 3e72b9f067b9..1087a8dd94f4 100644
> --- a/net/xfrm/espintcp.c
> +++ b/net/xfrm/espintcp.c
> @@ -466,9 +466,23 @@ static int espintcp_init_sk(struct sock *sk)

[ ... ]

> +	rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
> +
> +	sk->sk_data_ready = espintcp_data_ready;
> +	sk->sk_write_space = espintcp_write_space;
> +	sk->sk_destruct = espintcp_destruct;
>  
>  	if (sk->sk_family == AF_INET) {
>  		sk->sk_prot = &espintcp_prot;

[Severity: High]
Does rcu_assign_pointer() actually order the stores that come after it?
The commit message says:

  "rely on the barrier provided by rcu_assign_pointer to ensure callers
  will have a struct espintcp_ctx available."

rcu_assign_pointer() is smp_store_release(), and a release store only
orders the accesses before it. The plain stores after it are not ordered
after the publish. These are sk_data_ready, sk_write_space, sk_destruct,
sk->sk_prot and sk->sk_socket->ops.

On arm64, POWER or RISC-V these stores can become visible before
icsk_ulp_data. On any architecture, the compiler is free to move them above
the release store.

The read side has no ordering either. espintcp_getctx() uses a plain load:

include/net/espintcp.h:espintcp_getctx() {
	/* RCU is only needed for diag */
	return (__force void *)icsk->icsk_ulp_data;
}

Readers reach this load through an indirect call after reading ops or prot
with READ_ONCE() at most. That gives only a control dependency, which does
not order one load after another.

The writer holds only the socket lock:

do_tcp_setsockopt(TCP_ULP)->sockopt_lock_sock()->tcp_set_ulp()->
    __tcp_set_ulp()->espintcp_init_sk()

Several readers never take that lock:

sock_poll()->espintcp_poll()
    return datagram_poll_queue(file, sock, wait, &ctx->ike_queue);

inet_recvmsg()->espintcp_recvmsg()
    skb = __skb_recv_datagram(sk, &ctx->ike_queue, flags, &off, &err);

inet_sendmsg()->espintcp_sendmsg(), before lock_sock():
    struct espintcp_ctx *ctx = espintcp_getctx(sk);
    struct espintcp_msg *emsg = &ctx->partial;

esp_output_tcp_finish() in net/ipv4/esp4.c (and the same code in
net/ipv6/esp6.c) checks tcp_is_ulp_esp(), which is a plain sk_prot load.
It then calls espintcp_queue_out() or espintcp_push_skb(), and both
dereference ctx.

Suppose one thread runs setsockopt(fd, SOL_TCP, TCP_ULP, "espintcp") while
another calls poll(), recvmsg() or sendmsg() on the same fd. Can the second
thread still see the espintcp ops or prot with ctx still NULL, and oops on
a near-NULL dereference? TCP_ULP needs no capability, so an unprivileged
local user can reach this. The window is narrow. On x86, if the compiler
does not reorder the stores, the new order is enough.

tls_ctx_create() in net/tls/tls_main.c uses a similar pattern, but it pairs
its sk_prot stores with WRITE_ONCE().

Would a complete fix need ordering on both sides? On the write side, that
means an smp_wmb() after publishing ctx, or WRITE_ONCE()/release stores for
the callbacks, sk_prot and ops. On the read side, it means
rcu_dereference() or an acquire load in espintcp_getctx() and
tcp_is_ulp_esp(), or NULL checks on the lockless paths. Otherwise, should
the commit message and the comment in espintcp_getctx() drop the claim
that rcu_assign_pointer() gives this guarantee?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/dcb164e6a1064fcd32c2e72fd9ee918dd1427f15.1790617484.git.sd%40queasysnail.net