| Message ID | f4l6fadtxnvttlb27heyl3r2bxettwwfu5vrazqykrshvrl3vm@ejw2ccatg3wi (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, 2024-01-18 at 19:58 +0800, Shung-Hsi Yu wrote: > Hi, > > Compilation of lsm_cgroup.c will fail if the vmlinux.h comes from a > kernel that does _not_ have CONFIG_PACKET=y. The reason is that the > definition of struct sockaddr_ll is not present in vmlinux.h and the > compiler will complain that is has an incomplete type. > > CLNG-BPF [test_maps] lsm_cgroup.bpf.o > progs/lsm_cgroup.c:105:21: error: variable has incomplete type 'struct sockaddr_ll' > 105 | struct sockaddr_ll sa = {}; > | ^ > progs/lsm_cgroup.c:105:9: note: forward declaration of 'struct sockaddr_ll' > 105 | struct sockaddr_ll sa = {}; > | ^ > 1 error generated. > > While including linux/if_packet.h somehow made the compilation works for > me, IIUC this isn't a proper solution because vmlinux.h and kernel > headers should not be used at the same time (and would lead to > redefinition error when the kernel is built with CONFIG_PACKET=y, e.g. > on BPF CI). > > What would be the suggested way to work around this? > > Thanks, > Shung-Hsi Hi Shung-Hsi, One option is to use CO-RE, e.g. as at the bottom of this email (not sure if people would agree with me). But that would not produce usable test anyways, as load would fail with unresolved CO-RE relocation. But what is your final goal? As far as I understand, selftests are supposed to be built and run using specific configuration, here is how config for x86 CI is prepared: ./scripts/kconfig/merge_config.sh \ ./tools/testing/selftests/bpf/config \ ./tools/testing/selftests/bpf/config.vm \ ./tools/testing/selftests/bpf/config.x86_64 (root is kernel source). I'm not sure if other configurations are supposed to be supported. Thanks, Eduard --- diff --git a/tools/testing/selftests/bpf/progs/lsm_cgroup.c b/tools/testing/selftests/bpf/progs/lsm_cgroup.c index 02c11d16b692..8381e5a202c8 100644 --- a/tools/testing/selftests/bpf/progs/lsm_cgroup.c +++ b/tools/testing/selftests/bpf/progs/lsm_cgroup.c @@ -2,6 +2,7 @@ #include "vmlinux.h" #include "bpf_tracing_net.h" +#include <bpf/bpf_core_read.h> #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> @@ -98,11 +99,15 @@ int BPF_PROG(socket_post_create2, struct socket *sock, int family, return real_create(sock, family, protocol); } +struct sockaddr_ll___local { + __be16 sll_protocol; +} __attribute__((preserve_access_index)); + static __always_inline int real_bind(struct socket *sock, struct sockaddr *address, int addrlen) { - struct sockaddr_ll sa = {}; + __u16 sll_protocol = 0; if (sock->sk->__sk_common.skc_family != AF_PACKET) return 1; @@ -110,8 +115,10 @@ static __always_inline int real_bind(struct socket *sock, if (sock->sk->sk_kern_sock) return 1; - bpf_probe_read_kernel(&sa, sizeof(sa), address); - if (sa.sll_protocol) + bpf_probe_read_kernel( + &sll_protocol, sizeof(sll_protocol), + (__u8*)address + bpf_core_field_offset(struct sockaddr_ll___local, sll_protocol)); + if (sll_protocol) return 0; /* EPERM */ /* Can access cgroup local storage. */
On Thu, 2024-01-18 at 17:58 +0200, Eduard Zingerman wrote: [...] > here is how config for x86 CI is prepared: > > ./scripts/kconfig/merge_config.sh \ > ./tools/testing/selftests/bpf/config \ > ./tools/testing/selftests/bpf/config.vm \ > ./tools/testing/selftests/bpf/config.x86_64 > (For whatever reason CONFIG_PACKET is defined in .../config.x86_64, maybe that should be moved to .../config?)
On 1/18/24 8:05 AM, Eduard Zingerman wrote: > On Thu, 2024-01-18 at 17:58 +0200, Eduard Zingerman wrote: > [...] >> here is how config for x86 CI is prepared: >> >> ./scripts/kconfig/merge_config.sh \ >> ./tools/testing/selftests/bpf/config \ >> ./tools/testing/selftests/bpf/config.vm \ >> ./tools/testing/selftests/bpf/config.x86_64 >> > (For whatever reason CONFIG_PACKET is defined in .../config.x86_64, > maybe that should be moved to .../config?) Sounds a good idea to me.
On Thu, Jan 18, 2024 at 05:58:20PM +0200, Eduard Zingerman wrote: > On Thu, 2024-01-18 at 19:58 +0800, Shung-Hsi Yu wrote: > > Compilation of lsm_cgroup.c will fail if the vmlinux.h comes from a > > kernel that does _not_ have CONFIG_PACKET=y. The reason is that the > > definition of struct sockaddr_ll is not present in vmlinux.h and the > > compiler will complain that is has an incomplete type. > > > > CLNG-BPF [test_maps] lsm_cgroup.bpf.o > > progs/lsm_cgroup.c:105:21: error: variable has incomplete type 'struct sockaddr_ll' > > 105 | struct sockaddr_ll sa = {}; > > | ^ > > progs/lsm_cgroup.c:105:9: note: forward declaration of 'struct sockaddr_ll' > > 105 | struct sockaddr_ll sa = {}; > > | ^ > > 1 error generated. > > > > [...] > > Hi Shung-Hsi, > > One option is to use CO-RE, e.g. as at the bottom of this email > (not sure if people would agree with me). > But that would not produce usable test anyways, > as load would fail with unresolved CO-RE relocation. > > But what is your final goal? Final goal would be have BPF selftests compiled and test against our own kernel, without having to come up with a specific kernel flavor that is used to build and run the selftest. For v5.14 and v5.19-based kernel it works: compilation is successful and I was able to run the verifier tests. (Did not try running the other tests though) > As far as I understand, selftests are supposed to be built and run > using specific configuration, here is how config for x86 CI is prepared: > > ./scripts/kconfig/merge_config.sh \ > ./tools/testing/selftests/bpf/config \ > ./tools/testing/selftests/bpf/config.vm \ > ./tools/testing/selftests/bpf/config.x86_64 > > (root is kernel source). > I'm not sure if other configurations are supposed to be supported. Would it make sense to have makefile target that builds/runs a smaller subset of general, config-agnostic selftests that tests the core feature (e.g. verifier + instruction set)? > [...]
On Fri, 2024-01-19 at 16:04 +0800, Shung-Hsi Yu wrote: [...] > Final goal would be have BPF selftests compiled and test against our own > kernel, without having to come up with a specific kernel flavor that is > used to build and run the selftest. For v5.14 and v5.19-based kernel it > works: compilation is successful and I was able to run the verifier > tests. (Did not try running the other tests though) You mean ./test_verifier binary, right? A lot of tests had been moved from ./test_verifier to ./test_progs since. > > As far as I understand, selftests are supposed to be built and run > > using specific configuration, here is how config for x86 CI is prepared: > > > > ./scripts/kconfig/merge_config.sh \ > > ./tools/testing/selftests/bpf/config \ > > ./tools/testing/selftests/bpf/config.vm \ > > ./tools/testing/selftests/bpf/config.x86_64 > > > > (root is kernel source). > > I'm not sure if other configurations are supposed to be supported. > > Would it make sense to have makefile target that builds/runs a smaller > subset of general, config-agnostic selftests that tests the core feature > (e.g. verifier + instruction set)? In ideal world I'd say that ./test_progs should include/exclude tests conditioned on current configuration, but I don't know how much work would it be to adapt build system for this.
On Fri, Jan 19, 2024 at 4:23 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Fri, 2024-01-19 at 16:04 +0800, Shung-Hsi Yu wrote: > > [...] > > > Final goal would be have BPF selftests compiled and test against our own > > kernel, without having to come up with a specific kernel flavor that is > > used to build and run the selftest. For v5.14 and v5.19-based kernel it > > works: compilation is successful and I was able to run the verifier > > tests. (Did not try running the other tests though) > > You mean ./test_verifier binary, right? > A lot of tests had been moved from ./test_verifier to ./test_progs since. > > > > As far as I understand, selftests are supposed to be built and run > > > using specific configuration, here is how config for x86 CI is prepared: > > > > > > ./scripts/kconfig/merge_config.sh \ > > > ./tools/testing/selftests/bpf/config \ > > > ./tools/testing/selftests/bpf/config.vm \ > > > ./tools/testing/selftests/bpf/config.x86_64 > > > > > > (root is kernel source). > > > I'm not sure if other configurations are supposed to be supported. > > > > Would it make sense to have makefile target that builds/runs a smaller > > subset of general, config-agnostic selftests that tests the core feature > > (e.g. verifier + instruction set)? > > In ideal world I'd say that ./test_progs should include/exclude tests > conditioned on current configuration, but I don't know how much work > would it be to adapt build system for this. > I would also suggest skipping building the specific bpf test code when a specific CONFIG is removed, sometimes I only want to test some bpf selftests code I am interested in :)
On Fri, Jan 19, 2024 at 7:00 AM Vincent Li <vincent.mc.li@gmail.com> wrote: > > On Fri, Jan 19, 2024 at 4:23 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Fri, 2024-01-19 at 16:04 +0800, Shung-Hsi Yu wrote: > > > > [...] > > > > > Final goal would be have BPF selftests compiled and test against our own > > > kernel, without having to come up with a specific kernel flavor that is > > > used to build and run the selftest. For v5.14 and v5.19-based kernel it > > > works: compilation is successful and I was able to run the verifier > > > tests. (Did not try running the other tests though) > > > > You mean ./test_verifier binary, right? > > A lot of tests had been moved from ./test_verifier to ./test_progs since. > > > > > > As far as I understand, selftests are supposed to be built and run > > > > using specific configuration, here is how config for x86 CI is prepared: > > > > > > > > ./scripts/kconfig/merge_config.sh \ > > > > ./tools/testing/selftests/bpf/config \ > > > > ./tools/testing/selftests/bpf/config.vm \ > > > > ./tools/testing/selftests/bpf/config.x86_64 > > > > > > > > (root is kernel source). > > > > I'm not sure if other configurations are supposed to be supported. > > > > > > Would it make sense to have makefile target that builds/runs a smaller > > > subset of general, config-agnostic selftests that tests the core feature > > > (e.g. verifier + instruction set)? > > > > In ideal world I'd say that ./test_progs should include/exclude tests > > conditioned on current configuration, but I don't know how much work > > would it be to adapt build system for this. > > > > I would also suggest skipping building the specific bpf test code when > a specific CONFIG is removed, sometimes > I only want to test some bpf selftests code I am interested in :) I don't think we should be complicating bpf selftests to test configurations with reduced kconfig. bpf/config.* is what we target in bpf CI and we expect developers do the same amount of testing before they send patches.
On Fri, Jan 19, 2024 at 2:26 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Jan 19, 2024 at 7:00 AM Vincent Li <vincent.mc.li@gmail.com> wrote: > > > > On Fri, Jan 19, 2024 at 4:23 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > On Fri, 2024-01-19 at 16:04 +0800, Shung-Hsi Yu wrote: > > > > > > [...] > > > > > > > Final goal would be have BPF selftests compiled and test against our own > > > > kernel, without having to come up with a specific kernel flavor that is > > > > used to build and run the selftest. For v5.14 and v5.19-based kernel it > > > > works: compilation is successful and I was able to run the verifier > > > > tests. (Did not try running the other tests though) > > > > > > You mean ./test_verifier binary, right? > > > A lot of tests had been moved from ./test_verifier to ./test_progs since. > > > > > > > > As far as I understand, selftests are supposed to be built and run > > > > > using specific configuration, here is how config for x86 CI is prepared: > > > > > > > > > > ./scripts/kconfig/merge_config.sh \ > > > > > ./tools/testing/selftests/bpf/config \ > > > > > ./tools/testing/selftests/bpf/config.vm \ > > > > > ./tools/testing/selftests/bpf/config.x86_64 > > > > > > > > > > (root is kernel source). > > > > > I'm not sure if other configurations are supposed to be supported. > > > > > > > > Would it make sense to have makefile target that builds/runs a smaller > > > > subset of general, config-agnostic selftests that tests the core feature > > > > (e.g. verifier + instruction set)? > > > > > > In ideal world I'd say that ./test_progs should include/exclude tests > > > conditioned on current configuration, but I don't know how much work > > > would it be to adapt build system for this. > > > > > > > I would also suggest skipping building the specific bpf test code when > > a specific CONFIG is removed, sometimes > > I only want to test some bpf selftests code I am interested in :) > > I don't think we should be complicating bpf selftests to test > configurations with reduced kconfig. > bpf/config.* is what we target in bpf CI and we expect > developers do the same amount of testing before they send patches. Totally understand that from the kernel bpf developer perspective. I am a bpf user learning how to write a bpf program from selftests, but I guess there is another way to learn, selftests is not for teaching bpf users, no need to complicate.
On Fri, Jan 19, 2024 at 3:13 PM Vincent Li <vincent.mc.li@gmail.com> wrote: > > On Fri, Jan 19, 2024 at 2:26 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Jan 19, 2024 at 7:00 AM Vincent Li <vincent.mc.li@gmail.com> wrote: > > > > > > On Fri, Jan 19, 2024 at 4:23 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > > > On Fri, 2024-01-19 at 16:04 +0800, Shung-Hsi Yu wrote: > > > > > > > > [...] > > > > > > > > > Final goal would be have BPF selftests compiled and test against our own > > > > > kernel, without having to come up with a specific kernel flavor that is > > > > > used to build and run the selftest. For v5.14 and v5.19-based kernel it > > > > > works: compilation is successful and I was able to run the verifier > > > > > tests. (Did not try running the other tests though) > > > > > > > > You mean ./test_verifier binary, right? > > > > A lot of tests had been moved from ./test_verifier to ./test_progs since. > > > > > > > > > > As far as I understand, selftests are supposed to be built and run > > > > > > using specific configuration, here is how config for x86 CI is prepared: > > > > > > > > > > > > ./scripts/kconfig/merge_config.sh \ > > > > > > ./tools/testing/selftests/bpf/config \ > > > > > > ./tools/testing/selftests/bpf/config.vm \ > > > > > > ./tools/testing/selftests/bpf/config.x86_64 > > > > > > > > > > > > (root is kernel source). > > > > > > I'm not sure if other configurations are supposed to be supported. > > > > > > > > > > Would it make sense to have makefile target that builds/runs a smaller > > > > > subset of general, config-agnostic selftests that tests the core feature > > > > > (e.g. verifier + instruction set)? > > > > > > > > In ideal world I'd say that ./test_progs should include/exclude tests > > > > conditioned on current configuration, but I don't know how much work > > > > would it be to adapt build system for this. > > > > > > > > > > I would also suggest skipping building the specific bpf test code when > > > a specific CONFIG is removed, sometimes > > > I only want to test some bpf selftests code I am interested in :) > > > > I don't think we should be complicating bpf selftests to test > > configurations with reduced kconfig. > > bpf/config.* is what we target in bpf CI and we expect > > developers do the same amount of testing before they send patches. > > Totally understand that from the kernel bpf developer perspective. I > am a bpf user learning how to write a bpf program from selftests, but > I guess there is another way to learn, selftests is not for teaching > bpf users, no need to complicate. Try libbpf-bootstrap ([0]) as a simple setup to play with new BPF features. minimal or bootstrap examples are usually good starting points. [0] https://github.com/libbpf/libbpf-bootstrap
On Fri, Jan 19, 2024 at 3:35 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Jan 19, 2024 at 3:13 PM Vincent Li <vincent.mc.li@gmail.com> wrote: > > > > On Fri, Jan 19, 2024 at 2:26 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Fri, Jan 19, 2024 at 7:00 AM Vincent Li <vincent.mc.li@gmail.com> wrote: > > > > > > > > On Fri, Jan 19, 2024 at 4:23 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > > > > > On Fri, 2024-01-19 at 16:04 +0800, Shung-Hsi Yu wrote: > > > > > > > > > > [...] > > > > > > > > > > > Final goal would be have BPF selftests compiled and test against our own > > > > > > kernel, without having to come up with a specific kernel flavor that is > > > > > > used to build and run the selftest. For v5.14 and v5.19-based kernel it > > > > > > works: compilation is successful and I was able to run the verifier > > > > > > tests. (Did not try running the other tests though) > > > > > > > > > > You mean ./test_verifier binary, right? > > > > > A lot of tests had been moved from ./test_verifier to ./test_progs since. > > > > > > > > > > > > As far as I understand, selftests are supposed to be built and run > > > > > > > using specific configuration, here is how config for x86 CI is prepared: > > > > > > > > > > > > > > ./scripts/kconfig/merge_config.sh \ > > > > > > > ./tools/testing/selftests/bpf/config \ > > > > > > > ./tools/testing/selftests/bpf/config.vm \ > > > > > > > ./tools/testing/selftests/bpf/config.x86_64 > > > > > > > > > > > > > > (root is kernel source). > > > > > > > I'm not sure if other configurations are supposed to be supported. > > > > > > > > > > > > Would it make sense to have makefile target that builds/runs a smaller > > > > > > subset of general, config-agnostic selftests that tests the core feature > > > > > > (e.g. verifier + instruction set)? > > > > > > > > > > In ideal world I'd say that ./test_progs should include/exclude tests > > > > > conditioned on current configuration, but I don't know how much work > > > > > would it be to adapt build system for this. > > > > > > > > > > > > > I would also suggest skipping building the specific bpf test code when > > > > a specific CONFIG is removed, sometimes > > > > I only want to test some bpf selftests code I am interested in :) > > > > > > I don't think we should be complicating bpf selftests to test > > > configurations with reduced kconfig. > > > bpf/config.* is what we target in bpf CI and we expect > > > developers do the same amount of testing before they send patches. > > > > Totally understand that from the kernel bpf developer perspective. I > > am a bpf user learning how to write a bpf program from selftests, but > > I guess there is another way to learn, selftests is not for teaching > > bpf users, no need to complicate. > > Try libbpf-bootstrap ([0]) as a simple setup to play with new BPF > features. minimal or bootstrap examples are usually good starting > points. > > [0] https://github.com/libbpf/libbpf-bootstrap Thanks! I am aware of libbpf-bootstrap, I am on an old centos 8 distro which often miss linux headers that some selftests happens to require, especially the ones that are not using vmlinux.h, when a bpf kernel developer submit patches and selftests that I am interested in, I want to run that selftests and learn the new feature, and then probably port the new useful selftests code to a real use case bpf program. I often run into other selftests compiling errors when I want to selftest the new feature I am interested in. Anyway, it is my build environment problem, not selftests.
diff --git a/tools/testing/selftests/bpf/progs/lsm_cgroup.c b/tools/testing/selftests/bpf/progs/lsm_cgroup.c index 02c11d16b692..5394ec7ae1d8 100644 --- a/tools/testing/selftests/bpf/progs/lsm_cgroup.c +++ b/tools/testing/selftests/bpf/progs/lsm_cgroup.c @@ -2,6 +2,7 @@ #include "vmlinux.h" #include "bpf_tracing_net.h" +#include <linux/if_packet.h> #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h>
Hi, Compilation of lsm_cgroup.c will fail if the vmlinux.h comes from a kernel that does _not_ have CONFIG_PACKET=y. The reason is that the definition of struct sockaddr_ll is not present in vmlinux.h and the compiler will complain that is has an incomplete type. CLNG-BPF [test_maps] lsm_cgroup.bpf.o progs/lsm_cgroup.c:105:21: error: variable has incomplete type 'struct sockaddr_ll' 105 | struct sockaddr_ll sa = {}; | ^ progs/lsm_cgroup.c:105:9: note: forward declaration of 'struct sockaddr_ll' 105 | struct sockaddr_ll sa = {}; | ^ 1 error generated. While including linux/if_packet.h somehow made the compilation works for me, IIUC this isn't a proper solution because vmlinux.h and kernel headers should not be used at the same time (and would lead to redefinition error when the kernel is built with CONFIG_PACKET=y, e.g. on BPF CI). What would be the suggested way to work around this? Thanks, Shung-Hsi --- tools/testing/selftests/bpf/progs/lsm_cgroup.c | 1 + 1 file changed, 1 insertion(+)