Re: [PATCH v2] tcp: add support for SO_PEEK_OFF

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2024-02-08 at 09:50 -0500, Jon Maloy wrote:
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index fce5668a6a3d..ad03e0cee3c1 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -863,6 +863,14 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
>  }
>  EXPORT_SYMBOL(tcp_splice_read);
>  
> +int tcp_set_peek_offset(struct sock *sk, int val)
> +{
> +	WRITE_ONCE(sk->sk_peek_off, val);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tcp_set_peek_offset);

this looks equal to sk_set_peek_off. why not reusing the latter?

[...]
> @@ -2317,6 +2323,7 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len,
>  	long timeo;
>  	struct sk_buff *skb, *last;
>  	u32 urg_hole = 0;
> +	u32 peek_offset = 0;

Very minor nit: the variable definition should follow the reverse xmas
tree order. Here such order is already broken, but I still place this
definition just before 'urg_hole'.

>  	err = -ENOTCONN;
>  	if (sk->sk_state == TCP_LISTEN)
> @@ -2774,6 +2785,7 @@ void __tcp_close(struct sock *sk, long timeout)
>  		data_was_unread += len;
>  		__kfree_skb(skb);
>  	}
> +	sk_set_peek_off(sk, -1);

Why are you resetting the peek offset at close time? nobody can read or
poll the socket at this point.

You should instead reset it at tcp_disconnect() time.

>  	/* If socket has been already reset (e.g. in tcp_reset()) - kill it. */
>  	if (sk->sk_state == TCP_CLOSE)
> @@ -4492,7 +4504,7 @@ void tcp_done(struct sock *sk)
>  		reqsk_fastopen_remove(sk, req, false);
>  
>  	WRITE_ONCE(sk->sk_shutdown, SHUTDOWN_MASK);
> -
> +	sk_set_peek_off(sk, -1);

Similar question here. tcp_done is called e.g. after an incoming
reset() and the socket is still readable after an incoming reset, if
there is pending data in the receive buffer.

Resetting the peek offset could produce unexpected results for the
reader.

Cheers,

Paolo