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: Handle fragments in SNAT flows #25340

Merged
merged 4 commits into from
Nov 13, 2023
Merged

bpf: Handle fragments in SNAT flows #25340

merged 4 commits into from
Nov 13, 2023

Conversation

gentoo-root
Copy link
Contributor

The code that extracts ports in snat_v4_nat and snat_v4_rev_nat doesn't take into account that the packet may be a second or further fragment. In this case, garbage will be read instead of the real port numbers. Fix this by using the existing ipv4_ct_extract_l4_ports that takes into account fragmentation.

This change makes the behavior of the switch inside the aforementioned functions closer to ct_extract_ports4.

Consider fragments in snat_v4_rewrite_ingress, avoid rewriting ports if it's not the first fragment.

Fixes: 14a653a ("bpf/nat: introduce snat_v4_nat() and snat_v4_rev_nat functions")

This should be applied on top of #25112.

Handle IPv4 fragments in SNAT flows correctly.

@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 May 9, 2023
@julianwiedmann julianwiedmann mentioned this pull request May 29, 2023
Merged
@gentoo-root
Copy link
Contributor Author

/test

@gentoo-root gentoo-root mentioned this pull request Jun 29, 2023
Merged
@github-actions
Copy link

github-actions bot commented Jul 8, 2023

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 8, 2023
@gentoo-root gentoo-root removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 10, 2023
@gentoo-root
Copy link
Contributor Author

/test

@gentoo-root
Copy link
Contributor Author

/test

@github-actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 15, 2023
@gentoo-root gentoo-root removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 18, 2023
@github-actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 22, 2023
@gentoo-root gentoo-root removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 25, 2023
@gentoo-root gentoo-root force-pushed the nat-fragments-fix branch 3 times, most recently from b4f954d to e2ac9d1 Compare September 27, 2023 16:40
@gentoo-root
Copy link
Contributor Author

/test

@gentoo-root gentoo-root marked this pull request as ready for review September 27, 2023 22:16
@gentoo-root gentoo-root requested a review from a team as a code owner September 27, 2023 22:16
@gentoo-root gentoo-root added the release-note/misc This PR makes changes that have no direct user impact. label Sep 29, 2023
@julianwiedmann julianwiedmann added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Oct 2, 2023
julianwiedmann
julianwiedmann requested changes Oct 2, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! It's been a long time coming :).

A few smaller notes, but the only thing that really needs addressing is how ct_lazy_lookup4() obtains the is_fragment variable. Happy to brain-storm further if passing around an additional parameter is too much hassle.

bpf/lib/nat.h Outdated Show resolved Hide resolved
bpf/lib/nat.h Outdated Show resolved Hide resolved
bpf/lib/conntrack.h Outdated Show resolved Hide resolved
@gentoo-root gentoo-root force-pushed the nat-fragments-fix branch 2 times, most recently from 8b83858 to 595f9e3 Compare October 16, 2023 16:45
@gentoo-root
Copy link
Contributor Author

/test

1 similar comment
@gentoo-root
Copy link
Contributor Author

/test

julianwiedmann
julianwiedmann requested changes Oct 18, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good, just the ct_lazy_lookup4() that needs adjusting.

Should we maybe expedite the checksum fixes through a separate PR ?

bpf/lib/conntrack.h Outdated Show resolved Hide resolved
bpf/lib/nat.h Outdated Show resolved Hide resolved
@julianwiedmann julianwiedmann mentioned this pull request Oct 23, 2023
Merged
@gentoo-root
Copy link
Contributor Author

Should we maybe expedite the checksum fixes through a separate PR ?

Separated to another PR: #28768

@gentoo-root gentoo-root force-pushed the nat-fragments-fix branch 3 times, most recently from 3f6a6b7 to 4e5ca0a Compare October 30, 2023 17:11
julianwiedmann
julianwiedmann reviewed Oct 31, 2023
View reviewed changes
bpf/lib/nodeport.h Outdated Show resolved Hide resolved
@gentoo-root
bpf: Handle fragments in SNAT flows
7d6d2b8 
The code that extracts ports in snat_v4_nat and snat_v4_rev_nat doesn't
take into account that the packet may be a second or further fragment.
In this case, garbage will be read instead of the real port numbers. Fix
this by using the existing ipv4_ct_extract_l4_ports that takes into
account fragmentation.

This change makes the behavior of the switch inside the aforementioned
functions closer to ct_extract_ports4.

Consider fragments in snat_v4_rewrite_ingress, avoid rewriting ports if
it's not the first fragment.

Fixes: 14a653a ("bpf/nat: introduce snat_v4_nat() and snat_v4_rev_nat functions")
Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root gentoo-root force-pushed the nat-fragments-fix branch 3 times, most recently from a0c1f0f to 00a25b7 Compare November 10, 2023 16:16
@gentoo-root
bpf: Pass is_fragment to ct_lazy_lookup4
5d0eb42 
The next commit will use it to update metrics. Note that revalidate_data
shouldn't be done in place, because the L3 header offset may be not
equal to ETH_HLEN in XDP flows.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root
bpf: Move fragment metric update to __ct_lookup4
13bcbc1 
The previous commit ("bpf: Handle fragments in SNAT flows") added an
extra call to ipv4_ct_extract_l4_ports into the SNAT paths. It can lead
to double-accounting of the same fragments, failing the "Supports IPv4
fragments" test, because ipv4_handle_fragmentation is called one more
time in those flows.

Fix it by moving the metric update out of ipv4_handle_fragmentation
directly to __ct_lookup4.

Suggested-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root
bpf: Use snat_v4_rewrite_headers for snat_v4_rev_nat
4906275 
Replace snat_v4_rewrite_ingress by a generic snat_v4_rewrite_headers for
better code reuse. This change should not change the behavior, the new
code should be functionally equivalent to the old one.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root
Copy link
Contributor Author

/test

@julianwiedmann julianwiedmann added the kind/enhancement This would improve or streamline existing functionality. label Nov 13, 2023
julianwiedmann
julianwiedmann approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

ty Maxim, looks great!

@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 Nov 13, 2023
@julianwiedmann julianwiedmann merged commit 8206494 into cilium:main Nov 13, 2023
61 checks passed
@El-Begawy El-Begawy mentioned this pull request May 14, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
feature/snat Relates to SNAT or Masquerading of traffic kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gentoo-root @julianwiedmann