[PATCH bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines

[PATCH bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines

[Stanislav Fomichev]

With arch_prepare_bpf_trampoline removed on x86:

 #98/1    lsm_cgroup/functional:SKIP
 #98      lsm_cgroup:SKIP
 Summary: 1/0 PASSED, 1 SKIPPED, 0 FAILED

Fixes: dca85aac8895 ("selftests/bpf: lsm_cgroup functional test")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
index d40810a742fa..c542d7e80a5b 100644
--- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
+++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
@@ -9,6 +9,10 @@
 #include "cgroup_helpers.h"
 #include "network_helpers.h"
 
+#ifndef ENOTSUPP
+#define ENOTSUPP 524
+#endif
+
 static struct btf *btf;
 
 static __u32 query_prog_cnt(int cgroup_fd, const char *attach_func)
@@ -100,6 +104,10 @@ static void test_lsm_cgroup_functional(void)
 	ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count");
 	ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count");
 	err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0);
+	if (err == -ENOTSUPP) {
+		test__skip();
+		goto close_cgroup;
+	}
 	if (!ASSERT_OK(err, "attach alloc_prog_fd"))
 		goto detach_cgroup;
 	ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 1, "prog count");
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines

[Hao Luo]

Hi Stan,

On Thu, Jun 30, 2022 at 3:42 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> With arch_prepare_bpf_trampoline removed on x86:
>
>  #98/1    lsm_cgroup/functional:SKIP
>  #98      lsm_cgroup:SKIP
>  Summary: 1/0 PASSED, 1 SKIPPED, 0 FAILED
>
> Fixes: dca85aac8895 ("selftests/bpf: lsm_cgroup functional test")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> index d40810a742fa..c542d7e80a5b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> @@ -9,6 +9,10 @@
>  #include "cgroup_helpers.h"
>  #include "network_helpers.h"
>
> +#ifndef ENOTSUPP
> +#define ENOTSUPP 524
> +#endif
> +
>  static struct btf *btf;
>
>  static __u32 query_prog_cnt(int cgroup_fd, const char *attach_func)
> @@ -100,6 +104,10 @@ static void test_lsm_cgroup_functional(void)
>         ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count");
>         ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count");
>         err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0);
> +       if (err == -ENOTSUPP) {
> +               test__skip();
> +               goto close_cgroup;
> +       }

It seems ENOTSUPP is only used in the kernel. I wonder whether we
should let libbpf map ENOTSUPP to ENOTSUP, which is the errno used in
userspace and has been used in libbpf.

Maybe the right thing is having bpf syscall return EOPNOTSUPP.

But, anyway, this fix looks good to me.

Acked-by: Hao Luo <haoluo@google.com>



>         if (!ASSERT_OK(err, "attach alloc_prog_fd"))
>                 goto detach_cgroup;
>         ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 1, "prog count");
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>

Re: [PATCH bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines

[Stanislav Fomichev]

On Thu, Jun 30, 2022 at 4:48 PM Hao Luo <haoluo@google.com> wrote:
>
> Hi Stan,
>
> On Thu, Jun 30, 2022 at 3:42 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > With arch_prepare_bpf_trampoline removed on x86:
> >
> >  #98/1    lsm_cgroup/functional:SKIP
> >  #98      lsm_cgroup:SKIP
> >  Summary: 1/0 PASSED, 1 SKIPPED, 0 FAILED
> >
> > Fixes: dca85aac8895 ("selftests/bpf: lsm_cgroup functional test")
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> > index d40810a742fa..c542d7e80a5b 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> > @@ -9,6 +9,10 @@
> >  #include "cgroup_helpers.h"
> >  #include "network_helpers.h"
> >
> > +#ifndef ENOTSUPP
> > +#define ENOTSUPP 524
> > +#endif
> > +
> >  static struct btf *btf;
> >
> >  static __u32 query_prog_cnt(int cgroup_fd, const char *attach_func)
> > @@ -100,6 +104,10 @@ static void test_lsm_cgroup_functional(void)
> >         ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count");
> >         ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count");
> >         err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0);
> > +       if (err == -ENOTSUPP) {
> > +               test__skip();
> > +               goto close_cgroup;
> > +       }
>
> It seems ENOTSUPP is only used in the kernel. I wonder whether we
> should let libbpf map ENOTSUPP to ENOTSUP, which is the errno used in
> userspace and has been used in libbpf.

Yeah, this comes up occasionally, I don't think we've agreed on some
kind of general policy about what to do with these :-(
Thanks for the review!

> Maybe the right thing is having bpf syscall return EOPNOTSUPP.
>
> But, anyway, this fix looks good to me.
>
> Acked-by: Hao Luo <haoluo@google.com>
>
>
>
> >         if (!ASSERT_OK(err, "attach alloc_prog_fd"))
> >                 goto detach_cgroup;
> >         ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 1, "prog count");
> > --
> > 2.37.0.rc0.161.g10f37bed90-goog
> >

