Add support for function pointers

[pippolo84]

This PR adds support for function pointer relocations, following the same logic in libbpf: torvalds/linux@53eddb5

For example, this kind of relocation is needed when using helpers such as:
long bpf_for_each_map_elem(struct bpf_map *map, void *callback_fn, void *callback_ctx, u64 flags)
which takes a callback as a parameter.

The kernel selftests for_each_array_{array,map}_elem have been enabled as they should be supported now.

Fixes #467

[lmb]

Thanks for the PR! I've left a couple of comments, it looks good overall :)

Can you add some tests outside of the libbpf integration please? It's not trivial to debug those if they fail, and we're pretty lax when getting errors from these tests. I think you should be able to adjust loader.c to include a load of a function pointer.

[pippolo84]

Thanks for the PR! I've left a couple of comments, it looks good overall :)

Can you add some tests outside of the libbpf integration please? It's not trivial to debug those if they fail, and we're pretty lax when getting errors from these tests. I think you should be able to adjust loader.c to include a load of a function pointer.

Hi @lmb , thank you for your comments. I'll polish the code and add some tests as you suggested.

[pippolo84]

I added a test in elf_reader_test.go for subprogram relocation. AFAIK this kind of relocation is supported from 5.13 onwards (running the test on with my distro kernel, v5.10, leads to a fail, instead it passes on a v5.15. And the same goes using libbpf).

To avoid complicating things too much in loader.c, I wrote a separate BPF program and Go test, adding testutils.SkipOnOldKernel to skip it for older kernel.

[pippolo84]

I realized that I misunderstood the reason why the test needs a kernel >= 5.13.
I used the bpf helper for_each_map_elem, that's the reason for the requirement. Is this acceptable or should I modify the test to run on earlier kernel version?
I thought about using that helper because it seemed the most common case to exercise the subprogram relocation with ld_imm64 instruction.

Regarding the CI failure I find it surprising: maybe just a matter of flakyness? Unfortunately I am not able to see the output from the failing test on semaphore.

[lmb]

I think requiring 5.13 for the test is OK as long as it's documented why. What use is there for subprog loads before for_each_map_elem appeared?

[lmb]

So a load of a static function doesn't require the same offset fixup as for Call?

[pippolo84]

No, they are relocated in a slightly different way: https://github.com/libbpf/libbpf/blob/472c0726e84d821186a315889c885b23895b155e/src/libbpf.c#L5963-L5966

[pippolo84]

I think requiring 5.13 for the test is OK as long as it's documented why. What use is there for subprog loads before for_each_map_elem appeared?

TBH I am not aware of any other case. Digging a little bit through the relevant kernel patches it seems that this kind of relocation has been introduced to support that helper in the first place: https://www.spinics.net/lists/bpf/msg35623.html

[lmb]

Thanks!

[pippolo84]

Thanks!

Thank you for your review and comments!