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.

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

pull skb data at the entrance of from-containter #19308

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

liuyuan10
Copy link
Contributor

@liuyuan10 liuyuan10 commented Apr 1, 2022

When the skb is non-linear, revalidate_data drops the packet
immediately. For example, This can happen when pods are using AF_PACKET + mmap ring buffer.

calling revalidate_data_pull (bpf_skb_pull_data) helps to "Pull in non-linear data in case the skb is non-linear and not all of len are part of the linear section."

Fixes #18951

Signed-off-by: Yuan Liu liuyuan@google.com

Fix drop for packets sent via AF_PACKET + mmap ring buffer in pod

When the skb is non-linear, revalidate_data drops the packet
immediately. For example, This can happen when pods are using AF_PACKET
+ mmap ring buffer.

calling revalidate_data_pull (bpf_skb_pull_data) helps to "Pull in non-linear data in case the skb is non-linear and not all of len are part of the linear section."

Fixes cilium#18951

Signed-off-by: Yuan Liu <liuyuan@google.com>
@liuyuan10 liuyuan10 requested a review from a team April 1, 2022 22:55
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 1, 2022
@@ -473,7 +473,7 @@ int tail_handle_ipv6(struct __ctx_buff *ctx)
struct ipv6hdr *ip6;
int ret;

if (!revalidate_data(ctx, &data, &data_end, &ip6))
if (!revalidate_data_pull(ctx, &data, &data_end, &ip6))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would bpf_skb_pull_data error out if the skb is linear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. It should be no op if it's linear already. The same revalidate_data_pull is already used in bpf_host and bpf_overlay

@YutaroHayakawa
Copy link
Member

/test

@YutaroHayakawa YutaroHayakawa added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Apr 6, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 6, 2022
@YutaroHayakawa YutaroHayakawa added kind/bug This is a bug in the Cilium logic. needs-backport/1.10 labels Apr 6, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.4 Apr 6, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.10 Apr 6, 2022
@YutaroHayakawa YutaroHayakawa self-assigned this Apr 6, 2022
@YutaroHayakawa
Copy link
Member

YutaroHayakawa commented Apr 7, 2022

@YutaroHayakawa
Copy link
Member

/test-1.23-net-next

@YutaroHayakawa
Copy link
Member

/test-runtime

@YutaroHayakawa
Copy link
Member

/ci-nat46x64

@YutaroHayakawa
Copy link
Member

ci-nat46x64 failed with (#19350), but since other L4LB tests are passing, the change only affects the header extraction, I believe it is safe to merge now.

@YutaroHayakawa YutaroHayakawa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 8, 2022
@YutaroHayakawa YutaroHayakawa removed their assignment Apr 8, 2022
@nbusseneau nbusseneau merged commit 2876483 into cilium:master Apr 8, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.10.10 Apr 12, 2022
@nbusseneau
Copy link
Member

@YutaroHayakawa I have removed the backport label for 1.10 because the changes in this PR do not cleanly apply on top on v1.10, as they are dependent (at least) on #17758 which was not backported. If we need these changes in v1.10, then we should backport the dependent PRs first.

@nbusseneau
Copy link
Member

Same for 1.11.

@nbusseneau
Copy link
Member

@liuyuan10 Thanks, can you point out specifically at which lines in the following files should the patch be applied?

This way we can re-label for backport and TopHat can incorporate those in the next round (cc @tklauser).

@liuyuan10
Copy link
Contributor Author

v1.11 bpf_lxc.c: line 547 and 477
v1.10 bpf_lxc.c: line 537 and 467

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.4 Apr 14, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.10 Apr 14, 2022
@joestringer joestringer moved this from Needs backport from master to Backport done to v1.10 in 1.10.10 Apr 15, 2022
@joestringer joestringer moved this from Backport done to v1.10 to Needs backport from master in 1.10.10 Apr 15, 2022
@joestringer joestringer added this to Needs backport from master in 1.10.11 Apr 15, 2022
@joestringer joestringer removed this from Needs backport from master in 1.10.10 Apr 15, 2022
@joestringer joestringer added this to Needs backport from master in 1.11.5 Apr 15, 2022
@joestringer joestringer removed this from Needs backport from master in 1.11.4 Apr 15, 2022
@tklauser tklauser added backport-pending/1.10 backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed needs-backport/1.10 labels Apr 19, 2022
@tklauser tklauser moved this from Needs backport from master to Backport done to v1.11 in 1.11.5 Apr 20, 2022
@tklauser tklauser moved this from Needs backport from master to Backport pending to v1.10 in 1.10.11 Apr 20, 2022
@tklauser
Copy link
Member

It looks like the backport of this PR to v1.10 is causing BPF regeneration issues on all 4.9 jobs, see #19482 (comment)

Unfortunately, the failed jobs don't seem to have agent logs for further analysis. I haven't yet had cycles to reproduce locally, thus I've dropped the backported commit for now.

@tklauser tklauser moved this from Backport pending to v1.10 to Needs backport from master in 1.10.11 Apr 22, 2022
@joestringer
Copy link
Member

Removing 1.10 backport label as backporters are hitting issues while backporting this.

@joestringer joestringer removed this from Needs backport from master in 1.10.11 Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.11.5
Backport done to v1.11
Development

Successfully merging this pull request may close these issues.

DROP_INVALID for packet sent via AF_PACKET + mmap ring buffer in pod
7 participants