Skip to content

Conversation

gentoo-root
Copy link
Contributor

The old_port != new_port flow in snat_v4_rewrite_headers has a few bugs in checksum calculation:

  1. l4_sum is overwritten, so the change in the IP addresses is not taken into account when later calling csum_l4_replace.
  2. The change in ports is accounted with BPF_F_PSEUDO_HDR, which leads to incorrect calculations because ports are not part of the pseudo-header.
  3. The ICMPv4 branch is correct as is, because it takes into account only the ports and clears the BPF_F_PSEUDO_HDR flag for them, but it has to be tweaked as well after fixing (1) and (2).

Fix the above bugs by separating patching the L4 checksum due to changes in the ports and the addresses, pass BPF_F_PSEUDO_HDR only for the addresses, add the explanatory comments, and ensure that ICMPv4 is still handled correctly after the fixes.

Fix IPv4 checksum recalculation in SNAT flows where ports are rewritten.

The old_port != new_port flow in snat_v4_rewrite_headers has a few bugs
in checksum calculation:

1. l4_sum is overwritten, so the change in the IP addresses is not taken
   into account when later calling csum_l4_replace.
2. The change in ports is accounted with BPF_F_PSEUDO_HDR, which leads
   to incorrect calculations because ports are not part of the
   pseudo-header.
3. The ICMPv4 branch is correct as is, because it takes into account
   only the ports and clears the BPF_F_PSEUDO_HDR flag for them, but it
   has to be tweaked as well after fixing (1) and (2).

Fix the above bugs by separating patching the L4 checksum due to changes
in the ports and the addresses, pass BPF_F_PSEUDO_HDR only for the
addresses, add the explanatory comments, and ensure that ICMPv4 is still
handled correctly after the fixes.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root gentoo-root added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Oct 24, 2023
@gentoo-root
Copy link
Contributor Author

/test

1 similar comment
@julianwiedmann
Copy link
Member

/test

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.

looks good, ty Maxim!

(edit: updated the release-note label, as this change luckily didn't make it into a release yet)

@julianwiedmann julianwiedmann added area/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. feature/snat Relates to SNAT or Masquerading of traffic and removed release-note/bug This PR fixes an issue in a previous release of Cilium. labels Oct 25, 2023
@gentoo-root gentoo-root marked this pull request as ready for review October 25, 2023 14:19
@gentoo-root gentoo-root requested a review from a team as a code owner October 25, 2023 14:19
@julianwiedmann julianwiedmann merged commit 495af07 into cilium:main Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants