Re: [PATCH net 1/1] xfrm: espintcp: publish ULP context before entry points

[Sabrina Dubroca <sd@queasysnail.net>]

Thanks for the fix. A small note: IPsec fixes go through the "ipsec"
tree, not "net", so the prefix should be [PATCH ipsec]

Some comments inline:

2026-05-11, 21:40:58 +0800, Ren Wei wrote:
> diff --git a/include/net/espintcp.h b/include/net/espintcp.h
> index c70efd704b6d..034be559786b 100644
> --- a/include/net/espintcp.h
> +++ b/include/net/espintcp.h
> @@ -34,7 +34,16 @@ static inline struct espintcp_ctx *espintcp_getctx(const struct sock *sk)
>  {
>  	const struct inet_connection_sock *icsk = inet_csk(sk);
>  
> -	/* RCU is only needed for diag */
> -	return (__force void *)icsk->icsk_ulp_data;
> +	/*
> +	 * The caller reached an ESP entry point by observing sk_prot,
> +	 * sk_socket->ops, or one of the socket callbacks.  Keep the ctx
> +	 * load after that observation so the caller cannot see the new
> +	 * entry point while still seeing stale icsk_ulp_data.

I don't think this comment is really helpful.

> +	 *
> +	 * Pairs with smp_wmb() in espintcp_init_sk().
> +	 */
> +	smp_rmb();
> +
> +	return (__force void *)READ_ONCE(icsk->icsk_ulp_data);

I think smp_store_release/smp_load_acquire is the "standard spelling"
for this now.


[...]
> @@ -472,34 +476,46 @@ 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);
> +	ctx->saved_data_ready = READ_ONCE(sk->sk_data_ready);
> +	ctx->saved_write_space = READ_ONCE(sk->sk_write_space);
> +	ctx->saved_destruct = READ_ONCE(sk->sk_destruct);

If something is changing those while espintcp_init_sk is running,
READ_ONCE won't help us. We'll end up with the wrong saved_*
values. Can this actually happen here?


> +	INIT_WORK(&ctx->work, espintcp_tx_work);
> +
> +	/* avoid using task_frag */
> +	sk->sk_allocation = GFP_ATOMIC;
> +	sk->sk_use_task_frag = false;
>  
>  	if (sk->sk_family == AF_INET) {
> -		sk->sk_prot = &espintcp_prot;
> -		sk->sk_socket->ops = &espintcp_ops;
> +		prot = &espintcp_prot;
> +		ops = &espintcp_ops;
>  	} else {
>  		mutex_lock(&tcpv6_prot_mutex);
>  		if (!espintcp6_prot.recvmsg)
> -			build_protos(&espintcp6_prot, &espintcp6_ops, sk->sk_prot, sk->sk_socket->ops);
> +			build_protos(&espintcp6_prot, &espintcp6_ops,
> +				     READ_ONCE(sk->sk_prot),
> +				     READ_ONCE(sk->sk_socket->ops));

And similar here. Those should always be tcpv6_prot/inet6_stream_ops,
but I wrote it this way to avoid having to use stubs, back when IPv6
could be built as a module. This could now be moved into espintcp_init
like the ipv4 variant of this.

>  		mutex_unlock(&tcpv6_prot_mutex);
>  
> -		sk->sk_prot = &espintcp6_prot;
> -		sk->sk_socket->ops = &espintcp6_ops;
> +		prot = &espintcp6_prot;
> +		ops = &espintcp6_ops;
>  	}

Or just move the whole block to the end, instead of introducing those
temporary variables?

> -	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;
> +	/*
> +	 * Publish the fully initialized ctx before publishing any entry point
> +	 * that can call espintcp_getctx().  The read barrier there runs after
> +	 * the caller has observed one of these pointers.
> +	 */
> +	smp_wmb();
> +	WRITE_ONCE(sk->sk_prot, prot);
> +	WRITE_ONCE(sk->sk_socket->ops, ops);
> +	WRITE_ONCE(sk->sk_data_ready, espintcp_data_ready);
> +	WRITE_ONCE(sk->sk_write_space, espintcp_write_space);
> +	WRITE_ONCE(sk->sk_destruct, espintcp_destruct);
> +
> +	strp_check_rcv(&ctx->strp);
>  
>  	return 0;
>  
> @@ -530,7 +546,7 @@ static void espintcp_close(struct sock *sk, long timeout)
>  
>  	strp_stop(&ctx->strp);
>  
> -	sk->sk_prot = &tcp_prot;
> +	WRITE_ONCE(sk->sk_prot, &tcp_prot);

Actually this should be the original sk_prot, which could be
tcpv6_prot.

I'm not sure how much the WRITE_ONCE matters here. What is it
protecting against/synchronizing with?

-- 
Sabrina