Re: [PATCH 04/14] xfrm: espintcp: reorder operations within espintcp_init_sk

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch annotates only part of the concurrent access to
  sk->sk_prot.

Pre-existing issues:
- [Critical] esp_find_tcp_sk() (net/ipv4/esp4.c:138-145) and
  esp6_find_tcp_sk() (net/ipv6/esp6.c:155-162) drop the lookup reference…
- [Medium] espintcp_close() (net/xfrm/espintcp.c:536) always restores
  `sk->sk_prot = &tcp_prot`, even for AF_INET6 sockets.
- [Medium] On the AF_INET6 path, the first espintcp_init_sk() call after
  boot fills the static espintcp6_prot/espintcp6_ops with…

--

xfrm: espintcp: reorder operations within espintcp_init_sk

This commit reorders espintcp_init_sk() so the espintcp_ctx is fully
set up and published as icsk_ulp_data before the socket callbacks and
proto/ops are switched to the espintcp versions. An smp_wmb()/smp_rmb()
pair is added between espintcp_init_sk() and espintcp_getctx(), along
with some READ_ONCE/WRITE_ONCE annotations on sk_prot, sk_socket->ops
and the callbacks.

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 3e72b9f067b9b..4052ae5bbb2ea 100644
> --- a/net/xfrm/espintcp.c
> +++ b/net/xfrm/espintcp.c
> @@ -434,7 +434,9 @@ static void espintcp_destruct(struct sock *sk)
>  
>  bool tcp_is_ulp_esp(struct sock *sk)
>  {
> -	return sk->sk_prot == &espintcp_prot || sk->sk_prot == &espintcp6_prot;
> +	const struct proto *prot = READ_ONCE(sk->sk_prot);
> +
> +	return prot == &espintcp_prot || prot == &espintcp6_prot;
>  }

[Severity: Low]

tcp_is_ulp_esp() now reads sk->sk_prot with READ_ONCE(), and
espintcp_init_sk() installs it with WRITE_ONCE(). Should the restore in
espintcp_close() get the same treatment?

net/xfrm/espintcp.c:espintcp_close() {
	...
	strp_stop(&ctx->strp);

	sk->sk_prot = &tcp_prot;

	synchronize_rcu();
	...
}

This plain store is the one that actually runs at the same time as the
lockless reader. espintcp_close() is reached through
inet_release()->sk->sk_prot->close() without lock_sock. esp_find_tcp_sk()
and esp6_find_tcp_sk() call tcp_is_ulp_esp() holding only
rcu_read_lock(). The synchronize_rcu() right after the store exists
because of those readers.

The TLS ULP marks its restore path with WRITE_ONCE(sk->sk_prot,
ctx->sk_proto) in net/tls/tls_main.c. Would WRITE_ONCE(sk->sk_prot,
&tcp_prot) in espintcp_close() complete the annotation?

[Severity: Critical]

This is a pre-existing issue, but since this patch touches
tcp_is_ulp_esp(): can its callers call sock_put() on a minisocket?

net/ipv4/esp4.c:esp_find_tcp_sk() {
	...
	sk = inet_lookup_established(net, x->id.daddr.a4, dport,
				     x->props.saddr.a4, sport, 0);
	if (!sk)
		return ERR_PTR(-ENOENT);

	if (!tcp_is_ulp_esp(sk)) {
		sock_put(sk);
		return ERR_PTR(-EINVAL);
	}
	...
}

esp6_find_tcp_sk() in net/ipv6/esp6.c follows the same pattern.

__inet_lookup_established() walks the ehash chain, which also contains
TIME_WAIT and NEW_SYN_RECV sockets, and takes a reference with
refcount_inc_not_zero(). Its own mismatch path drops that reference
with sock_gen_put().

A timewait socket's tw_prot is sk_prot_creator (tcp_prot or
tcpv6_prot), so tcp_is_ulp_esp() always returns false for it and the
sock_put() path runs.

Here is one possible sequence. The espintcp socket was actively closed,
so its 4-tuple is in TIME_WAIT with tw_refcnt == 3:

