Message-ID: <f2pur5raimm5y3phmtwubf6yf3sniphwgql4c4k7md25lxcehm@3qwyp4zibnrd>
Date: Thu, 12 Dec 2024 14:33:43 +0800
From: Jiayuan Chen <mrpre@....com>
To: John Fastabend <john.fastabend@...il.com>
Cc: bpf@...r.kernel.org, martin.lau@...ux.dev, ast@...nel.org, 
	edumazet@...gle.com, jakub@...udflare.com, davem@...emloft.net, dsahern@...nel.org, 
	kuba@...nel.org, pabeni@...hat.com, linux-kernel@...r.kernel.org, song@...nel.org, 
	andrii@...nel.org, mhal@...x.co, yonghong.song@...ux.dev, daniel@...earbox.net, 
	xiyou.wangcong@...il.com, horms@...nel.org
Subject: Re: [PATCH bpf v2 1/2] bpf: fix wrong copied_seq calculation

On Tue, Dec 10, 2024 at 06:11:26PM -0800, John Fastabend wrote:
> 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@....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.

Thanks to John Fastabend.

Let me explain it.
Now I only replace the read_sock handler when using strparser.

My previous implementation added the replacement of read_sock in
sk_psock_start_strp() to more explicitly require replacement when using
strparser, for instance:
'''skmsg.c
void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
{
    ...
    sk->sk_data_ready = sk_psock_strp_data_ready;
    /* Replacement */
    sk->sk_socket->ops->read_sock = tcp_bpf_read_sock;
}
'''

As you can see that it only works for strparser.
(The current implementation of replacement in tcp_bpf_update_proto()
achieves the same effect just not as obviously.)

So the current implementation of recv_actor() can only be strp_recv(),
with the call stack as follows:
'''
sk_psock_strp_data_ready
  -> strp_data_ready
    -> strp_read_sock
      -> strp_recv
'''

The implementation of strp_recv() will consume all input skb. Even if it
reads part of the data, it will clone it, then place it into its own
queue, expecting the caller to release the skb. I didn't find any
logic like tcp_try_coalesce() (fix me if i miss something).

The record of the 'offset' is stored within its own context(strparser/_strp_msg).
With all skbs and offset saved in strp context, the caller does not need to
maintain it.

I have also added various logic tests for this situation in the test
case, and it works correctly. 
> > +		/* 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 {

Powered by blists - more mailing lists