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

cocci: Detect unlogged missed tail calls #11808

Merged
merged 3 commits into from Jun 3, 2020
Merged

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Jun 2, 2020

The first commit introduces a new Coccinelle script to detect tail calls not followed by a DROP_MISSED_TAIL_CALL log. The second commit adapts the code a bit in two places to allow Coccinelle to detect that all paths from tail call contain a DROP_MISSED_TAIL_CALL log. The last commit fixes two instances of unlogged missed tail calls (dating from Cilium 0.10.1).

@cilium/bpf Do we want to backport the two missing log for the tail calls?

Fixes: #190
Updates: #11257

@pchaigno pchaigno added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels Jun 2, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 2, 2020
@pchaigno pchaigno mentioned this pull request Jun 2, 2020
9 tasks
@coveralls
Copy link

coveralls commented Jun 2, 2020

Coverage Status

Coverage remained the same at 36.9% when pulling a1d1886 on pr/pchaigno/cocci-tail-calls into d4a968e on master.

@pchaigno pchaigno force-pushed the pr/pchaigno/cocci-tail-calls branch from dd975fc to 16ad4f8 Compare June 2, 2020 10:49
This new Coccinelle script will detect tail calls (ep_tail_call() calls)
that are not followed by either a call to send_drop_notify_error(arg1,
arg2, DROP_MISSED_TAIL_CALL, ...) or return DROP_MISSED_TAIL_CALL.

Tail calls in macros are not analyzed.

The script cannot patch such unlogged tail calls as the correct fix
depends on the context. It is up to the developer to fix any error found.

Signed-off-by: Paul Chaignon <paul@cilium.io>
Without these changes, and because Coccinelle doesn't analyze the values
of variables to prune branches, the new Coccinelle script detects the
two tail calls as missing a subsequent DROP_MISSED_TAIL_CALL.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/cocci-tail-calls branch from 16ad4f8 to b80a74e Compare June 3, 2020 06:32
The tail calls to tail_handle_ipv{4,6} will not be logged if they fail.
This commit fixes it.

Fixes: 9e108c1 ("nat46: Enable ipv6 containers to talk to an ipv4 container")
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/cocci-tail-calls branch from b80a74e to a1d1886 Compare June 3, 2020 06:37
@pchaigno
Copy link
Member Author

pchaigno commented Jun 3, 2020

test-me-please

@pchaigno pchaigno marked this pull request as ready for review June 3, 2020 06:46
@pchaigno pchaigno requested a review from a team June 3, 2020 06:46
@pchaigno
Copy link
Member Author

pchaigno commented Jun 3, 2020

retest-gke

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks great!

Looks like a candidate for at least 1.8 backport to me, in case it helps catch or trace a bug in the future? Plus it should not be difficult to backport to 1.8.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 3, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 3, 2020
@pchaigno pchaigno requested a review from borkmann June 3, 2020 19:27
@cilium cilium deleted a comment from pchaigno Jun 3, 2020
@borkmann borkmann merged commit 0bc0aa8 into master Jun 3, 2020
1.8.0 automation moved this from In progress to Merged Jun 3, 2020
@borkmann borkmann deleted the pr/pchaigno/cocci-tail-calls branch June 3, 2020 20:14
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 5, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

None yet

5 participants