Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bpf: fix wrong copied_seq calculation and add tests #4742

Closed
wants to merge 2 commits into from
Closed

bpf: fix wrong copied_seq calculation and add tests #4742

wants to merge 2 commits into from

Conversation

kernel-patches-daemon-bpf-rc[bot]
Copy link

Pull request for series with
subject: bpf: fix wrong copied_seq calculation and add tests
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=914899

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: e2cf913
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=914899
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: e2cf913
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=914899
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 509df67
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=914899
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 509df67
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=914899
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 509df67
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=914899
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: b5f2170
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=914899
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: fac04ef
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=914899
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: fac04ef
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=916022
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 78d4f34
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=916022
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 78d4f34
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=916022
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 2357901
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=916022
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 2357901
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=916022
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 2357901
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=918986
version: 3

mrpre added 2 commits December 17, 2024 21:40
@mrpre
bpf: fix wrong copied_seq calculation
dd88440 
'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.

Fixes: e5c6de5 ("bpf, sockmap: Incorrectly handling copied_seq")
Signed-off-by: Jiayuan Chen <[email protected]>
@mrpre
selftests/bpf: add strparser test for bpf
12e44fa 
Add 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.

Signed-off-by: Jiayuan Chen <[email protected]>
@kernel-patches-daemon-bpf-rc
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=918986 expired. Closing PR.

@kernel-patches-daemon-bpf-rc kernel-patches-daemon-bpf-rc bot deleted the series/914899=>bpf branch December 21, 2024 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bpf changes-requested V3-ci-pass V3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@mrpre