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.

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

daemon: Do not attach bpf_host to L3 dev if skb_change_head is unavailable #15343

Merged
merged 2 commits into from
Apr 13, 2021
Merged

daemon: Do not attach bpf_host to L3 dev if skb_change_head is unavailable #15343

merged 2 commits into from
Apr 13, 2021

Conversation

brb
Copy link
Member

@brb brb commented Mar 15, 2021 •
edited

See commit msgs.

@brb brb added the release-note/ci This PR makes changes to the CI. label Mar 15, 2021
@brb brb requested a review from a team March 15, 2021 08:32
@brb brb requested a review from a team as a code owner March 15, 2021 08:32
@brb brb requested review from jrfastab and twpayne March 15, 2021 08:32
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 15, 2021
@brb brb mentioned this pull request Mar 15, 2021
Merged
pchaigno
pchaigno reviewed Mar 15, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Is ETH_HLEN only ever used in bpf_lxc (i.e., not in bpf_host)?

@pchaigno pchaigno added the area/CI Continuous Integration testing issue or flake label Mar 15, 2021
twpayne
twpayne approved these changes Mar 15, 2021
View reviewed changes
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

LGTM

@brb
Copy link
Member Author

brb commented Mar 17, 2021

Is ETH_HLEN only ever used in bpf_lxc (i.e., not in bpf_host)?

@pchaigno In bpf_lxc it's used for NodePort BPF rev-DNAT.

@brb
Copy link
Member Author

brb commented Mar 17, 2021

test-me-please

@pchaigno
Copy link
Member

pchaigno commented Mar 19, 2021 •
edited

Provisioning error:
test-1.20-4.9

@pchaigno
Copy link
Member

pchaigno commented Mar 19, 2021 •
edited

Provisioning error:
test-1.13-netnext

@pchaigno
Copy link
Member

k8s-1.19-kernel-4.19 hit a complexity issue.

@brb
Copy link
Member Author

brb commented Mar 19, 2021

I think 4.19 hit the following:

R0=inv(id=0,umin_value=18446744071562067968,var_off=(0xffffffff80000000; 0x7fffffff)) R1=pkt_end(id=0,off=0,imm=0) R2=pkt(id=0,off=40,r=40,imm=0) R6=ctx(id=0,off=0,imm=0) R7=inv4294967162 R8=inv40 R9=ctx(id=0,off=0,imm=0) R10=fp0,call_-1 fp-240=0 fp-248=0 fp-256=0 fp-264=0 fp-272=0 fp-280=0 fp-296=ctx fp-304=map_value fp-320=0
	 2562: (61) r6 = *(u32 *)(r9 +16)
	 2563: (bf) r1 = r9
	 2564: (b7) r2 = 14
	 2565: (b7) r3 = 0
	 2566: (85) call bpf_skb_change_head#43
	 unknown func bpf_skb_change_head#43

The helper is not available in 4.19.

@pchaigno
Copy link
Member

Ah, I guess it may be easier to fix? :-S

The helper was added in 4.10 but is accessible from tc BPF programs only since v5.8 with commit 6f3f65d. I guess we now need a probe for this.

@pchaigno pchaigno marked this pull request as draft March 23, 2021 08:02
@brb brb force-pushed the pr/brb/inc-complexity-eth-len-zero branch from 5565d72 to ef0fdbb Compare April 2, 2021 14:26
@brb brb changed the title bpf: Add ETH_HLEN=0 to stress test complexity daemon: Do not attach bpf_host to L3 dev if skb_change_head is unavailable Apr 2, 2021
@brb brb marked this pull request as ready for review April 2, 2021 15:05
@brb brb requested review from a team and jibi April 2, 2021 15:05
@pchaigno pchaigno removed their assignment Apr 6, 2021
pchaigno
pchaigno reviewed Apr 7, 2021
View reviewed changes
bpf/Makefile Outdated Show resolved Hide resolved
@brb brb force-pushed the pr/brb/inc-complexity-eth-len-zero branch from 2160db7 to ac34e44 Compare April 9, 2021 05:26
@brb
Copy link
Member Author

brb commented Apr 9, 2021

test-net-next

@pchaigno
Copy link
Member

