Re: [RFC PATCH net-next 01/12] net: add an exclusive TCP receive claim

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] tcp: returning -EBUSY in tcp_disconnect() breaks internal
  callers
- [Medium] net: missing sock_rx_owned() checks in kcm_attach() and
  ovpn_tcp_socket_attach()
--

commit db5306ce28288ea69fe78b0895ec7be46b42bf0b
Author: Alex Markuze <amarkuze@redhat.com>

net: add an exclusive TCP receive claim

This commit adds an optional sk_rx_owner pointer to struct sock so a
tcp_read_sock() consumer can claim exclusive access to a TCP socket's
receive sequence and reject competing readers or operations that reset the
receive stream.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index df385a5a961e8..5c753bcb808c5 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -754,6 +754,10 @@ struct sk_psock *sk_psock_init(struct sock *sk, int node)
>  
>  	write_lock_bh(&sk->sk_callback_lock);
>  
> +	if (sock_rx_owned(sk)) {
> +		psock = ERR_PTR(-EBUSY);
> +		goto out;
> +	}
>  	if (sk_is_inet(sk) && inet_csk_has_ulp(sk)) {
>  		psock = ERR_PTR(-EINVAL);
>  		goto out;

[Severity: Medium]
Should kcm_attach() and ovpn_tcp_socket_attach() also check
sock_rx_owned()?

Like sk_psock_init(), both attach to a TCP socket by checking only
sk->sk_user_data, which remains NULL when sk_rx_owner is set.  Attaching
via kcm_attach() installs a strparser and replaces sk->sk_state_change
with psock_state_change(), which sets sk->sk_err = EPIPE on a normal peer
FIN and causes the receive owner to fail with -EPIPE instead of clean EOF.
Attaching via ovpn_tcp_socket_attach() replaces sk->sk_prot and
sk->sk_socket->ops with ovpn_tcp_recvmsg(), bypassing the sock_rx_owned()
checks.

> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 7677b0343abd4..54c30a59bbce4 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -649,6 +649,8 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr_unsized *uaddr,
>  			return -EINVAL;
>  
>  		if (uaddr->sa_family == AF_UNSPEC) {
> +			if (sock_rx_owned(sk))
> +				return -EBUSY;
>  			sk->sk_disconnects++;
>  			err = sk->sk_prot->disconnect(sk, flags);
>  			sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index b9de7c302ba83..6de6ce153fda1 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c

[ ... ]

> @@ -3374,6 +3389,9 @@ int tcp_disconnect(struct sock *sk, int flags)
>  	struct request_sock *req;
>  	u32 seq;
>  
> +	if (sock_rx_owned(sk))
> +		return -EBUSY;
> +
>  	if (old_state != TCP_CLOSE)
>  		tcp_set_state(sk, TCP_CLOSE);
>  

[Severity: Medium]
Does returning -EBUSY from tcp_disconnect() break internal callers of
sk->sk_prot->disconnect()?

__inet_stream_connect() already rejects connect(AF_UNSPEC) above before
calling sk->sk_prot->disconnect().  Other callers expect tcp_disconnect()
to succeed unconditionally:

net/ipv4/af_inet.c:__inet_stream_connect() {
    ...
sock_error:
	err = sock_error(sk) ? : -ECONNABORTED;
	sock->state = SS_UNCONNECTED;
	sk->sk_disconnects++;
	if (sk->sk_prot->disconnect(sk, flags))
		sock->state = SS_DISCONNECTING;
	goto out;
}

When a socket connects via non-blocking connect(), sock->state stays
SS_CONNECTING after the handshake reaches TCP_ESTABLISHED.  If a receive
owner is attached and the connection is then closed by a peer RST or
timeout (moving sk->sk_state to TCP_CLOSE while sk_rx_owner is still set),
a subsequent connect() jumps to sock_error:, where disconnect() returns
-EBUSY without cleaning up the socket and leaves sock->state permanently
wedged in SS_DISCONNECTING even after the owner detaches.

__tcp_close() also ignores the return value of sk->sk_prot->disconnect(sk,
0) on a zero-linger close and assumes it moves sk->sk_state to TCP_CLOSE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110132006.3280986-1-amarkuze@redhat.com?part=1