Re: [PATCH bpf v9 1/5] strparser: add read_sock callback
On Wed, Jan 22, 2025 at 06:09 PM +08, Jiayuan Chen wrote: > Added a new read_sock handler, allowing users to customize read operations > instead of relying on the native socket's read_sock. > > Signed-off-by: Jiayuan Chen > --- Reviewed-by: Jakub Sitnicki
Re: [PATCH bpf v9 2/5] bpf: fix wrong copied_seq calculation
On Wed, Jan 22, 2025 at 06:09 PM +08, 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 updates 'sk->copied_seq', leading to duplicate
> 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.
>
> We do not want to add new proto_ops to implement a new version of
> tcp_read_sock, as this would introduce code complexity [1].
>
> We could have added noack and copied_seq to desc, and then called
> ops->read_sock. However, unfortunately, other modules didn’t fully
> initialize desc to zero. So, for now, we are directly calling
> tcp_read_sock_noack() in tcp_bpf.c.
>
> [1]: https://lore.kernel.org/bpf/20241218053408.437295-1-mr...@163.com
> Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")
> Suggested-by: Jakub Sitnicki
> Signed-off-by: Jiayuan Chen
> ---
I'm happy with how this turned out, but let's run it by Eric.
Reviewed-by: Jakub Sitnicki
Re: [PATCH bpf v9 3/5] bpf: disable non stream socket for strparser
On Wed, Jan 22, 2025 at 06:09 PM +08, Jiayuan Chen wrote:
> Currently, only TCP supports strparser, but sockmap doesn't intercept
> non-TCP connections to attach strparser. For example, with UDP, although
> the read/write handlers are replaced, strparser is not executed due to
> the lack of a read_sock operation.
>
> Furthermore, in udp_bpf_recvmsg(), it checks whether the psock has data,
> and if not, it falls back to the native UDP read interface, making
> UDP + strparser appear to read correctly. According to its commit history,
> this behavior is unexpected.
>
> Moreover, since UDP lacks the concept of streams, we intercept it directly.
>
> Fixes: 1fa1fe8ff161 ("bpf, sockmap: Test shutdown() correctly exits epoll and
> recv()=0")
> Signed-off-by: Jiayuan Chen
> ---
Acked-by: Jakub Sitnicki
Re: [PATCH bpf v9 0/5] bpf: fix wrong copied_seq calculation and add tests
On Wed, Jan 22, 2025 at 06:09 PM +08, Jiayuan Chen wrote: > A previous commit described in this topic > http://lore.kernel.org/bpf/20230523025618.113937-9-john.fastab...@gmail.com > directly updated 'sk->copied_seq' 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. > > That commit 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 updates '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. > > Also we added test cases for bpf + strparser and separated them from > sockmap_basic, as strparser has more encapsulation and parsing > capabilities compared to sockmap. > > --- > V8 -> v9 > https://lore.kernel.org/bpf/20250121050707.55523-1-mr...@163.com/ > Fixed some issues suggested by Jakub Sitnicki. > > V7 -> V8 > https://lore.kernel.org/bpf/20250116140531.108636-1-mr...@163.com/ > Avoid using add read_sock to psock. (Jakub Sitnicki) > Avoid using warpper function to check whether strparser is supported. > > V3 -> V7: > https://lore.kernel.org/bpf/20250109094402.50838-1-mr...@163.com/ > https://lore.kernel.org/bpf/20241218053408.437295-1-mr...@163.com/ > Avoid introducing new proto_ops. (Jakub Sitnicki). > Add more edge test cases for strparser + bpf. > Fix patchwork fail of test cases code. > Fix psock fetch without rcu lock. > Move code of modifying to tcp_bpf.c. > > V1 -> V3: > https://lore.kernel.org/bpf/20241209152740.281125-1-mr...@163.com/ > Fix patchwork fail by adding Fixes tag. > Save skb data offset for ENOMEM. (John Fastabend) > --- Thanks for addressing all feedback, Jiayuan. Series LGTM. Feel free to carry my tags if there is another iteration. -jkbs
Re: [PATCH bpf v9 5/5] selftests/bpf: add strparser test for bpf
On Wed, Jan 22, 2025 at 06:09 PM +08, Jiayuan Chen wrote: > Add test cases for bpf + strparser and separated them from > sockmap_basic, as strparser has more encapsulation and parsing > capabilities compared to standard sockmap. > > Signed-off-by: Jiayuan Chen > --- Acked-by: Jakub Sitnicki
Re: [PATCH bpf v9 4/5] selftests/bpf: fix invalid flag of recv()
On Wed, Jan 22, 2025 at 06:09 PM +08, Jiayuan Chen wrote: > SOCK_NONBLOCK flag is only effective during socket creation, not during > recv. Use MSG_DONTWAIT instead. > > Signed-off-by: Jiayuan Chen > --- Acked-by: Jakub Sitnicki
