RE: [PATCH bpf v2 1/2] bpf: fix wrong copied_seq calculation

[John Fastabend <john.fastabend@gmail.com>]

Jiayuan Chen wrote:
> 'sk->copied_seq' was updated in the tcp_eat_skb() function when the
> action of a BPF program was SK_REDIRECT. For other actions, like SK_PASS,
> the update logic for 'sk->copied_seq' was moved to
> tcp_bpf_recvmsg_parser() to ensure the accuracy of the 'fionread' feature.
> 
> It works for a single stream_verdict scenario, as it also modified
> 'sk_data_ready->sk_psock_verdict_data_ready->tcp_read_skb'
> to remove updating 'sk->copied_seq'.
> 
> However, for programs where both stream_parser and stream_verdict are
> active(strparser purpose), tcp_read_sock() was used instead of
> tcp_read_skb() (sk_data_ready->strp_data_ready->tcp_read_sock)
> tcp_read_sock() now still update 'sk->copied_seq', leading to duplicated
> updates.
> 
> In summary, for strparser + SK_PASS, copied_seq is redundantly calculated
> in both tcp_read_sock() and tcp_bpf_recvmsg_parser().
> 
> The issue causes incorrect copied_seq calculations, which prevent
> correct data reads from the recv() interface in user-land.
> 
> Modifying tcp_read_sock() or strparser implementation directly is
> unreasonable, as it is widely used in other modules.
> 
> Here, we introduce a method tcp_bpf_read_sock() to replace
> 'sk->sk_socket->ops->read_sock' (like 'tls_build_proto()' does in
> tls_main.c). Such replacement action was also used in updating
> tcp_bpf_prots in tcp_bpf.c, so it's not weird.
> (Note that checkpatch.pl may complain missing 'const' qualifier when we
> define the bpf-specified 'proto_ops', but we have to do because we need
> update it).
> 
> Also we remove strparser check in tcp_eat_skb() since we implement custom
> function tcp_bpf_read_sock() without copied_seq updating.
> 
> Since strparser currently supports only TCP, it's sufficient for 'ops' to
> inherit inet_stream_ops.
> 
> In strparser's implementation, regardless of partial or full reads,
> it completely clones the entire skb, allowing us to unconditionally
> free skb in tcp_bpf_read_sock().
> 
> Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")
> Signed-off-by: Jiayuan Chen <mrpre@163.com>

[...]

> +/* The tcp_bpf_read_sock() is an alternative implementation
> + * of tcp_read_sock(), except that it does not update copied_seq.
> + */
> +static int tcp_bpf_read_sock(struct sock *sk, read_descriptor_t *desc,
> +			     sk_read_actor_t recv_actor)
> +{
> +	struct sk_buff *skb;
> +	int copied = 0;
> +
> +	if (sk->sk_state == TCP_LISTEN)
> +		return -ENOTCONN;
> +
> +	while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
> +		u8 tcp_flags;
> +		int used;
> +
> +		WARN_ON_ONCE(!skb_set_owner_sk_safe(skb, sk));
> +		tcp_flags = TCP_SKB_CB(skb)->tcp_flags;
> +		used = recv_actor(desc, skb, 0, skb->len);

Here the skb is still on the receive_queue how does this work with
tcp_try_coalesce()? So I believe you need to unlink before you
call the actor which creates a bit of trouble if recv_actor
doesn't want the entire skb.  

I think easier is to do similar logic to read_sock and track
offset and len? Did I miss something.

> +		/* strparser clone and consume all input skb
> +		 * even in waiting head or body status
> +		 */
> +		tcp_eat_recv_skb(sk, skb);
> +		if (used <= 0) {
> +			if (!copied)
> +				copied = used;
> +			break;
> +		}
> +		copied += used;
> +		if (!desc->count)
> +			break;
> +		if (tcp_flags & TCPHDR_FIN)
> +			break;
> +	}
> +	return copied;
> +}
> +
>  enum {
>  	TCP_BPF_IPV4,
>  	TCP_BPF_IPV6,
> @@ -595,6 +636,10 @@ enum {