elf: support weak functions

[pchaigno]

This commit adds support for the weak attribute on global functions. This attribute can be useful to prevent the compiler from changing the prototype of the function on some callsites.

This change was also tested in Cilium with a new global, weak function. Without this change, loading would fail with the following error:

offset 122800: relocating instruction: call: snat_v6_needs_masquerade: unsupported binding: STB_WEAK

[ti-mo]

@pchaigno Thanks for the patch! The reason we've held off on this so far is the object linking case: #466. I think this discussion stranded because we haven't decided what the linker should do when it encounters multiple weak candidates, and how the selection process would work. Might be worth looking at what libbpf is doing and using that behaviour to guide our implementation.

This attribute can be useful to prevent the compiler from changing the prototype of the function on some callsites.

Could you elaborate a bit on this? This would be a useful detail to take into account and write down somewhere.

[pchaigno]

Sorry for the late reply! I got pulled into an incident :(

This attribute can be useful to prevent the compiler from changing the prototype of the function on some callsites.

Could you elaborate a bit on this?

Here's an example if I try to make the snat_v6_needs_masquerade function in Cilium a global function, but without the weak attribute:

319: (18) r1 = 0xffffaaf080d2a004     ; R1_w=map_value(map=.rodata.config,ks=4,vs=157,off=4)
321: (71) r1 = *(u8 *)(r1 +0)         ; R1_w=14
[...]
372: (55) if r5 != 0x0 goto pc+100 473: R0=40 R1=14 R2_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=255,var_off=(0x0; 0xff)) R3_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=0xffff,var_off=(0x0; 0xffff)) R4_w=0 R5_w=scalar(umin=1) R6=pkt(off=14,r=54) R7_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R8=0 R9=scalar(id=4,smin=smin32=0,smax=umax=smax32=umax32=60,var_off=(0x0; 0x3f)) R10=fp0 fp-136=mmmm???? fp-144=mmmm14 fp-152=????0 fp-160=????5 fp-168=map_value(map=ct_tuple_storag,ks=4,vs=38) fp-176=????scalar(id=1,smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) fp-184=ctx() fp-192=map_value(map=nat_target_stor,ks=4,vs=28) fp-208=0
; ret = snat_v6_needs_masquerade(ctx, fraginfo, l4_off); @ nodeport_egress.h:91
473: (85) call pc+2889
arg#0 expects pointer to ctx
Caller passes invalid args into func#1 ('snat_v6_needs_masquerade')

For some callsites, LLVM ends up optimizing the arguments. In the above example, it seems to skip the first argument, ctx. R1 thus holds a SCALAR instead of the PTR_TO_CTX that the verifier expected.

@dylandreimerink suggested using __atttribute__(weak) to work around that. I'm unsure why LLVM optimizes the global function in this way, but it makes sense marking it weak would prevent any such optimization.

I think this discussion stranded because we haven't decided what the linker should do when it encounters multiple weak candidates, and how the selection process would work. Might be worth looking at what libbpf is doing and using that behaviour to guide our implementation.

As per Dylan's experiments, libbpf seems to combine the weak candidates. That looks a bit weird to me, but I guess it's fine. That being said, couldn't we simply accept a single weak function and reject if there are multiple? That would at least allow for the LLVM workaround I mentioned above.

[ti-mo]

@pchaigno Cool, thanks for the explanation! That makes sense. I wonder if declaring the function static would have the same effect, since the function would have to verify regardless of callsite, meaning some optimizations cannot be performed.

[...] libbpf seems to combine the weak candidates. That looks a bit weird to me [...]

Indeed, if the goal is to pick the first candidate anyway (which libbpf does), it may as well throw away the other versions. There may be some technical limitation preventing this, haven't looked into why.

[..] couldn't we simply accept a single weak function and reject if there are multiple?

Yep, that's the idea. We'll cut the insn stream right before the instruction with the second func_info in the symbol. Same for maps, their static allocations in maps/.maps also get pasted together, so they need to be cut at the appropriate offset.

ICYMI, this PR is superseded by #1942 which implements the above. Thanks for flagging!