| Message ID | 20241209152740.281125-1-mrpre@163.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bpf: fix wrong copied_seq calculation and add tests | expand |
On Mon, Dec 09, 2024 at 11:27 PM +08, Jiayuan Chen wrote: [...] > We added test cases for bpf + strparser and separated them from > sockmap_basic. This is because we need to add more test cases for > strparser in the future. > > Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq") > > --- I have a question unrelated to the fix itself - Are you an active strparser+verdict sockmap user? I was wondering if we can deprecate strparser if/when KCM time comes [1]. [1] https://lore.kernel.org/netdev/CANn89iK60jxsJCzq29WPSZJnYNHHpPS09_ZmSi1JHmbkZ2GznA@mail.gmail.com/
On Sat, Dec 14, 2024 at 07:50:37PM +0100, Jakub Sitnicki wrote: > On Mon, Dec 09, 2024 at 11:27 PM +08, Jiayuan Chen wrote: > > [...] > > > We added test cases for bpf + strparser and separated them from > > sockmap_basic. This is because we need to add more test cases for > > strparser in the future. > > > > Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq") > > > > --- > > I have a question unrelated to the fix itself - > > Are you an active strparser+verdict sockmap user? > > I was wondering if we can deprecate strparser if/when KCM time comes I am afraid not. strparser is very different from skb verdict, upper layer (e.g. HTTP) protocol messages may be splitted accross sendmsg() call's, strparser is the only place where we can assemble the messages and parse them as a whole. And I don't think we have to use KCM together with strparser. Therefore, even _if_ KCM can be deprecated, strparse still can't. Regards.
On Sat, Dec 14, 2024 at 05:18 PM -08, Cong Wang wrote: > On Sat, Dec 14, 2024 at 07:50:37PM +0100, Jakub Sitnicki wrote: >> On Mon, Dec 09, 2024 at 11:27 PM +08, Jiayuan Chen wrote: >> >> [...] >> >> > We added test cases for bpf + strparser and separated them from >> > sockmap_basic. This is because we need to add more test cases for >> > strparser in the future. >> > >> > Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq") >> > >> > --- >> >> I have a question unrelated to the fix itself - >> >> Are you an active strparser+verdict sockmap user? >> >> I was wondering if we can deprecate strparser if/when KCM time comes > > I am afraid not. > > strparser is very different from skb verdict, upper layer (e.g. HTTP) > protocol messages may be splitted accross sendmsg() call's, strparser > is the only place where we can assemble the messages and parse them as a > whole. > > And I don't think we have to use KCM together with strparser. Therefore, > even _if_ KCM can be deprecated, strparse still can't. Thanks for the context. Good to know we have strparser users. I also wanna ask - did you guys consider migrating strp_data_ready->strp_read_sock->...->strp_recv to read_skb / tcp_read_skb to prevent the duplicate copied_seq update? tcp_bpf_read_sock looks awfully lot like tcp_read_skb. I realize it is easier said than done because there is an interface mismatch - desc.count used to stop reading, and desc.error to signal OOM / need to requeue is missing. And then there is the SW kTLS read_sock callback that would need adapting as well. Definitely more work, but maybe less code duplication in the long run?
A previous commit described in this topic http://lore.kernel.org/bpf/20230523025618.113937-9-john.fastabend@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 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(). We added test cases for bpf + strparser and separated them from sockmap_basic. This is because we need to add more test cases for strparser in the future. Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq") --- v1-v2: fix patchwork fail by adding Fixes tag --- --- Jiayuan Chen (2): bpf: fix wrong copied_seq calculation selftests/bpf: add strparser test for bpf include/linux/skmsg.h | 1 + include/net/tcp.h | 1 + net/core/skmsg.c | 3 + net/ipv4/tcp.c | 2 +- net/ipv4/tcp_bpf.c | 77 +++++- .../selftests/bpf/prog_tests/sockmap_basic.c | 53 ---- .../selftests/bpf/prog_tests/sockmap_strp.c | 255 ++++++++++++++++++ .../selftests/bpf/progs/test_sockmap_strp.c | 51 ++++ 8 files changed, 386 insertions(+), 57 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/sockmap_strp.c create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_strp.c base-commit: 5a6ea7022ff4d2a65ae328619c586d6a8909b48b