On Tue, Dec 24, 2024 at 03:16 PM +08, Jiayuan Chen wrote: > On Mon, Dec 23, 2024 at 11:57:58PM +0800, Jakub Sitnicki wrote: >> On Mon, Dec 23, 2024 at 09:57 PM +01, Jakub Sitnicki wrote: >> > On Thu, Dec 19, 2024 at 05:30 PM +08, Jiayuan Chen wrote: >> >> Currently, not all modules using strparser have issues with >> >> copied_seq miscalculation. The issue exists mainly with >> >> bpf::sockmap + strparser because bpf::sockmap implements a >> >> proprietary read interface for user-land: tcp_bpf_recvmsg_parser(). >> >> >> >> Both this and strp_recv->tcp_read_sock update copied_seq, leading >> >> to errors. >> >> >> >> This is why I rewrote the tcp_read_sock() interface specifically for >> >> bpf::sockmap. >> > >> > All right. Looks like reusing read_skb is not going to pan out. >> > >> > But I think we should not give up just yet. It's easy to add new code. >> > >> > We can try to break up and parametrize tcp_read_sock - if other >> > maintainers are not against it. Does something like this work for you? >> > >> > https://github.com/jsitnicki/linux/commits/review/stp-copied_seq/idea-2/ >> >> Actually it reads better if we just add early bailout to tcp_read_sock: >> >> https://github.com/jsitnicki/linux/commits/review/stp-copied_seq/idea-2.1/ >> >> ---8<--- >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index 6a07d98017f7..6564ea3b6cd4 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -1565,12 +1565,13 @@ EXPORT_SYMBOL(tcp_recv_skb); >> * or for 'peeking' the socket using this routine >> * (although both would be easy to implement). >> */ >> -int tcp_read_sock(struct sock *sk, read_descriptor_t *desc, >> - sk_read_actor_t recv_actor) >> +static inline int __tcp_read_sock(struct sock *sk, read_descriptor_t *desc, >> + sk_read_actor_t recv_actor, bool noack, >> + u32 *copied_seq) >> { >> struct sk_buff *skb; >> struct tcp_sock *tp = tcp_sk(sk); >> - u32 seq = tp->copied_seq; >> + u32 seq = *copied_seq; >> u32 offset; >> int copied = 0; >> >> @@ -1624,9 +1625,12 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc, >> tcp_eat_recv_skb(sk, skb); >> if (!desc->count) >> break; >> - WRITE_ONCE(tp->copied_seq, seq); >> + WRITE_ONCE(*copied_seq, seq); >> } >> - WRITE_ONCE(tp->copied_seq, seq); >> + WRITE_ONCE(*copied_seq, seq); >> + >> + if (noack) >> + goto out; >> >> tcp_rcv_space_adjust(sk); >> >> @@ -1635,10 +1639,25 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc, >> tcp_recv_skb(sk, seq, &offset); >> tcp_cleanup_rbuf(sk, copied); >> } >> +out: >> return copied; >> } >> + >> +int tcp_read_sock(struct sock *sk, read_descriptor_t *desc, >> + sk_read_actor_t recv_actor) >> +{ >> + return __tcp_read_sock(sk, desc, recv_actor, false, >> + &tcp_sk(sk)->copied_seq); >> +} >> EXPORT_SYMBOL(tcp_read_sock); >> >> +int tcp_read_sock_noack(struct sock *sk, read_descriptor_t *desc, >> + sk_read_actor_t recv_actor, u32 *copied_seq) >> +{ >> + return __tcp_read_sock(sk, desc, recv_actor, true, copied_seq); >> +} >> +EXPORT_SYMBOL(tcp_read_sock_noack); >> + >> int tcp_read_skb(struct sock *sk, skb_read_actor_t recv_actor) >> { >> struct sk_buff *skb; > > This modification definitely reduces code duplication and makes it more > elegant compared to my previous idea. Also If we want to avoid modifying > the strp code and not introduce new ops, perhaps we could revert to the > simplest solution: > ''' > void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock) > { > ... > sk->sk_data_ready = sk_psock_strp_data_ready; > /* Replacement */ > psock->saved_read_sock = sk->sk_socket->ops->read_sock; > sk->sk_socket->ops->read_sock = tcp_read_sock_noack; > } > ''' > If acceptable, I can incorporate this approach in the next patch version. SGTM