Re: [PATCH bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Thu, 30 Jun 2022 15:42:03 -0700 you wrote:
> With arch_prepare_bpf_trampoline removed on x86:
> 
>  #98/1    lsm_cgroup/functional:SKIP
>  #98      lsm_cgroup:SKIP
>  Summary: 1/0 PASSED, 1 SKIPPED, 0 FAILED
> 
> Fixes: dca85aac8895 ("selftests/bpf: lsm_cgroup functional test")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines
    https://git.kernel.org/bpf/bpf-next/c/b0d93b44641a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines

[Daniel Borkmann]

On 7/1/22 2:16 AM, Stanislav Fomichev wrote:
> On Thu, Jun 30, 2022 at 4:48 PM Hao Luo <haoluo@google.com> wrote:
>> On Thu, Jun 30, 2022 at 3:42 PM Stanislav Fomichev <sdf@google.com> wrote:
[...]
>>> With arch_prepare_bpf_trampoline removed on x86:
>>>
>>>   #98/1    lsm_cgroup/functional:SKIP
>>>   #98      lsm_cgroup:SKIP
>>>   Summary: 1/0 PASSED, 1 SKIPPED, 0 FAILED
>>>
>>> Fixes: dca85aac8895 ("selftests/bpf: lsm_cgroup functional test")
>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>> ---
>>>   tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
>>> index d40810a742fa..c542d7e80a5b 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
>>> @@ -9,6 +9,10 @@
>>>   #include "cgroup_helpers.h"
>>>   #include "network_helpers.h"
>>>
>>> +#ifndef ENOTSUPP
>>> +#define ENOTSUPP 524
>>> +#endif
>>> +
>>>   static struct btf *btf;
>>>
>>>   static __u32 query_prog_cnt(int cgroup_fd, const char *attach_func)
>>> @@ -100,6 +104,10 @@ static void test_lsm_cgroup_functional(void)
>>>          ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count");
>>>          ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count");
>>>          err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0);
>>> +       if (err == -ENOTSUPP) {
>>> +               test__skip();
>>> +               goto close_cgroup;
>>> +       }
>>
>> It seems ENOTSUPP is only used in the kernel. I wonder whether we
>> should let libbpf map ENOTSUPP to ENOTSUP, which is the errno used in
>> userspace and has been used in libbpf.
> 
> Yeah, this comes up occasionally, I don't think we've agreed on some
> kind of general policy about what to do with these :-(
> Thanks for the review!

Consensus was that for existing code, the ship has sailed to change it given
applications could one way or another depend on this error code, but it should
be avoided for new APIs (e.g. [0]).

Thanks,
Daniel

   [0] https://lore.kernel.org/bpf/20211209182349.038ac2b8@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/

Re: [PATCH bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines

[Stanislav Fomichev]

On Fri, Jul 1, 2022 at 6:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/1/22 2:16 AM, Stanislav Fomichev wrote:
> > On Thu, Jun 30, 2022 at 4:48 PM Hao Luo <haoluo@google.com> wrote:
> >> On Thu, Jun 30, 2022 at 3:42 PM Stanislav Fomichev <sdf@google.com> wrote:
> [...]
> >>> With arch_prepare_bpf_trampoline removed on x86:
> >>>
> >>>   #98/1    lsm_cgroup/functional:SKIP
> >>>   #98      lsm_cgroup:SKIP
> >>>   Summary: 1/0 PASSED, 1 SKIPPED, 0 FAILED
> >>>
> >>> Fixes: dca85aac8895 ("selftests/bpf: lsm_cgroup functional test")
> >>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >>> ---
> >>>   tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c | 8 ++++++++
> >>>   1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> >>> index d40810a742fa..c542d7e80a5b 100644
> >>> --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> >>> +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> >>> @@ -9,6 +9,10 @@
> >>>   #include "cgroup_helpers.h"
> >>>   #include "network_helpers.h"
> >>>
> >>> +#ifndef ENOTSUPP
> >>> +#define ENOTSUPP 524
> >>> +#endif
> >>> +
> >>>   static struct btf *btf;
> >>>
> >>>   static __u32 query_prog_cnt(int cgroup_fd, const char *attach_func)
> >>> @@ -100,6 +104,10 @@ static void test_lsm_cgroup_functional(void)
> >>>          ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count");
> >>>          ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count");
> >>>          err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0);
> >>> +       if (err == -ENOTSUPP) {
> >>> +               test__skip();
> >>> +               goto close_cgroup;
> >>> +       }
> >>
> >> It seems ENOTSUPP is only used in the kernel. I wonder whether we
> >> should let libbpf map ENOTSUPP to ENOTSUP, which is the errno used in
> >> userspace and has been used in libbpf.
> >
> > Yeah, this comes up occasionally, I don't think we've agreed on some
> > kind of general policy about what to do with these :-(
> > Thanks for the review!
>
> Consensus was that for existing code, the ship has sailed to change it given
> applications could one way or another depend on this error code, but it should
> be avoided for new APIs (e.g. [0]).

Ah, great, thanks Daniel!

> Thanks,
> Daniel
>
>    [0] https://lore.kernel.org/bpf/20211209182349.038ac2b8@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/

Re: [PATCH bpf-next] selftests/bpf: skip lsm_cgroup when don't have trampolines

[Hao Luo]

On Fri, Jul 1, 2022 at 6:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/1/22 2:16 AM, Stanislav Fomichev wrote:
> > On Thu, Jun 30, 2022 at 4:48 PM Hao Luo <haoluo@google.com> wrote:
> >> On Thu, Jun 30, 2022 at 3:42 PM Stanislav Fomichev <sdf@google.com> wrote:
[...]
> >>
> >> It seems ENOTSUPP is only used in the kernel. I wonder whether we
> >> should let libbpf map ENOTSUPP to ENOTSUP, which is the errno used in
> >> userspace and has been used in libbpf.
> >
> > Yeah, this comes up occasionally, I don't think we've agreed on some
> > kind of general policy about what to do with these :-(
> > Thanks for the review!
>
> Consensus was that for existing code, the ship has sailed to change it given
> applications could one way or another depend on this error code, but it should
> be avoided for new APIs (e.g. [0]).
>
> Thanks,
> Daniel
>
>    [0] https://lore.kernel.org/bpf/20211209182349.038ac2b8@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/

Thanks for the reference!