bpf:nat: restore a NAT entry if its REV NAT is not found by sugangli · Pull Request #35304 · cilium/cilium

sugangli · 2024-10-08T19:17:01Z

NAT Map LRU behavior could evict any entry in the MAP. This change restore a reverse NAT entry when it is expected but not found. Previously, we were only relying on existence of forwarding NAT to determine if the NAT pair exists or not.

This fix is verified by using the same repro steps in #34833. After the fix, we saw much less timeout from the client, but the case like eviction happens between TCP SYN and TCP SYN ACK will still drop the returning packets.

Fixes: #34833

bpf/lib/nat.h

joestringer · 2024-10-09T22:29:59Z

/test

maintainer-s-little-helper · 2024-10-15T18:01:48Z

Commit 50ad1eb does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

sugangli · 2024-10-16T16:20:46Z

Hi @joestringer, let me know if I need to add anything else to this PR.

bpf/lib/nat.h

sypakine · 2024-10-21T15:20:41Z

/test

joestringer · 2024-10-21T21:13:03Z

@gentoo-root Do you have intentions to review this PR or can you find someone else from @cilium/sig-datapath ?

bpf/lib/nat.h

julianwiedmann

Could you please also add the IPv6 part of the fix?

Agree with Tom that this needs tests. I could see a bpf_host test under bpf/tests/ that walks through the desired packet flow, and reproduces the whole scenario you're fixing:

transmit a packet, have it SNATed
receive a reply for that connection, check that it gets revSNATed
manually delete the revSNAT entry
receive a second reply, check that it no longer gets revSNATed
transmit a second packet, validate that it re-creates the missing state
receive a third reply, check that it gets revSNATed again

ORG NAT entry could be removed by LRU eviction. This commit restore the ORG NAT entry from REV NAT entry and tuple in snat_v4_rev_nat_handle_mapping(). Thus, it can mitigate network failures caused by LRU eviction. Relates: cilium#35304 Signed-off-by: Gyutae Bae <gyu.8ae@gmail.com>

ORG NAT entry could be removed by LRU eviction. This commit restore the ORG NAT entry from REV NAT entry and tuple in snat_v4_rev_nat_handle_mapping(). Thus, it can mitigate network failures caused by LRU eviction. Relates: #35304 Signed-off-by: Gyutae Bae <gyu.8ae@gmail.com>

snat_v4_new_mapping creates a RevSNAT entry, then a SNAT entry for new connections. However, we also have a fallback mechanism in snat_v4_rev_nat_handle_mapping, which restores the SNAT entry if it was evicted by LRU, but the original RevSNAT entry still exists. There is a hypothesis that the above two functions may race with each other, and the following is possible: 1. snat_v4_new_mapping creates a RevSNAT entry. 2. snat_v4_rev_nat_handle_mapping finds the RevSNAT entry and "restores" the SNAT entry (which has never existed yet). 3. snat_v4_new_mapping goes on to create the SNAT entry, corresponding to the RevSNAT entry, created previously. 4. snat_v4_new_mapping fails because the SNAT entry already exists, created by snat_v4_rev_nat_handle_mapping. 5. snat_v4_new_mapping then removes the RevSNAT entry, trying to revert everything. The right outcome in this case would be to keep the SNAT entry, whoever created it first, so it makes sense not to fail on -EEXIST. It also makes the fallback mechanism more robust, because there is a time gap between __snat_lookup and __snat_create. Ref: #35304 Ref: #37747 Fixes: #38466 Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

snat_v4_new_mapping creates a RevSNAT entry, then a SNAT entry for new connections. However, we also have a fallback mechanism in snat_v4_rev_nat_handle_mapping, which restores the SNAT entry if it was evicted by LRU, but the original RevSNAT entry still exists. There is a hypothesis that the above two functions may race with each other, and the following is possible: 1. snat_v4_new_mapping creates a RevSNAT entry. 2. snat_v4_rev_nat_handle_mapping finds the RevSNAT entry and "restores" the SNAT entry (which has never existed yet). 3. snat_v4_new_mapping goes on to create the SNAT entry, corresponding to the RevSNAT entry, created previously. 4. snat_v4_new_mapping fails because the SNAT entry already exists, created by snat_v4_rev_nat_handle_mapping. 5. snat_v4_new_mapping then removes the RevSNAT entry, trying to revert everything. The right outcome in this case would be to keep the SNAT entry, whoever created it first, so it makes sense not to fail on -EEXIST. It also makes the fallback mechanism more robust, because there is a time gap between __snat_lookup and __snat_create. Ref: cilium#35304 Ref: cilium#37747 Fixes: cilium#38466 Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

[ upstream commit b1bdfdd ] snat_v4_new_mapping creates a RevSNAT entry, then a SNAT entry for new connections. However, we also have a fallback mechanism in snat_v4_rev_nat_handle_mapping, which restores the SNAT entry if it was evicted by LRU, but the original RevSNAT entry still exists. There is a hypothesis that the above two functions may race with each other, and the following is possible: 1. snat_v4_new_mapping creates a RevSNAT entry. 2. snat_v4_rev_nat_handle_mapping finds the RevSNAT entry and "restores" the SNAT entry (which has never existed yet). 3. snat_v4_new_mapping goes on to create the SNAT entry, corresponding to the RevSNAT entry, created previously. 4. snat_v4_new_mapping fails because the SNAT entry already exists, created by snat_v4_rev_nat_handle_mapping. 5. snat_v4_new_mapping then removes the RevSNAT entry, trying to revert everything. The right outcome in this case would be to keep the SNAT entry, whoever created it first, so it makes sense not to fail on -EEXIST. It also makes the fallback mechanism more robust, because there is a time gap between __snat_lookup and __snat_create. Ref: #35304 Ref: #37747 Fixes: #38466 Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com> Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

