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: don't access TCP flags for non initial IPv4 fragments #13908

Merged
merged 1 commit into from
Nov 9, 2020
Merged

bpf: don't access TCP flags for non initial IPv4 fragments #13908

merged 1 commit into from
Nov 9, 2020

Conversation

jibi
Copy link
Member

@jibi jibi commented Nov 5, 2020

When dealing with IPv4 fragmented packets, only the first fragment will
contain the L4 header.

In ct_lookup4() the packet's TCP flags are tested without checking first
if the packet is an initial fragment/not fragmented, which may lead to
incorrectly treating the l4 payload of the non initial IPv4 fragments as
TCP flags.

This commit fixes this.

@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 Nov 5, 2020
@jibi
Copy link
Member Author

jibi commented Nov 5, 2020

test-me-please

@jibi
bpf: don't access TCP flags for non initial IPv4 fragments
ab570e1 
When dealing with IPv4 fragmented packets, only the first fragment will
contain the L4 header.

In ct_lookup4() the packet's TCP flags are tested without checking first
if the packet is an initial fragment/not fragmented, which may lead to
incorrectly treating the l4 payload of the non initial IPv4 fragments as
TCP flags.

This commit fixes this.

Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
@jibi
Copy link
Member Author

jibi commented Nov 5, 2020

test-me-please

@jibi jibi marked this pull request as ready for review November 6, 2020 09:19
@jibi jibi requested review from a team and fristonio November 6, 2020 09:19
@jibi
Copy link
Member Author

jibi commented Nov 6, 2020

By looking at the test results and at the CI dashboard I think Test Checks service across nodes with L7 policy Tests NodePort with L7 Policy is a flake

@jibi jibi requested a review from qmonnet November 6, 2020 09:22
@jibi
Copy link
Member Author

jibi commented Nov 6, 2020

retest-4.9

pchaigno
pchaigno approved these changes Nov 6, 2020
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Nice catch!

@pchaigno
Copy link
Member

pchaigno commented Nov 6, 2020

By looking at the test results and at the CI dashboard I think Test Checks service across nodes with L7 policy Tests NodePort with L7 Policy is a flake

Was it #13011 maybe? If it looks different, please open a CI flake issue.

@pchaigno pchaigno added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Nov 6, 2020
@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 Nov 6, 2020
qmonnet
qmonnet approved these changes Nov 6, 2020
View reviewed changes
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.

Nice, thanks a lot for fixing this!

@qmonnet qmonnet removed their assignment Nov 6, 2020
@jibi
Copy link
Member Author

jibi commented Nov 6, 2020

By looking at the test results and at the CI dashboard I think Test Checks service across nodes with L7 policy Tests NodePort with L7 Policy is a flake

Was it #13011 maybe? If it looks different, please open a CI flake issue.

Yep, looks about the same :)

fristonio
fristonio approved these changes Nov 6, 2020
View reviewed changes
Copy link
Member

@fristonio fristonio left a comment

Choose a reason for hiding this comment

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

LGTM, good catch 🚀 🥳

@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 6, 2020
@aanm aanm merged commit 1ed1db1 into cilium:master Nov 9, 2020
@jibi jibi deleted the pr/jibi/fix-ctlookup4-frag-packets branch November 9, 2020 08:55
@tklauser tklauser mentioned this pull request Nov 9, 2020
Merged
@jrajahalme jrajahalme mentioned this pull request Nov 9, 2020
Merged
@tklauser
Copy link
Member

Note for future backports this caused complexity issues on 1.8, see #13951 (comment) and https://cilium.slack.com/archives/C7PE7V806/p1605016684205600.

@twpayne twpayne mentioned this pull request Nov 13, 2020
Merged
@twpayne
Copy link
Contributor

twpayne commented Nov 13, 2020

I'm not re-adding needs-backport/1.8 label as it looks like this needs some thought (ref).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@qmonnet qmonnet qmonnet approved these changes

@fristonio fristonio fristonio approved these changes

Assignees

@fristonio fristonio

Labels
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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jibi @pchaigno @tklauser @twpayne @qmonnet @fristonio @joestringer @aanm @jrajahalme