pchaigno commented Apr 9, 2021 •
edited

Previous net-next job had only flakes AFAICS, but even if the changes are fairly simple, I'm not comfortable merging without the full end-to-end tests.

test-me-please

@brb
Copy link
Member Author

brb commented Apr 9, 2021 •
edited

CI 4.9 and GKE:

Egress DNS connectivity should be allowed for pod "app2-5cc5d58844-xwz5j"
Expected command: kubectl exec -n 202104091050k8spolicytestbasictestchecksallkindofkubernetespoli app2-5cc5d58844-xwz5j -- host -v -R3 -N1 -t A kubernetes.default.svc.cluster.local. 
To succeed, but it failed:
Exitcode: 1 
Err: exit status 1
Stdout:
 	 Trying "kubernetes.default.svc.cluster.local"
	 Host kubernetes.default.svc.cluster.local not found: 5(REFUSED)
	 Received 54 bytes from 10.115.240.10#53 in 2 ms

@brb
Copy link
Member Author

brb commented Apr 9, 2021

test-4.9

@brb
Copy link
Member Author

brb commented Apr 9, 2021

test-gke

@brb
Copy link
Member Author

brb commented Apr 9, 2021

CI net-next hit #15455.

@brb
Copy link
Member Author

brb commented Apr 9, 2021

test-net-next

@brb brb marked this pull request as draft April 9, 2021 14:33
@brb
Copy link
Member Author

brb commented Apr 9, 2021

Converted this to draft, as this is blocked by #15565.

brb added 2 commits April 13, 2021 08:48
@brb
bpf: Add ETH_HLEN=0 to stress test complexity
1a699db 
The PR "datapath: Support NodePort BPF on L2-less devices" [1] has
increased the complexity of bpf_host and bpf_lxc by introducing a
support for ETH_HLEN=0.

Extend the base options by adding ETH_HLEN=0 to stress test the verifier
complexity when running on net-next (ETH_HLEN=0 depends on the
skb_change_head helper which was introduced in 5.8).

[1]: #14858

Suggested-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
daemon: Ignore L3 devices on unsupported kernels
c14f323 
Skip devices which don't have L2 addr, if the kernel doesn't have
skb_change_head helper (the case for < 5.8 kernels).

Without the helper it is not possible to create the headroom for L2 hdr.
Unfortunately, "skb_adjust_room(skb, 14, BPF_ADJ_ROOM_MAC, 0)" cannot be
used either, as it does not set "skb->mac_header" which makes the packet
to be dropped by [1] when redirecting to another device.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/core/filter.c?h=v5.8#n2118

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/inc-complexity-eth-len-zero branch from ac34e44 to c14f323 Compare April 13, 2021 06:59
@brb brb marked this pull request as ready for review April 13, 2021 06:59
@brb brb requested a review from a team as a code owner April 13, 2021 06:59
@brb
Copy link
Member Author

brb commented Apr 13, 2021

test-me-please

pchaigno
pchaigno approved these changes Apr 13, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Mostly to catch the newly-required cilium/loader review, but I also reviewed latest changes.

@brb
Copy link
Member Author

brb commented Apr 13, 2021

@brb
Copy link
Member Author

brb commented Apr 13, 2021

test-4.9

@brb
Copy link
Member Author

brb commented Apr 13, 2021

test-4.19

@brb
Copy link
Member Author

brb commented Apr 13, 2021

CI 4.19 hit #15455. Marking as ready-to-merge.

@brb brb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 13, 2021
@qmonnet qmonnet merged commit 27ae873 into master Apr 13, 2021
1.10.0 automation moved this from In progress to Done Apr 13, 2021
@qmonnet qmonnet deleted the pr/brb/inc-complexity-eth-len-zero branch April 13, 2021 16:13
This was referenced Apr 28, 2021
Closed
Merged
@pchaigno pchaigno mentioned this pull request Mar 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twpayne twpayne twpayne approved these changes

@jibi jibi jibi approved these changes

@borkmann borkmann borkmann approved these changes

@jrfastab jrfastab jrfastab approved these changes

@pchaigno pchaigno pchaigno approved these changes

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@brb @pchaigno @twpayne @jibi @borkmann @jrfastab @christarazi @qmonnet