lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Shung-Hsi Yu]

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(+)

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>

Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Eduard Zingerman]

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. */

Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Eduard Zingerman]

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?)

Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Yonghong Song]

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.

Re: Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Shung-Hsi Yu]

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)?

> [...]

Re: Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Eduard Zingerman]

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.

Re: Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Vincent Li]

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 :)

Re: Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Alexei Starovoitov]

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.

Re: Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Vincent Li]

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.

Re: Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Andrii Nakryiko]

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

Re: Re: lsm_cgroup.c selftest fails to compile when CONFIG_PACKET!=y

[Vincent Li]

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.