Skip to content

bpf: Correct refinement of inner packet L4 checksum detection#43868

Merged
julianwiedmann merged 1 commit intocilium:mainfrom
br4243:update-1a018d5-implementation
Jan 21, 2026
Merged

bpf: Correct refinement of inner packet L4 checksum detection#43868
julianwiedmann merged 1 commit intocilium:mainfrom
br4243:update-1a018d5-implementation

Conversation

@br4243
Copy link
Copy Markdown
Contributor

@br4243 br4243 commented Jan 19, 2026

The previous commit updating short ICMP error packet NAT handling improperly computed the length of a short packet by neglecting to account for the ihl field of the IP header representing 32 bit words rather than bytes. This caused a reversion of #33844.

Furthermore, the change in short packet detection depends on the incoming bpf_context structure having the "len" field set correctly, which the BPF tests for that condition did not set.

This corrects the comparison for the inner L4 packet length and updates the short ICMP error packet BPF tests to ensure the ctx->len field is set like the kernel does.

Fixes: 1a018d56d623 ("bpf: Refine inner packet L4 checksum detection")

Fix Layer 4 length computation for checksum detection in SNAT-ed ICMP DestinationUnreachable (FragmentationNeeded) packets.

@br4243 br4243 requested a review from a team as a code owner January 19, 2026 11:11
@br4243 br4243 requested a review from smagnani96 January 19, 2026 11:11
@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 Jan 19, 2026
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 19, 2026
@br4243
Copy link
Copy Markdown
Contributor Author

br4243 commented Jan 19, 2026

@yushoyamaguchi @pchaigno
This fixes the length comparison for 1a018d56d623

@br4243 br4243 force-pushed the update-1a018d5-implementation branch 2 times, most recently from ff1dc74 to 4450925 Compare January 19, 2026 11:23
@br4243 br4243 force-pushed the update-1a018d5-implementation branch from e543e6e to 0a22318 Compare January 20, 2026 13:22
The previous commit updating short ICMP error packet NAT
handling improperly computed the length of a short packet
by neglecting to account for the ihl field of the IP header
representing 32 bit words rather than bytes.  This caused
a reversion of cilium#33844.

Furthermore, the change in short packet detection depends on
the incoming bpf_context structure having the "len" field set
correctly, which the BPF tests for that condition did not set.

This corrects the comparison for the inner L4 packet length
and updates the short ICMP error packet BPF tests to ensure
the ctx->len field is set like the kernel does.

Fixes: cilium@1a018d56d623 ("bpf: Refine inner packet L4 checksum detection")

Signed-off-by: Bill Reese <ReeseW@computer.org>
@br4243 br4243 force-pushed the update-1a018d5-implementation branch from 0a22318 to 4b20127 Compare January 20, 2026 13:38
@smagnani96
Copy link
Copy Markdown
Contributor

/test

@smagnani96 smagnani96 added kind/bug This is a bug in the Cilium logic. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. feature/snat Relates to SNAT or Masquerading of traffic needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch needs-backport/1.19 This PR / issue needs backporting to the v1.19 branch labels Jan 21, 2026
@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 Jan 21, 2026
Copy link
Copy Markdown
Contributor

@smagnani96 smagnani96 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!
@julianwiedmann I added labels, feel free to make any needed adjustment.
I see the original PR has been backported to v1.18, so we need this in v1.18 and v1.19 too.

@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 Jan 21, 2026
@julianwiedmann julianwiedmann added this pull request to the merge queue Jan 21, 2026
@julianwiedmann
Copy link
Copy Markdown
Member

Thank you!

@pchaigno
Copy link
Copy Markdown
Member

We're still missing a release note here.

Merged via the queue into cilium:main with commit b7c4147 Jan 21, 2026
84 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jan 21, 2026
@smagnani96
Copy link
Copy Markdown
Contributor

We're still missing a release note here.

I thought PR title was enough, but maybe not too representative.

@br4243 Refer to the Development Doc. We usually include that at the end of the PR description, which will be used when generating release notes. I gave it a shot (changed PR description), please double check and adjust accordingly if needed.

@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jan 21, 2026

I thought PR title was enough, but maybe not too representative.

It's too generic. We'd need something that tells users exactly who's concerned and what the impact is. One way to achieve that is to follow the pattern Fix a bug that would ... when using ....

@giorio94 giorio94 mentioned this pull request Jan 22, 2026
6 tasks
@giorio94 giorio94 added backport-pending/1.19 The backport for Cilium 1.19.x for this PR is in progress. and removed needs-backport/1.19 This PR / issue needs backporting to the v1.19 branch labels Jan 22, 2026
@giorio94 giorio94 mentioned this pull request Jan 22, 2026
1 task
@giorio94 giorio94 added backport-pending/1.18 The backport for Cilium 1.18.x for this PR is in progress. and removed needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch labels Jan 22, 2026
@br4243
Copy link
Copy Markdown
Contributor Author

br4243 commented Jan 22, 2026

Thank you for the pointers on the release notes; I'll be sure to include that next time!

@br4243 br4243 deleted the update-1a018d5-implementation branch January 22, 2026 12:02
@github-actions github-actions bot added backport-done/1.18 The backport for Cilium 1.18.x for this PR is done. backport-done/1.19 The backport for Cilium 1.19.x for this PR is done. and removed backport-pending/1.18 The backport for Cilium 1.18.x for this PR is in progress. backport-pending/1.19 The backport for Cilium 1.19.x for this PR is in progress. labels Jan 22, 2026
@yushoyamaguchi
Copy link
Copy Markdown
Member

@br4243
Thank you so much for finding and correcting my mistake!

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. backport-done/1.18 The backport for Cilium 1.18.x for this PR is done. backport-done/1.19 The backport for Cilium 1.19.x for this PR is done. feature/snat Relates to SNAT or Masquerading of traffic kind/bug This is a bug in the Cilium logic. kind/community-contribution This was a contribution made by a community member. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

No open projects
Status: Released

Development

Successfully merging this pull request may close these issues.

6 participants