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.

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

Small ICMP fragments dropped by datapath #16036

Closed
joamaki opened this issue May 6, 2021 · 2 comments
Closed

Small ICMP fragments dropped by datapath #16036

joamaki opened this issue May 6, 2021 · 2 comments
Labels
kind/bug This is a bug in the Cilium logic. kind/question Frequently asked questions & answers. This issue will be linked from the documentation's FAQ. pinned These issues are not marked stale by our issue bot. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.

Comments

@joamaki
Copy link
Contributor

joamaki commented May 6, 2021

While testing MTU settings with e.g. ping -s <large> I noticed that Cilium datapath currently seems to drop (tested rev 0ff85f2) ICMP fragments when the fragment is smaller than sizeof(icmphdr) due to the ICMP type check in __policy_can_access (https://github.com/cilium/cilium/blob/0ff85f2f9/bpf/lib/policy.h#L117) as the code is not checking whether the packet in question is a fragment and thus might not contain the ICMP header.

Would it be worth adding the extra branch to the datapath to fix this edge case?

@joamaki joamaki added kind/question Frequently asked questions & answers. This issue will be linked from the documentation's FAQ. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels May 6, 2021
@joamaki joamaki changed the title Small fragmented ICMP packets dropped by datapath Fragmented ICMP packets dropped by datapath May 10, 2021
@joamaki joamaki changed the title Fragmented ICMP packets dropped by datapath Small ICMP fragments dropped by datapath May 10, 2021
@stale
Copy link

stale bot commented Jul 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 11, 2021
@kkourt kkourt added the pinned These issues are not marked stale by our issue bot. label Jul 12, 2021
@stale stale bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 12, 2021
@kkourt kkourt added the kind/bug This is a bug in the Cilium logic. label Jul 12, 2021
@kkourt
Copy link
Contributor

kkourt commented Jul 12, 2021

Discussing this offline, it was determined that it's not worth addressing it (at least at this point), since this is a corner case and adding a branch in the datapath code will increase complexity. Hence, closing the issue. We can revisit if needed.

@kkourt kkourt closed this as completed Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is a bug in the Cilium logic. kind/question Frequently asked questions & answers. This issue will be linked from the documentation's FAQ. pinned These issues are not marked stale by our issue bot. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

No branches or pull requests

2 participants