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

[Steffen Klassert <steffen.klassert@secunet.com>]

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?