possibility to make libbpf dependency static or to support git submodule approach though this is not standard way for systemd.

Please, don't. Either libbpf is ready for production and thus provides a stable interface, and then it can be depended upon like any other system library that's already in use, or it isn't and hence it should not be used in core projects like systemd at all.

In fact, looking at how/where it's used I'd say it's a good candidate for dynamic loading via dlopen at runtime - please see how other libraries like cryptsetup are handled, eg: https://github.com/systemd/systemd/blob/master/src/shared/cryptsetup-util.c

@bluca

Please, don't. Either libbpf is ready for production and thus provides a stable interface, and then it can be depended upon like any other system library that's already in use, or it isn't and hence it should not be used in core projects like systemd at all.

I think API/ABI compatibility is a lesser concern since it's fully in control of kernel community.
Relying on distribution packaging though means that there will be a time lag between top-notch features available and features which can be used in systemd. A user space developer has to keep in mind which BPF feature available on which kernel, dynamic linking adds another feature-to-distribution mapping on top of that.

That means we can't introduce features which are both mandatory and recent.

In fact, looking at how/where it's used I'd say it's a good candidate for dynamic loading via dlopen at runtime - please see how other libraries like cryptsetup are handled, eg: https://github.com/systemd/systemd/blob/master/src/shared/cryptsetup-util.c

Thanks for the pointer, taking a look!

Converting to draft to add feature gate in meson_options.txt, attach type probing and to document better.
Also investigating compilation failure in Arch Linux

@bluca

Please, don't. Either libbpf is ready for production and thus provides a stable interface, and then it can be depended upon like any other system library that's already in use, or it isn't and hence it should not be used in core projects like systemd at all.

I think API/ABI compatibility is a lesser concern since it's fully in control of kernel community.
Relying on distribution packaging though means that there will be a time lag between top-notch features available and features which can be used in systemd. A user space developer has to keep in mind which BPF feature available on which kernel, dynamic linking adds another feature-to-distribution mapping on top of that.

That means we can't introduce features which are both mandatory and recent.

Speaking as a downstream once-removed distro maintainer at $work, in my opinion this is not a problem at all - in fact, it's entirely expected. It's how every other existing dependency works - I'm not seeing anything that indicates this is special in any way to warrant a different, custom treatment for this one option. As long as every requirement is clearly indicated, it's fully expected that optional features do not work if the minimum build and runtime requirements are not met.
For example, to make use of dm-verity signatures (RootHashSignature, etc), you need at least kernel >= 5.4 with the relevant kconfig enabled (disabled by default), and a more recent version of libcryptsetup than the minimum requirement. That is fine, because it's clearly documented: https://github.com/systemd/systemd/blob/master/README and so are plenty of other kernel-related or libs-related features (user namespaces, network features, etc etc).

In fact, looking at how/where it's used I'd say it's a good candidate for dynamic loading via dlopen at runtime - please see how other libraries like cryptsetup are handled, eg: https://github.com/systemd/systemd/blob/master/src/shared/cryptsetup-util.c

Thanks for the pointer, taking a look!

@bluca

Speaking as a downstream once-removed distro maintainer at $work, in my opinion this is not a problem at all - in fact, it's entirely expected. It's how every other existing dependency works - I'm not seeing anything that indicates this is special in any way to warrant a different, custom treatment for this one option. As long as every requirement is clearly indicated, it's fully expected that optional features do not work if the minimum build and runtime requirements are not met.

Listing trade-offs I can think about.

Static

(+) Ability to introduce new libbpf features is not limited by presence of recent versions in distributions.
(+) End users don't need to worry about installing and updating libbpf on every host where systemd runs, only on those where it builds.
(+) Features can be declared mandatory on 4.9+ kernels.
(-) Increases binary size and compile time.

Shared

Basically, swap pros and cons of static:
(+) No binary size and compile time overhead.
(+) Standard for systemd development.
(-) Package availability in distros is a requirement.
(-) End users have to worry about one more package
(-) Only optional features.

In the patch set I also included libbpf to libsystemd-shared bundle:

$ ldd ./src/shared/libsystemd-shared-247.so
	linux-vdso.so.1 (0x00007ffcb9902000)
	libblkid.so.1 => /lib64/libblkid.so.1 (0x00007f6c5ec60000)
	libbpf.so.0 => /lib64/libbpf.so.0 (0x00007f6c5ec31000)
	libcap.so.2 => /lib64/libcap.so.2 (0x00007f6c5ec2a000)
	libcrypt.so.2 => /lib64/libcrypt.so.2 (0x00007f6c5ebef000)
	libmount.so.1 => /lib64/libmount.so.1 (0x00007f6c5eb8f000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f6c5eb84000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f6c5ea3c000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f6c5ea35000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f6c5ea13000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f6c5e849000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f6c5f00e000)
	libelf.so.1 => /lib64/libelf.so.1 (0x00007f6c5e82e000)
	libz.so.1 => /lib64/libz.so.1 (0x00007f6c5e814000)
	libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f6c5e7e5000)
	libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f6c5e74c000)

I think this straightens dynamic linking approach by simplifying libbpf delivery to end users. Wdyt?

@bluca

Speaking as a downstream once-removed distro maintainer at $work, in my opinion this is not a problem at all - in fact, it's entirely expected. It's how every other existing dependency works - I'm not seeing anything that indicates this is special in any way to warrant a different, custom treatment for this one option. As long as every requirement is clearly indicated, it's fully expected that optional features do not work if the minimum build and runtime requirements are not met.

Listing trade-offs I can think about.

Static

(+) Ability to introduce new libbpf features is not limited by presence of recent versions in distributions.
(+) End users don't need to worry about installing and updating libbpf on every host where systemd runs, only on those where it builds.
(+) Features can be declared mandatory on 4.9+ kernels.
(-) Increases binary size and compile time.

The problem is not so much binary size, although a few folks care about that for certain important and popular use cases, so it's certainly worth mentioning. In my view, the most important factor by some distance is security maintenance. Distributions rely on being able to update a library with a security bug without going around guessing whatever might have embedded copies all over the place. There are over fifty thousand binary packages in Debian, if every single one of them had embedded random libraries it would pretty much be game over.

Moreover, the maintainers here can correct me if I'm wrong, but my distinct impression is that the systemd distribution model is not random users installing random things - but you get and use what your distribution gives you. I do not see random end users building their own systemd. Good integration and synergy with the rest of the distro is paramaunt to a functional system. So in my opinion it's not really worth it to worry about what end users might or might not have to do. Developers testing things are qualified enough to look at the documentation, and personally I do not see a reason to believe this is particular one is any different from the other dozen or so optional dependencies.

For example, iproute2 just got support for libbpf, as a normal shared library like many others. I maintain it in Debian, and once it's released I'll enable it and ensure the proper requirements are set, and that will be it - no reason to do anything special, as libbpf is just another dependency like libmnl or libelf or libselinux etc etc

Shared

Basically, swap pros and cons of static:
(+) No binary size and compile time overhead.
(+) Standard for systemd development.
(-) Package availability in distros is a requirement.
(-) End users have to worry about one more package
(-) Only optional features.

In the patch set I also included libbpf to libsystemd-shared bundle:

$ ldd ./src/shared/libsystemd-shared-247.so
	linux-vdso.so.1 (0x00007ffcb9902000)
	libblkid.so.1 => /lib64/libblkid.so.1 (0x00007f6c5ec60000)
	libbpf.so.0 => /lib64/libbpf.so.0 (0x00007f6c5ec31000)
	libcap.so.2 => /lib64/libcap.so.2 (0x00007f6c5ec2a000)
	libcrypt.so.2 => /lib64/libcrypt.so.2 (0x00007f6c5ebef000)
	libmount.so.1 => /lib64/libmount.so.1 (0x00007f6c5eb8f000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f6c5eb84000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f6c5ea3c000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f6c5ea35000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f6c5ea13000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f6c5e849000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f6c5f00e000)
	libelf.so.1 => /lib64/libelf.so.1 (0x00007f6c5e82e000)
	libz.so.1 => /lib64/libz.so.1 (0x00007f6c5e814000)
	libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f6c5e7e5000)
	libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f6c5e74c000)

I think this straightens dynamic linking approach by simplifying libbpf delivery to end users. Wdyt?

There's been a trend to move optional dependencies to dlopen(), to minimize hard requirements for container images and so on. It's the maintainers call how to handle this one of course, but in my view it's a good candidate to be handled that way too. Eg, see: https://github.com/systemd/systemd/blob/master/src/shared/cryptsetup-util.c

However, it can of course happen later as a follow-up if the maintainers want to handle it that way, I don't think we need to worry about it now. Current approach looks fine.

CentOS CI logs indicate that libbpf package in ArchLinux was bumped up 0.3, whereas the latest release is 0.2

01:26:51 archlinux_systemd_ci: Run-time dependency libbpf found: YES 0.3.0

cc @mrc0mmand: Is 0.3 the latest libbpf but versioned differently?

That also adds a point about how libbpf dependent features will work with customly versioned libbpf packages residing in private and corporate repos.

CentOS CI logs indicate that libbpf package in ArchLinux was bumped up 0.3, whereas the latest release is 0.2

01:26:51 archlinux_systemd_ci: Run-time dependency libbpf found: YES 0.3.0

cc @mrc0mmand: Is 0.3 the latest libbpf but versioned differently?

CentOS CI uses the master branch of the libpf repo, so it's usually a newer version than the latest stable release.

@bluca

Looking at dlopen, major version of shared lib is always hardcoded in search path:

find src/shared -name '*.[ch]' | xargs grep dlopen\(
src/shared/cryptsetup-util.c:        dl = dlopen("libcryptsetup.so.12", RTLD_LAZY);
src/shared/idn-util.c:        dl = dlopen("libidn2.so.0", RTLD_LAZY);
src/shared/idn-util.c:        dl = dlopen("libidn.so.12", RTLD_LAZY);
src/shared/idn-util.c:                dl = dlopen("libidn.so.11", RTLD_LAZY);
src/shared/pwquality-util.c:        dl = dlopen("libpwquality.so.1", RTLD_LAZY);
src/shared/qrcode-util.c:        dl = dlopen("libqrencode.so.4", RTLD_LAZY);
src/shared/userdb.c:        dl = dlopen(ROOTLIBDIR "/libnss_systemd.so.2", RTLD_LAZY|RTLD_NODELETE);
src/shared/userdb.c:                log_debug("Failed to dlopen(libnss_systemd.so.2), ignoring: %s", dlerror());

Bumping major in a distro will cause systemd to out-out from codepath depending on a lib.
Have it ever been an issue for any of the listed libs? Or the preferred way for systemd is to bump the major in search path early so distributions should catch up?

@bluca

Looking at dlopen, major version of shared lib is always hardcoded in search path:

find src/shared -name '*.[ch]' | xargs grep dlopen\(
src/shared/cryptsetup-util.c:        dl = dlopen("libcryptsetup.so.12", RTLD_LAZY);
src/shared/idn-util.c:        dl = dlopen("libidn2.so.0", RTLD_LAZY);
src/shared/idn-util.c:        dl = dlopen("libidn.so.12", RTLD_LAZY);
src/shared/idn-util.c:                dl = dlopen("libidn.so.11", RTLD_LAZY);
src/shared/pwquality-util.c:        dl = dlopen("libpwquality.so.1", RTLD_LAZY);
src/shared/qrcode-util.c:        dl = dlopen("libqrencode.so.4", RTLD_LAZY);
src/shared/userdb.c:        dl = dlopen(ROOTLIBDIR "/libnss_systemd.so.2", RTLD_LAZY|RTLD_NODELETE);
src/shared/userdb.c:                log_debug("Failed to dlopen(libnss_systemd.so.2), ignoring: %s", dlerror());

Bumping major in a distro will cause systemd to out-out from codepath depending on a lib.
Have it ever been an issue for any of the listed libs? Or the preferred way for systemd is to bump the major in search path early so distributions should catch up?

Not so far, but this usage is very new. The issue you raised is a valid one, and getting the soname at build time rather than hard-coding, and other issues, are currently being discussed here: #17416

This would be a problem only in case of ABI-but-not-API break (if the soname changes because of an API break, there are code changes requires anyway), which is less common, but of course very much possible, and it very much does happen.

@bluca

Thanks for the pointer once again!

Is there a rule of thumb to separate weak dependencies from critical? The present BPF-powered features in systemd are responsible for security (firewall, device control), same as AllowBind introduced with this PR. IMO graceful degradation shouldn't be the case for security.

Present features are implemented with 'assembly' bpf_insn and depend only on kernel. Favoring dlopen approach means that both firewall and devices can not be migrated to human friendly C code since this is security relaxation: not having libbpf.so on target host would be enough to opt out.

I would love to replace BPF firewall with C code and add ports support, but what's the point if it can be bypassed.

@bluca

Thanks for the pointer once again!

Is there a rule of thumb to separate weak dependencies from critical? The present BPF-powered features in systemd are responsible for security (firewall, device control), same as AllowBind introduced with this PR. IMO graceful degradation shouldn't be the case for security.

Present features are implemented with 'assembly' bpf_insn and depend only on kernel. Favoring dlopen approach means that both firewall and devices can not be migrated to human friendly C code since this is security relaxation: not having libbpf.so on target host would be enough to opt out.

I would love to replace BPF firewall with C code and add ports support, but what's the point if it can be bypassed.

Ultimately which way to go is a question for the core maintainers. Your PR is fine at the moment and it can be changed later with regards to this aspect.

My take is that folks who want to build minimal images, really mean minimal, no matter what the cost - and to them, all of these features are optional. As long as everything is nicely documented, distro builders are able to make their choices. I expect the vast majority of distros to ship with all of these, exceptions being things like Alpine and minimal builds of Yocto/Buildroot.

And as long as the failure mode is appropriate, I do not see a security issue. ie: if the options are set, but cannot be satisfied, there should be an error. For example, if you don't have libcrypsetup available and your unit sets some verity options, it will fail to start when dlopen fails.

Looks nice.

cc @iaguis @mauriciovasquezbernal
#18145 and #18385 should be unblocked.

hmm, i'll post a follow-up PR addressing a bunch of the items above, but see above, some things regarding errno are not clear to me...

CentOS CI uses the master branch of the libpf repo, so it's usually a newer version than the latest stable release.

@mrc0mmand could it be that the "dlopen" trick prevents the CI (where as far as I know ldd is used to figure out what should be installed on testbeds) from installing libbpf and the other dependencies loaded with dlopen?

CentOS CI uses the master branch of the libpf repo, so it's usually a newer version than the latest stable release.

@mrc0mmand could it be that the "dlopen" trick prevents the CI (where as far as I know ldd is used to figure out what should be installed on testbeds) from installing libbpf and the other dependencies loaded with dlopen?

It does, but we have a "workaround" for this in test-functions:

@mrc0mmand thanks! Now I'm puzzled as to why the CI can't catch libbpf/libbpf#391

$ sudo ./build/test-bpf-lsm
Found cgroup2 on /sys/fs/cgroup/unified, unified hierarchy for systemd controller
libbpf: elf: skipping unrecognized data section(7) .eh_frame
...
libbpf: elf: skipping unrecognized data section(8) .eh_frame
libbpf: elf: skipping relo section(14) .rel.eh_frame for section(8) .eh_frame
btf.c:2754:23: runtime error: member access within misaligned address 0x0000009ea865 for type 'const struct btf_ext_header', which requires 4 byte alignment
0x0000009ea865: note: pointer points here
 6f 63 6b 00 9f eb 01  00 20 00 00 00 00 00 00  00 24 00 00 00 24 00 00  00 d4 05 00 00 f8 05 00  00
             ^
    #0 0x7fe0a6a55fae in btf_ext_parse_hdr /home/vagrant/libbpf/src/btf.c:2754
    #1 0x7fe0a6a5564d in btf_ext__new /home/vagrant/libbpf/src/btf.c:2798
    #2 0x7fe0a6af5301 in bpf_object__init_btf /home/vagrant/libbpf/src/libbpf.c:2636
    #3 0x7fe0a6ae6d53 in bpf_object__elf_collect /home/vagrant/libbpf/src/libbpf.c:3146
    #4 0x7fe0a6a98749 in __bpf_object__open /home/vagrant/libbpf/src/libbpf.c:6590
    #5 0x7fe0a6a989b1 in bpf_object__open_mem /home/vagrant/libbpf/src/libbpf.c:6655
    #6 0x7fe0a6adb86e in bpf_object__open_skeleton /home/vagrant/libbpf/src/libbpf.c:11052
    #7 0x81b87b in socket_bind_bpf__open_opts src/core/bpf/socket_bind/socket-bind.skel.h:54
    #8 0x81b937 in socket_bind_bpf__open src/core/bpf/socket_bind/socket-bind.skel.h:68
    #9 0x81dd9b in prepare_socket_bind_bpf ../src/core/bpf-socket-bind.c:79
    #10 0x8206a4 in bpf_socket_bind_supported ../src/core/bpf-socket-bind.c:138
    #11 0x67db63 in cg_bpf_mask_supported ../src/core/cgroup.c:3238
    #12 0x681142 in manager_setup_cgroup ../src/core/cgroup.c:3395
    #13 0x47c985 in manager_new ../src/core/manager.c:916
    #14 0x41f848 in main ../src/test/test-bpf-lsm.c:96
    #15 0x7fe0aaf9e1e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)
    #16 0x41d76d in _start (/home/vagrant/systemd/build/test-bpf-lsm+0x41d76d)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior btf.c:2754:23 in

Hm, that's interesting, the output on my side looks a bit different, I wonder what's going on:

# /systemd-meson-build/test-bpf-lsm |& grep libbpf
libbpf: prog 'sd_bind4': failed to attach to cgroup: Bad file descriptor
libbpf: prog 'sd_restrictif_i': failed to attach to cgroup: Bad file descriptor

# grep libbpf strace.out 
openat(AT_FDCWD, "/systemd-meson-build/src/shared/libbpf.so.0", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/lib/libbpf.so.0", O_RDONLY|O_CLOEXEC) = 3
write(2, "libbpf: prog 'sd_bind4': failed "..., 73libbpf: prog 'sd_bind4': failed to attach to cgroup: Bad file descriptor
write(2, "libbpf: prog 'sd_restrictif_i': "..., 80libbpf: prog 'sd_restrictif_i': failed to attach to cgroup: Bad file descriptor

Ah, I see, it's because the libbpf is not compiled with ASan/UBSan in CIs. After rebuilding libbpf with CFLAGS=-fsanitize=address,undefined, the issue pops up:

# /systemd-meson-build/test-bpf-lsm  
Found cgroup2 on /sys/fs/cgroup/, full unified hierarchy
btf.c:2754:21: runtime error: member access within misaligned address 0x55b6227be37f for type 'const struct btf_ext_header', which requires 4 byte alignment
0x55b6227be37f: note: pointer points here
 6e 73 65 00 9f  eb 01 00 20 00 00 00 00  00 00 00 14 00 00 00 14  00 00 00 ac 01 00 00 c0  01 00 00
             ^ 
    #0 0x7f93c54a2af2 in btf_ext_parse_hdr (/usr/lib/libbpf.so.0+0x1c2af2)
    #1 0x7f93c54a3059 in btf_ext__new (/usr/lib/libbpf.so.0+0x1c3059)
    #2 0x7f93c54d44f6 in bpf_object__init_btf (/usr/lib/libbpf.so.0+0x1f44f6)
    #3 0x7f93c54d9748 in bpf_object__elf_collect (/usr/lib/libbpf.so.0+0x1f9748)
    #4 0x7f93c550957d in __bpf_object__open (/usr/lib/libbpf.so.0+0x22957d)
    #5 0x7f93c5509c72 in bpf_object__open_mem (/usr/lib/libbpf.so.0+0x229c72)
    #6 0x7f93c5537d95 in bpf_object__open_skeleton (/usr/lib/libbpf.so.0+0x257d95)
    #7 0x55b62242256f in restrict_fs_bpf__open_opts /systemd-meson-build/src/core/bpf/restrict_fs/restrict-fs-skel.h:51:15
    #8 0x55b622422418 in restrict_fs_bpf__open /systemd-meson-build/src/core/bpf/restrict_fs/restrict-fs-skel.h:65:9
    #9 0x55b62241d686 in prepare_restrict_fs_bpf /systemd-meson-build/../build/src/core/bpf-lsm.c:61:15
    #10 0x55b62241ccd0 in lsm_bpf_supported /systemd-meson-build/../build/src/core/bpf-lsm.c:170:13
    #11 0x55b62241a8cd in main /systemd-meson-build/../build/src/test/test-bpf-lsm.c:84:13
    #12 0x7f93ca4b7b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
    #13 0x55b62241a5cd in _start (/systemd-meson-build/test-bpf-lsm+0x34e5cd)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior btf.c:2754:21 in 

It does, but we have a "workaround" for this in test-functions:

Thinking about it a bit more, I'm not sure workarounds of this type have ever been applied to various tools building images using ldd (like dracut where the testsuite originally came from as far as I can remember), which probably means that sometimes all those features along with their dependencies just (unexpectedly) keep sitting on hosts where those images are built. Hopefully it doesn't happen very often :-)