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

bpf: nodeport: only set outer src IP for tunnel encap in XDP #26726

Merged
merged 1 commit into from Jul 10, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Jul 10, 2023

As part of introducing native tunnel support for XDP, we started passing a src_ip parameter to __encap_with_nodeid(). This was needed to manually build the packet's outer headers.

But for TC context we actually don't want to specify the outer src IP, and defer its selection to the kernel stack instead. Worse, specifying the outer src IP means that the skb implementation for ctx_set_encap_info() starts to use the local_ipv4 field in the bpf_tunnel_key. And that's not supported on older kernels, resulting in drops.

Fix this by only setting the src_ip parameter in XDP context.

Reported-by: Yusuke Suzuki yusuke-suzuki@cybozu.co.jp
Fixes: 43dffb2 ("bpf: encap: manually set src IP and port from nodeport XDP paths")

@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. sig/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. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. area/loadbalancing Impacts load-balancing and Kubernetes service implementations needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jul 10, 2023
@julianwiedmann julianwiedmann requested a review from a team as a code owner July 10, 2023 05:07
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.0 Jul 10, 2023
As part of introducing native tunnel support for XDP, we started passing
a `src_ip` parameter to `__encap_with_nodeid()`. This was needed to
manually build the packet's outer headers.

But for TC context we actually don't want to specify the outer src IP, and
defer its selection to the kernel stack instead. Worse, specifying the
outer src IP means that the skb implementation for `ctx_set_encap_info()`
starts to use the `local_ipv4` field in the `bpf_tunnel_key`. And that's
not supported on older kernels, resulting in drops.

Fix this by only setting the `src_ip` parameter in XDP context.

Reported-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>
Fixes: 43dffb2 ("bpf: encap: manually set src IP and port from nodeport XDP paths")
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test

Copy link
Member

@ysksuzuki ysksuzuki left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

I tested this fix with flatcar stable 3510.2.4 and confirmed that it's working.
https://www.flatcar.org/releases#release-3510.2.4

$ uname -a
Linux rack0-cs5 5.15.117-flatcar

$ cilium config view
agent-not-ready-taint-key                      node.cilium.io/agent-not-ready
arping-refresh-period                          30s
auto-direct-node-routes                        false
bgp-announce-lb-ip                             true
bpf-ct-timeout-regular-any                     1h0m0s
bpf-ct-timeout-service-any                     1h0m0s
bpf-lb-acceleration                            disabled
bpf-lb-algorithm                               maglev
bpf-lb-dsr-dispatch                            geneve
bpf-lb-dsr-l4-xlate                            backend
bpf-lb-external-clusterip                      false
bpf-lb-map-max                                 65536
bpf-lb-mode                                    dsr
bpf-lb-sock                                    true
bpf-lb-sock-hostns-only                        true
bpf-map-dynamic-size-ratio                     0.0025
bpf-policy-map-max                             65536
bpf-root                                       /sys/fs/bpf
cgroup-root                                    /sys/fs/cgroup

@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 Jul 10, 2023
@julianwiedmann julianwiedmann merged commit 5e1139d into cilium:main Jul 10, 2023
65 checks passed
@julianwiedmann julianwiedmann deleted the 1.15-bpf-nodeport-tunnel branch July 10, 2023 13:40
@jibi jibi mentioned this pull request Jul 13, 2023
13 tasks
@jibi jibi added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jul 13, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.0 Jul 13, 2023
@aanm aanm added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Jul 14, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.0 Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/loadbalancing Impacts load-balancing and Kubernetes service implementations backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. 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-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.14.0
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

5 participants