bpf: introduce libbpf dependency; replace some bpf syscall with libbpf API #13744
Conversation
417bd6f to
5bad0d6
Compare
|
In order to push this forward, some help from the community is needed, so I'm tagging people :)
So, a general question: let's pretend libbpf package is available on all relevant platforms, what are the steps to introduce it as a dependency to systemd and testing infra? How new package dependencies are usually added? cc @keszybz @mrc0mmand @poettering As an alternative, I don't mind reviving submodule approach #12151 but it also needs support from testing infra so ubuntu bionic tests are unblocked. Thanks for all you replies! [1] https://lore.kernel.org/netdev/A273A3DB-C63E-488F-BB0C-B7B2AB6D99BE@fb.com/ |
|
@wat-ze-hex regarding CentOS CI, I believe Regarding should make that part green. |
|
I've just realized that |
|
@evverx Just to clarify, do you mean |
|
@wat-ze-hex I was talking about
|
|
@wat-ze-hex I think what I was trying to say is documented in https://github.com/google/oss-fuzz/blob/97060c44dec9c25b8c74a4ee05a217bd68f119fe/docs/further-reading/fuzzer_environment.md#runtime-dependencies. When the fuzzers were introduced we kind of ignored that, made some assumptions about the runtime environment and somehow convinced the OSS-Fuzz team to add an undocumented feature allowing us to also use Ideally, we should fix the fuzzers but if it takes too long another option would be to use my YOLO approach from #12608 where both Anyway, I don't think at this point it's important enough. I'm pretty sure we'll figure something out eventually. My understanding is that it would be better to decide where the PR is headed in general. I hope it makes sense. |
|
@wat-ze-hex as for the CentOS 7 job, I could rebuild the |
keszybz
left a comment
There was a problem hiding this comment.
Please explain what is happening and why... Some things look suspicious, but without comments it's hard to understand why they are done this way.
I see that runtime dependency is a concern. What about libbpf linked statically, i.e.: In the case of static linking there is still a question of how to link the dependencies of libbpf – |
That would be very helpful, thank you! As for EPEL packaging it's WIP, the conversation can be tracked in [1] [1] https://lore.kernel.org/netdev/20191008073958.GA10009@krava/ |
|
fwiw, I think it's way too early to add a dependency on libbpf. |
I'll have to find out whether these are available there. Though, I was told (repeatedly) not to rely on anything. What I'm concerned about is that even if they are there it's going to be another stopgap we'll never fix (or, more precisely, the part linking
@mbiebl Given that some (probably most) projects heavily relying on BPF (most notably https://github.com/iovisor/bcc) have already switched to |
|
Well, I did a quick search and bcc seems to be supported neither in yocto nor buildroot afaict (there are some patches under buildroot, but they are still under consideration). I couldn't find libbpf either, but I didn't dig that deep. This is usually a strong sign that those libraries do not support cross-compilation which is essential for the embedded world. So... this would probably block any systemd upgrade in the embedded world, which is bad. especially if it's a mandatory dependency. |
@boucman have you tried to cross-compile it? If it doesn't work, it's possible to open an issue on their bug tracker at https://github.com/libbpf/libbpf. All issues I opened there were closed in less than a week so in my experience the project is very responsive. I'd also like to point out that |
I'm not sure if it was discussed at All Systems Go but given that it's going to be a normal dependency (as opposed to a manually implemented part of systemd), it seems it would make sense to make it possible to make it optional like almost everything else. Another option would be to keep using the old stuff if the library isn't available (so that the BPF part of systemd won't stop working suddenly) effectively cementing the status quo where libbpf won't be seen in the foreseeable future but without any updates and bug fixes. New features and bug fixes would be present in the maintained "libbpf" part only and if those features and bug fixes are important enough for, for example, embedded folks they would find to a way to bring libbpf there. |
That's a chicken and egg problem: packages don't pop up by themselves unless driven by a use case, a use case is blocked by absence of packages. |
The COPR EPEL7 build is pending, shouldn't take much longer. As for the Arch Linux jobs, I guess I'll finally have to learn how to use AUR, so we can test it there as well. |
|
@wat-ze-hex the CentOS 7 job finally got past the meson setup phase: |
|
@wat-ze-hex as for Arch Linux - I wrote a simple Is it worth pushing into the Arch User Repository (AUR) or you already have plans to do it yourself (either to AUR or core)? |
Summarizing, I would think about cementing the present stuff (which is maintenance anyway) only if there is no other way. And only with proclaimed EoL date. |
|
Thank you! I mentioned that in the kernel mailing list. Alternatively, I can do that. Could you please provide a brief instruction due to I haven't published anything to AUR yet. And thanks again. |
Could you please file a bug report to [1] if you see any issues with cross compilation? We can also think about a test for that, if given proper repro steps. Is there a test which checks x-compilation for |
I'll look into that, as I'll have to monitor the libbpf version anyway for the COPR repo (at least until its present in the EPEL) :-) |
|
@wat-ze-hex hmm, I noticed that libbpf seems to already part of this AUR package: https://aur.archlinux.org/packages/bcc-tools/, so I guess we could use that. |
|
Anyway, frankly, using AUR is not that much different than just cloning the libbpf repo and compiling it (as AUR packages have to be compiled locally anyway). Thus, I'm inclining to just add the |
This looks fine with the exception that we don't need So yeah, let's use use bcc in Arch tests. |
163fbed to
480ef09
Compare
|
Seems that the present
That needs a fix. |
the journal is saved for some tests, in the 'artifacts' tarball; but for test-exec (run by the 'root-unittests' autopkgtest) the journal doesn't get saved. I'll open a Debian MR to update the test to save the journal on failure, but even then it will still be in the 'artifacts' tarball. You can get that tarball by using the same URL for the log.gz file, but replace 'log.gz' at the end with 'artifacts.tar.gz'. The Ubuntu CI server could use improvement since that's not obvious at all.
I'm not exactly sure, but there were infrastructure problems earlier this week that might be the cause. You can check this page: https://status.admin.canonical.com/ |
|
Any advice on how to repro Keeping running into this: Failing w/ e.g. |
|
See https://www.freedesktop.org/wiki/Software/systemd/autopkgtest/ for how to locally reproduce and investigate the test failures. |
cba32f4 to
20d2ff7
Compare
|
Ping :) This PR needs some <3.
Thoughts? Thanks! |
|
@wat-ze-hex how safe is to drop the newly-introduced [0] https://src.fedoraproject.org/rpms/libbpf/blob/master/f/libbpf.spec#_28 |
|
Having Bumping to [1] https://apps.fedoraproject.org/packages/libbpf/overview/ |
I see. I completely understand the need of having a dependency on
Ah, I see, will wait a bit then!
|
poettering
left a comment
There was a problem hiding this comment.
Hmm, what does libbpf bring to the table for this btw? it's just a thin, trivial wrapper around the syscall, no? i.e. if it's that simple is the extra dependency really worth it? is there anything non-trivial it provides us with?
If i understand correctly its not even a shared lib yet, i.e. unstable API?
Compare with the keyring APIs. Yes there's a keyring library, but we don't link to it, since all we really need from it are trivial syscall wrappers which is are like 5 lines each. For that kind of stuff its not really worth linking to libbpf, is it?
Can you elaborate?
(I am not strictly against adding new deps, but each new dep must have a strong motivation, i.e. do something substantially more complex thatn just syscall wrapping of a single syscall...)
|
|
||
| int bpf_devices_cgroup_init(BPFProgram **ret, CGroupDevicePolicy policy, bool whitelist) { | ||
| log_error("BPF is not supported. Mock is called"); | ||
| return -1; |
There was a problem hiding this comment.
we exclusively return negative errno-style errors here. i.e. usw return -EOPNOTSUPP ...
There was a problem hiding this comment.
or actually:
return log_error_errno(EOPNOTSUPP, "BPF …");| const char *cgroup_path, | ||
| BPFProgram **prog_installed) { | ||
| log_error("BPF is not supported. Mock is called"); | ||
| return -1; |
| static void unit_prune_cgroup_bpf(Unit *u) { | ||
| #if HAVE_LIBBPF | ||
| u->bpf_device_control_installed = bpf_program_unref(u->bpf_device_control_installed); | ||
| #endif |
There was a problem hiding this comment.
such ifdeffery should be left to the implementation of bpf_program_unref(), not the users
| int bpf_devices_whitelist_static(BPFProgram *prog, const char *path) { | ||
| log_error("BPF is not supported. Mock is called"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
we usually keep such "not implemented" stubs as inline functions in the headers, so that they are optimized away...
| assert(paths); | ||
|
|
||
| #if !(HAVE_LIBBPF) | ||
| log_syntax(unit, LOG_ERR, filename, line, -1, "libbpf is not found, ignoring %m"); |
There was a problem hiding this comment.
do not make up error codes... i.e. no -1 please. use 0 here...
also why have this ifdeffery at all here? when bpf is disabled we should still compile the parsers for this, and this function at its end already covers the non-bpf case...
| memcpy(key_ipv4->data, &a->address, sizeof(uint32_t)); | ||
|
|
||
| r = bpf_map_update_element(ipv4_map_fd, key_ipv4, &value); | ||
| r = bpf_map_update_elem(ipv4_map_fd, key_ipv4, &value, BPF_ANY); |
There was a problem hiding this comment.
how does libbpf do error handling? we use negative return -errno style returns, and what does libbpf do? if they return -1 and set errno, this needs more love
|
Thanks for reviewing!
In this PR it's a syscall replacement: I'm not comfortable with introducing more changes in a single pull request, prefer smaller atomic changes. Though I can continue adding commits with functional changes in this PR, namely replacing macro assembly with C code. That's merely a matter of code organization. If only there were a way to stack PRs, so major features like this are not merged at once.
It's a shared lib. There is Fedora package also @mrc0mmand created a package for CentOS and |
|
Hmm, so I think I'd prefer sticking with our syscall wrappers for now, i.e. use libbpf only if it brings more to the table for us than just syscall wrappers. I hope that makes sense? |
|
Sure, I'll extend this PR beyond syscall wrappers, namely replace raw bpf instructions with programs in C. Will request re-review once it's ready. |
libbpf-dev is a Debian libbpf package built from kernel sources. That is suboptimal due to Debian libbpf updates are not tied to libbpf release process including tagging and testing. Nevertheless it's used to unblock Debian related testing in TravisCI until a proper packege built from github mirror [1] is out. [1] https://github.com/libbpf/libbpf
bpf-firewall.h is not used in src/core/dbus-unit.c and src/core/ip-address-access.c so remove it.
Introduce HAVE_LIBBPF macro checking the presence of libbpf package
in the system;
Introduce mock implementation of BPF interface in
bpf-{firewall|devices|program}-mock.c, use mock implementation if
libbpf is not found;
Mock implementation of bpf_{firewall|devices}_supported returns 0,
so other BPF dependent doesn't get executed.
In order to rewrite src/core/bpf-devices.{c|h},
src/core/bpf-devices/bpf-firewall.{c|h} from macro-assembly to eBPF, libbpf
library must be linked to libcore.a.
Replace
int bpf_map_new(enum bpf_map_type type, size_t key_size, size_t value_size, size_t max_entries, uint32_t flags);
int bpf_map_update_element(int fd, const void *key, void *value);
int bpf_map_lookup_element(int fd, const void *key, void *value);
with functions from libbpf.
Replace BPF_PROG_LOAD, BPF_PROG_LOAD, BPF_OBJ_GET syscalls.
|
Something's off: |
An attempt to replace direct bpf syscalls with libbpf was made in #12151 where libbpf was introduced as a git submodule.
This is another approach with libbpf as a library.
libbpfpackage build from GH mirror is available in Fedora [1].Debian package exists as well but it is build from kernel sources [2].
There is no CentOS package but it's possible to make one by rebuild the Fedora package and publishing it to EPEL repo.
I expect initial test failures, because they may lack of libbpf package installed thus libbpf dependency unresolved.
[1] https://rpms.remirepo.net/rpmphp/zoom.php?rpm=libbpf
[2] https://packages.debian.org/sid/libbpf-dev