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: add missing drop notifications #25183

Merged
merged 7 commits into from
May 12, 2023
Merged

bpf: add missing drop notifications #25183

merged 7 commits into from
May 12, 2023

Conversation

julianwiedmann
Copy link
Member

End goal was to get rid of the stray drop notification that's currently buried in __encap_and_redirect_with_nodeid(). Getting there took quite a few additional drive-by fixes :).

The last two patches can be dropped from the backport (encap_and_redirect_with_nodeid_opt() didn't exist in v1.13).

Add drop notifications for various error paths in the datapath.

@julianwiedmann
bpf: host: add drop notify for missed POLICY_EGRESSCALL_MAP tail-call
906e9e8 
We're returning a DROP reason, but nothing outside do_netdev() creates the
corresponding drop notification from it.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: host: add drop notify for do_netdev_encrypt()
f3b8e6f 
do_netdev_encrypt_encap() can return various errors, but its caller doesn't
raise the corresponding drop notification.

Also clean up the one case in do_netdev_encrypt_encap() where we currently
*do* raise a drop notification.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: host: check ep->tunnel_endpoint in do_netdev_encrypt_encap()
5590a4e 
__encap_and_redirect_with_nodeid() expects the caller to handle this check.
Otherwise we end up encapsulating with an OuterDstIP of 0.0.0.0.

I looked at all the other users, looks like this was the only one missing.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: overlay: add drop notify for __encap_and_redirect_with_nodeid()
b7fd5d9 
Align with all the other error paths in tail_handle_arp() and raise a drop
notification on error. This function is executed as a tail-call, so there's
no surrounding code that would do this for us otherwise.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: host: add drop notify for __encap_and_redirect_with_nodeid()
0bfdba6 
Add an error path for the from-netdev program that handles the missing
drop notification.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: host: add drop notify for encap_and_redirect_with_nodeid_opt()
939fa0f 
Fix one more missing drop notification in the from-netdev program.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 28, 2023
@julianwiedmann julianwiedmann requested a review from a team as a code owner April 28, 2023 07:13
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 Apr 28, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
bpf: encap: remove redundant drop notification
06626ae 
All direct and indirect callers are now properly handling errors from this
path, and raising the drop notification. So remove it from the low-level
helper code.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

BPF checkpatch complains about

CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#21: FILE: bpf/bpf_host.c:1278:

  •   		ret = encap_and_redirect_with_nodeid_opt(ctx,
  						  ctx_get_xfer(ctx,

which was already bad before. And fixing it means triggering checkpatch again, because we're then exceeding max line width 🤷. Think I'm good leaving it as is, we're planning to remove this code during the current release cycle anyway.

@julianwiedmann
Copy link
Member Author

@ldelossa 👋 how can I help with making your review easier?

ldelossa
ldelossa approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

LGTM

@julianwiedmann
Copy link
Member Author

One static checker complaint (#25183 (comment)) that is ok to ignore imo. The e2e workflow is marked as still expected, but that's just because when the PR was opened this was still called ci-datapath (which is green).

So Ready-to-merge (cc @youngnick 🙏)

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 12, 2023
@aanm aanm merged commit 83268f4 into cilium:main May 12, 2023
55 of 56 checks passed
@julianwiedmann julianwiedmann deleted the 1.14-bpf-drop-notify branch May 12, 2023 13:42
@gentoo-root gentoo-root mentioned this pull request May 12, 2023
Closed
3 tasks
@jibi jibi mentioned this pull request May 17, 2023
Merged
7 tasks
@jibi jibi added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels May 17, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.3 May 17, 2023
@jibi jibi added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels May 23, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.3 May 23, 2023
@qmonnet qmonnet mentioned this pull request Jun 9, 2023
Closed
This was referenced Jun 9, 2023
Closed
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport pending to v1.12 in 1.12.10 Oct 6, 2023
@julianwiedmann julianwiedmann mentioned this pull request Oct 6, 2023
Merged
1 task
@jrajahalme jrajahalme mentioned this pull request Oct 17, 2023
Merged
@julianwiedmann julianwiedmann added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Oct 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.10 Oct 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.10 Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldelossa ldelossa ldelossa approved these changes

Assignees
No one assigned
Labels
backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.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. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.12.10
Backport done to v1.12
1.13.3
Backport done to v1.13
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@julianwiedmann @ldelossa @jibi @aanm