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: always inline .bss in absence of a .data section #25837

Closed
wants to merge 1 commit into from

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented Jun 1, 2023

Due to an early vars == nil check in inlineGlobalData, ELFs without a .data section would not get their .bss references rewritten. This would cause verifier errors on pre-5.x kernels like 'unrecognized bpf_ld_imm64 insn', since those don't support pseudo-register load insns yet.

Delay the check and add a regression test.

bpf: always inline .bss in absence of a .data section

Due to an early vars == nil check in inlineGlobalData, ELFs without a .data
section would not get their .bss references rewritten. This would cause
verifier errors on pre-5.x kernels like 'unrecognized bpf_ld_imm64 insn',
since those don't support pseudo-register load insns yet.

Delay the check and add a regression test.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Reported-by: Quentin Monnet <quentin@isovalent.com>
@ti-mo ti-mo added kind/bug This is a bug in the Cilium logic. sig/loader Impacts the loading of BPF programs into the kernel. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jun 1, 2023
@ti-mo ti-mo requested a review from a team as a code owner June 1, 2023 19:11
@ti-mo ti-mo requested a review from rgo3 June 1, 2023 19:11
@ti-mo
Copy link
Contributor Author

ti-mo commented Jun 1, 2023

/test

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks!

}

if want, got := asm.R8, insns[0].Dst; want != got {
t.Errorf("unexpected Instruction OpCode: want: %s, got: %s", want, got)
Copy link
Contributor

Choose a reason for hiding this comment

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

OpCode -> Dst

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

The change breaks some of the unit tests, it introduces patterns such as:

             	150: (18) r2 = 0x0
            	152: (69) r2 = *(u16 *)(r2 +0)
            	R2 invalid mem access 'inv'

Observable here for this test PR. Unit tests were skipped on your PR because it doesn't touch bpf/ files.

@dylandreimerink
Copy link
Member

It looks like this change breaks global data which we use in the tests. For example in bpf/tests/xdp_nodeport_lb4_nat_lb.c we have a variable static __be16 nat_source_port;. It is zero initialized so it ends up in .bss, this change inlines it which should not happen. Giving the variables an initial value does not work, relocation entry moves to .data and then it also gets inlined.

I guess the tests always worked because we were hitting the edge case this PR is removing of not having any global data in .data so our global .bss variables would behave as expected.

I don't really know how to fix this one since we use global data for mocking and to confirm that mocked functions were invoked. Ideally we would mark our oddball Cilium variables in such a way that we can selectively inline them and not dynamic global variables.

@ti-mo
Copy link
Contributor Author

ti-mo commented Jun 6, 2023

This is a bad approach. We should instead try to avoid anything ending up in .bss by annotating all static variable declarations with .data or another snowflake section like .inline now that iproute2 is no longer used for loading.

We can then reject references to any other sections on pseudo-load instructions because variables likely ended up there by mistake. This would include rejecting references to .bss, which is a lot clearer than the verifier returning unrecognized bpf_ld_imm64 insn.

@ti-mo ti-mo closed this Jun 6, 2023
@ti-mo ti-mo deleted the tb/always-inline-bss branch June 6, 2023 15:50
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. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/loader Impacts the loading of BPF programs into the kernel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants