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

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

[Sabrina Dubroca]

When enabled on a socket, espintcp sets the socket callbacks to its
own before it has finished setting up its context and published it as
icsk_ulp_data. Any one of those callbacks that gets called before will
dereference a NULL icsk_ulp_data.

Fix this by reording the operations, and rely on the barrier provided
by rcu_assign_pointer to ensure callers will have a struct
espintcp_ctx available.

Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Cc: stable@vger.kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Reported-by: Peihan Liu <ronbogo@outlook.com>
Reported-by: Yilin Zhu <zylzyl2333@gmail.com>
Reported-by: Ren Wei <n05ec@lzu.edu.cn>
Reported-by: Eulgyu Kim <eulgyukim@snu.ac.kr>
Reported-by: Jaeyoung Chung <jjy600901@snu.ac.kr>
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
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/espintcp.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

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)
 
 	__sk_dst_reset(sk);
 
-	strp_check_rcv(&ctx->strp);
 	skb_queue_head_init(&ctx->ike_queue);
 	skb_queue_head_init(&ctx->out_queue);
+	INIT_WORK(&ctx->work, espintcp_tx_work);
+
+	ctx->saved_data_ready = sk->sk_data_ready;
+	ctx->saved_write_space = sk->sk_write_space;
+	ctx->saved_destruct = sk->sk_destruct;
+
+	/* avoid using task_frag */
+	sk->sk_allocation = GFP_ATOMIC;
+	sk->sk_use_task_frag = false;
+
+	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;
@@ -482,18 +496,8 @@ static int espintcp_init_sk(struct sock *sk)
 		sk->sk_prot = &espintcp6_prot;
 		sk->sk_socket->ops = &espintcp6_ops;
 	}
-	ctx->saved_data_ready = sk->sk_data_ready;
-	ctx->saved_write_space = sk->sk_write_space;
-	ctx->saved_destruct = sk->sk_destruct;
-	sk->sk_data_ready = espintcp_data_ready;
-	sk->sk_write_space = espintcp_write_space;
-	sk->sk_destruct = espintcp_destruct;
-	rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
-	INIT_WORK(&ctx->work, espintcp_tx_work);
 
-	/* avoid using task_frag */
-	sk->sk_allocation = GFP_ATOMIC;
-	sk->sk_use_task_frag = false;
+	strp_check_rcv(&ctx->strp);
 
 	return 0;
 
-- 
2.55.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[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 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

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

[Steffen Klassert]

On Thu, Oct 01, 2026 at 12:55:11PM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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.

Sabrina, do you plan to update this one?

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

[Sabrina Dubroca]

2026-10-07, 08:48:28 +0200, Steffen Klassert wrote:
> On Thu, Oct 01, 2026 at 12:55:11PM +0000, netdev-bot+sashiko@kernel.org wrote:
> > 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
> > 
[...]
> Sabrina, do you plan to update this one?

Sorry Steffen, too much stuff happening at the same time and this one
was a bit complex. Let's see if the AI is happy with v2.

-- 
Sabrina