CPU1 (ESP output)
esp_output_tcp_finish()
  esp_find_tcp_sk()
    __inet_lookup_established()
      refcount_inc_not_zero()        /* 3 -> 4 */

CPU2 (tw timer)
tw_timer_handler()
  inet_twsk_kill()
    sk_nulls_del_node_init_rcu()     /* 4 -> 3 */
    inet_twsk_bind_unhash()          /* 3 -> 2 */
    inet_twsk_put()                  /* 2 -> 1 */

CPU1
    tcp_is_ulp_esp(tw) returns false
    sock_put()                       /* 1 -> 0, sk_free(tw) */

sk_free() then decrements sk_wmem_alloc and may run __sk_free() on
memory that is not a full struct sock. inet_twsk_free() is skipped.

A NEW_SYN_RECV request socket on the same 4-tuple, which a remote SYN
can create, would hit the same wrong-destructor path.

Should esp_find_tcp_sk() and esp6_find_tcp_sk() use sock_gen_put()
here instead?

>  EXPORT_SYMBOL_GPL(tcp_is_ulp_esp);
>  
> @@ -466,34 +468,41 @@ static int espintcp_init_sk(struct sock *sk)

[ ... ]

> +	rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
> +
> +	/* pairs with smp_rmb() in espintcp_getctx() */
> +	smp_wmb();
> +
> +	WRITE_ONCE(sk->sk_data_ready, espintcp_data_ready);
> +	WRITE_ONCE(sk->sk_write_space, espintcp_write_space);
> +	sk->sk_destruct = espintcp_destruct;
>  
>  	if (sk->sk_family == AF_INET) {
> -		sk->sk_prot = &espintcp_prot;
> -		sk->sk_socket->ops = &espintcp_ops;
> +		WRITE_ONCE(sk->sk_prot, &espintcp_prot);
> +		WRITE_ONCE(sk->sk_socket->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);
>  		mutex_unlock(&tcpv6_prot_mutex);
>  
> -		sk->sk_prot = &espintcp6_prot;
> -		sk->sk_socket->ops = &espintcp6_ops;
> +		WRITE_ONCE(sk->sk_prot, &espintcp6_prot);
> +		WRITE_ONCE(sk->sk_socket->ops, &espintcp6_ops);
>  	}

[Severity: Medium]

This is a pre-existing issue, but the new smp_wmb() comes before the
AF_INET6 branch. On the first IPv6 espintcp conversion after boot,
build_protos() fills espintcp6_prot and espintcp6_ops using memcpy()
and plain stores. The two WRITE_ONCE() calls above then publish them.

Only mutex_unlock() sits between the two sets of stores. It is a
release, so it doesn't order the later publication stores after the
earlier build_protos() stores.

Can another thread on the same socket see the new pointer before the
struct contents are visible?

sock_recvmsg_nosec() calls READ_ONCE(sock->ops)->recvmsg. inet6_recvmsg()
and inet6_sendmsg() call prot->recvmsg/sendmsg after
READ_ONCE(sk->sk_prot). Neither takes lock_sock. If one of them sees a
still-zero ->recvmsg or ->sendmsg, it calls a NULL function pointer.

Would moving the smp_wmb() after the AF_INET6 build block, or
publishing with smp_store_release(), order both the ctx and the proto
contents?

[Severity: Medium]

This isn't a bug introduced by this patch, but for AF_INET6 sockets
this branch installs espintcp6_prot, which is copied from tcpv6_prot.
espintcp_close() always restores the IPv4 proto:

net/xfrm/espintcp.c:espintcp_close() {
	...
	sk->sk_prot = &tcp_prot;
	...
}

struct espintcp_ctx doesn't save the original proto, so close cannot
restore the per-family one. Does an IPv6 espintcp socket then run
tcp_close() with tcp_prot?

inet_unhash() and inet_twsk_hashdance_schedule() both call:

	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);

The socket was counted under tcpv6_prot's inuse_idx, which
espintcp6_prot copies, but it gets decremented on tcp_prot's. "TCP:
inuse" in /proc/net/sockstat would be undercounted and "TCP6: inuse"
would leak.

Could the ctx save the original sk_prot, as tls_context->sk_proto
does, and restore it in espintcp_close()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com