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

[John Fastabend]

Jiayuan Chen wrote:
> On Thu, Dec 12, 2024 at 05:36:15PM -0800, John Fastabend wrote:
> [...]
> > > > 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).
> > 
> > 
> > So here is what I believe the flow is,
> > 
> > sk_psock_strp_data_ready
> >   -> str_data_ready
> >      -> strp_read_sock
> >         -> sock->ops->read_sock(.., strp_recv)
> > 
> > 
> > We both have the same idea up to here. But then the proposed data_ready()
> > call
> > 
> > +	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);
> > 
> > The recv_actor here is strp_recv() all good so far. But, because
> > that skb is still on the sk_receive_queue() the TCP stack may
> > at the same time do
> > 
> >  tcp_data_queue
> >   -> tcp_queue_rcv
> >      -> tail = skb_peek_tail(&sk->sk_receive_queue);
> >         tcp_try_coalesce(sk, tail, skb, fragstolen)
> >          -> skb_try_coalesce()
> >             ... skb->len += len
> > 
> > So among other things you will have changed the skb->len and added some
> > data to it. If this happens while you are running the recv actor we will
> > eat the data by calling tcp_eat_recv_skb(). Any data added from the
> > try_coalesce will just be dropped and never handled? The clone() from
> > the strparser side doesn't help you the tcp_eat_recv_skb call will
> > unlik the skb from the sk_receive_queue.
> > 
> > I don't think you have any way to protect this at the moment.
> 
> Thanks John Fastabend.
> 
> It seems sk was always locked whenever data_ready called.
> 
> '''
> bh_lock_sock_nested(sk)
> tcp_v4_do_rcv(sk)
>    |
>    |-> tcp_rcv_established
>    	|-> tcp_queue_rcv 
>    		|-> tcp_try_coalesce
>    |
>    |-> tcp_rcv_state_process
>    	|-> tcp_queue_rcv
>    		|-> tcp_try_coalesce
>    |
>    |-> sk->sk_data_ready()
> 
> bh_unlock_sock(sk)
> '''
> 
> other data_ready path:
> '''
> lock_sk(sk)
> sk->sk_data_ready()
> release_sock(sk)
> '''
> 
> I can not find any concurrency there. 

OK thanks, one more concern though. What if strp_recv thorws an ENOMEM
error on the clone? Would we just drop the data then? This is problem
not the expected behavior its already been ACKed.

Thanks,
John