[ upstream commit b1bdfdd ] [ backporter's notes: minor conflicts due to different map name and some missing tests in stable branch. ] snat_v4_new_mapping creates a RevSNAT entry, then a SNAT entry for new connections. However, we also have a fallback mechanism in snat_v4_rev_nat_handle_mapping, which restores the SNAT entry if it was evicted by LRU, but the original RevSNAT entry still exists. There is a hypothesis that the above two functions may race with each other, and the following is possible: 1. snat_v4_new_mapping creates a RevSNAT entry. 2. snat_v4_rev_nat_handle_mapping finds the RevSNAT entry and "restores" the SNAT entry (which has never existed yet). 3. snat_v4_new_mapping goes on to create the SNAT entry, corresponding to the RevSNAT entry, created previously. 4. snat_v4_new_mapping fails because the SNAT entry already exists, created by snat_v4_rev_nat_handle_mapping. 5. snat_v4_new_mapping then removes the RevSNAT entry, trying to revert everything. The right outcome in this case would be to keep the SNAT entry, whoever created it first, so it makes sense not to fail on -EEXIST. It also makes the fallback mechanism more robust, because there is a time gap between __snat_lookup and __snat_create. Ref: #35304 Ref: #37747 Fixes: #38466 Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com> Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

sugangli requested a review from a team as a code owner October 8, 2024 19:17

sugangli requested a review from gentoo-root October 8, 2024 19:17

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 Oct 8, 2024

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 8, 2024

sugangli force-pushed the snat_restore branch 2 times, most recently from 6314beb to ac97018 Compare October 8, 2024 21:27

sypakine reviewed Oct 9, 2024

View reviewed changes

bpf/lib/nat.h Outdated Show resolved Hide resolved

sugangli force-pushed the snat_restore branch from ac97018 to 08c7723 Compare October 9, 2024 15:38

joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Oct 9, 2024

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 Oct 9, 2024

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 15, 2024

sugangli force-pushed the snat_restore branch from 50ad1eb to 08c7723 Compare October 16, 2024 15:44

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 16, 2024

sugangli force-pushed the snat_restore branch from 08c7723 to 980365f Compare October 16, 2024 16:05

aanm requested a review from joestringer October 21, 2024 10:01

sypakine approved these changes Oct 21, 2024

View reviewed changes

bpf/lib/nat.h Show resolved Hide resolved

bpf/lib/nat.h Outdated Show resolved Hide resolved

bpf/lib/nat.h Outdated Show resolved Hide resolved

sugangli force-pushed the snat_restore branch 2 times, most recently from cc029f0 to d9934ce Compare October 21, 2024 20:05

tommyp1ckles reviewed Oct 22, 2024

View reviewed changes

bpf/lib/nat.h Outdated Show resolved Hide resolved

julianwiedmann requested changes Oct 24, 2024

View reviewed changes

julianwiedmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. feature/snat Relates to SNAT or Masquerading of traffic kind/bug This is a bug in the Cilium logic. labels Oct 24, 2024

joestringer removed the request for review from gentoo-root October 24, 2024 17:30

sugangli force-pushed the snat_restore branch from d9934ce to 2ceeeae Compare October 28, 2024 20:41

Merged via the queue into cilium:main with commit 8022c21 Nov 28, 2024

gyutaeb mentioned this pull request Jan 16, 2025

bpf: nat: skip snat when using iptables masquerade #36631

Closed

8 tasks

gyutaeb mentioned this pull request Feb 10, 2025

Feature Proposal: Cilium Configuration Diagnostics Feature bmcustodio/kubectl-cilium#5

Open

gyutaeb mentioned this pull request Feb 19, 2025

bpf:nat: Restore ORG NAT entry if it's not found #37747

Merged

gentoo-root mentioned this pull request Aug 6, 2025

bpf: Don't fail on existing SNAT entries in __snat_create #40971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpf:nat: restore a NAT entry if its REV NAT is not found#35304

bpf:nat: restore a NAT entry if its REV NAT is not found#35304
aanm merged 2 commits intocilium:mainfrom
sugangli:snat_restore

sugangli commented Oct 8, 2024 •

edited

Loading

Uh oh!

Uh oh!

joestringer commented Oct 9, 2024

Uh oh!

maintainer-s-little-helper bot commented Oct 15, 2024

Uh oh!

sugangli commented Oct 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sypakine commented Oct 21, 2024

Uh oh!

joestringer commented Oct 21, 2024

Uh oh!

Uh oh!

julianwiedmann left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

sugangli commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

joestringer commented Oct 9, 2024

Uh oh!

maintainer-s-little-helper bot commented Oct 15, 2024

Uh oh!

sugangli commented Oct 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sypakine commented Oct 21, 2024

Uh oh!

joestringer commented Oct 21, 2024

Uh oh!

Uh oh!

julianwiedmann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sugangli commented Oct 8, 2024 •

edited

Loading