Skip to content

Add missing inner IP header in ICMP error-reply packet #21234

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
Merged
merged 1 commit into from
Oct 18, 2022
Merged

Add missing inner IP header in ICMP error-reply packet #21234

merged 1 commit into from
Oct 18, 2022

Conversation

nnbu
Copy link
Contributor

@nnbu nnbu commented Sep 7, 2022 •
edited
Loading

With the existing code, icmp packet reply can not be decoded because
it has the following format:
ipv4 packet = ipv4 + icmp + orig_ipv4 (has all 0s) + original_data(first 8 bytes)
ipv6 packet = ipv6 + icmpv6 + orig_ipv6 (has all 0s) + original_data(first 64 bytes)

This change adds the missing inner ip header so that packet can be
correctly decoded.
ipv4 packet = ipv4 + icmp + orig_ipv4 + 8 bytes of original data
ipv6 packet = ipv6 + icmpv6 + orig_ipv6 + 64 bytes of original data

Fixes: #21236
Signed-off-by: Nishant Burte nishantburte@gmail.com

@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 7, 2022
@nnbu nnbu changed the title Add ip header in icmp reply packet Add ipv4 header in icmp reply packet Sep 7, 2022
@nnbu nnbu changed the title Add ipv4 header in icmp reply packet Add inner ipv4 header in icmp reply packet Sep 7, 2022
@nnbu nnbu marked this pull request as ready for review September 8, 2022 19:06
@nnbu nnbu requested a review from a team as a code owner September 8, 2022 19:06
@nnbu nnbu requested a review from YutaroHayakawa September 8, 2022 19:06
sahid
sahid suggested changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@sahid sahid 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 this add!

@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@nnbu nnbu requested review from a team as code owners September 9, 2022 20:25
@YutaroHayakawa
Copy link
Member

Here we have a backport criteria of the current stable - 1 version. I think this change meets the criteria Major bug fixes relevant to the correct operation of Cilium. Let me put backport tags for v1.11 and v1.10.

https://docs.cilium.io/en/v1.12/contributing/release/backports/#backport-criteria-for-x-y-1-z-and-x-y-2-z

@pchaigno
Copy link
Member

I think this change meets the criteria Major bug fixes relevant to the correct operation of Cilium. Let me put backport tags for v1.11 and v1.10.

If we end up having any conflict while backporting, I'd be in favor of dropping it from backports. Calling this a major bug is a bit much IMO, especially without a description of the exact impact this bug can have.

@YutaroHayakawa
Copy link
Member

@pchaigno Then, how should @nnbu act when backport is dropped? Make backport PR by themselves?

@sahid
Copy link
Contributor

sahid commented Oct 19, 2022

I tend to agree with Paul, looks like this sounds more like a new support than a major bug fix. We should be careful on backporting changes in a stable branch, even if I would be glad to see this support in early releases.

@qmonnet qmonnet mentioned this pull request Oct 19, 2022
Merged
6 tasks
@qmonnet qmonnet mentioned this pull request Oct 19, 2022
Merged
3 tasks
@qmonnet
Copy link
Member

qmonnet commented Oct 19, 2022

Backport looked non-trivial, so I've kept it out of the regular backport round for 1.10, 1.11.

@qmonnet qmonnet mentioned this pull request Oct 19, 2022
Merged
3 tasks
@nnbu nnbu deleted the icmp_reply branch October 20, 2022 20:19
@qmonnet
Copy link
Member

qmonnet commented Oct 27, 2022

I also removed it from the current round of 1.12 backports.

@pchaigno pchaigno mentioned this pull request Nov 7, 2022
Merged
@michi-covalent michi-covalent mentioned this pull request Nov 16, 2022
Merged
@pchaigno
Copy link
Member

We've removed the backport of this fix from v1.12 because it was causing verifier errors (invalid size of register spill) there. If you need it backported, please open a backport PR.

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

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

+2 more reviewers

@NikAleksandrov NikAleksandrov NikAleksandrov left review comments

@sahid sahid sahid approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/bug This is a bug in the Cilium logic. 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.

dsr_reply_icmp4 does not add original ip header in the icmp message
10 participants
@nnbu @ldelossa @sahid @sayboras @YutaroHayakawa @qmonnet @joestringer @pchaigno @NikAleksandrov @michi-covalent