[PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Sasha Levin]

From: Lorenz Bauer <lorenz.bauer@isovalent.com>

[ Upstream commit dfdd608c3b365f0fd49d7e13911ebcde06b9865b ]

Add a regression test that ensures that a VAR pointing at a
modifier which follows a PTR (or STRUCT or ARRAY) is resolved
correctly by the datasec validator.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Link: https://lore.kernel.org/r/20230306112138.155352-3-lmb@isovalent.com
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/btf.c | 28 ++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index de1b5b9eb93a8..d8d1292e73b53 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -879,6 +879,34 @@ static struct btf_raw_test raw_tests[] = {
 	.btf_load_err = true,
 	.err_str = "Invalid elem",
 },
+{
+	.descr = "var after datasec, ptr followed by modifier",
+	.raw_types = {
+		/* .bss section */				/* [1] */
+		BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 2),
+			sizeof(void*)+4),
+		BTF_VAR_SECINFO_ENC(4, 0, sizeof(void*)),
+		BTF_VAR_SECINFO_ENC(6, sizeof(void*), 4),
+		/* int */					/* [2] */
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),
+		/* int* */					/* [3] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 2),
+		BTF_VAR_ENC(NAME_TBD, 3, 0),			/* [4] */
+		/* const int */					/* [5] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_CONST, 0, 0), 2),
+		BTF_VAR_ENC(NAME_TBD, 5, 0),			/* [6] */
+		BTF_END_RAW,
+	},
+	.str_sec = "\0a\0b\0c\0",
+	.str_sec_size = sizeof("\0a\0b\0c\0"),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = ".bss",
+	.key_size = sizeof(int),
+	.value_size = sizeof(void*)+4,
+	.key_type_id = 0,
+	.value_type_id = 1,
+	.max_entries = 1,
+},
 /* Test member exceeds the size of struct.
  *
  * struct A {
-- 
2.39.2

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Lorenz Bauer]

On Mon, Mar 20, 2023 at 12:53 AM Sasha Levin <sashal@kernel.org> wrote:
>
> From: Lorenz Bauer <lorenz.bauer@isovalent.com>
>
> [ Upstream commit dfdd608c3b365f0fd49d7e13911ebcde06b9865b ]
>
> Add a regression test that ensures that a VAR pointing at a
> modifier which follows a PTR (or STRUCT or ARRAY) is resolved
> correctly by the datasec validator.
>
> Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
> Link: https://lore.kernel.org/r/20230306112138.155352-3-lmb@isovalent.com
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>

Hi Sasha,

Can you explain why this patch was selected? I'd prefer to not
backport the test, since it frequently leads to breakage when trying
to build selftests/bpf on stable kernels.

Thanks
Lorenz

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Greg KH]

On Mon, Mar 20, 2023 at 03:31:42PM +0000, Lorenz Bauer wrote:
> On Mon, Mar 20, 2023 at 12:53 AM Sasha Levin <sashal@kernel.org> wrote:
> >
> > From: Lorenz Bauer <lorenz.bauer@isovalent.com>
> >
> > [ Upstream commit dfdd608c3b365f0fd49d7e13911ebcde06b9865b ]
> >
> > Add a regression test that ensures that a VAR pointing at a
> > modifier which follows a PTR (or STRUCT or ARRAY) is resolved
> > correctly by the datasec validator.
> >
> > Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
> > Link: https://lore.kernel.org/r/20230306112138.155352-3-lmb@isovalent.com
> > Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> > Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> Hi Sasha,
> 
> Can you explain why this patch was selected? I'd prefer to not
> backport the test, since it frequently leads to breakage when trying
> to build selftests/bpf on stable kernels.

Why would it break?  Is that because the test is buggy, or the kernel is
buggy?

thanks,

greg k-h

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Lorenz Bauer]

On Mon, Mar 20, 2023 at 3:48 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Why would it break?  Is that because the test is buggy, or the kernel is
> buggy?

This test will be fine, but there have been several times when
selftests/bpf for stable kernel releases didn't actually compile due
to backported tests. This is because macros we're redefined, etc.
Unless those also get picked (seems like a sisyphean task) we'll keep
seeing broken selftests/bpf on stable.

Best
Lorenz

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Lorenz Bauer]

On Tue, Mar 28, 2023 at 11:18 AM Lorenz Bauer <lmb@isovalent.com> wrote:
>
> On Mon, Mar 20, 2023 at 3:48 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > Why would it break?  Is that because the test is buggy, or the kernel is
> > buggy?
>
> This test will be fine, but there have been several times when
> selftests/bpf for stable kernel releases didn't actually compile due
> to backported tests. This is because macros we're redefined, etc.
> Unless those also get picked (seems like a sisyphean task) we'll keep
> seeing broken selftests/bpf on stable.

Hi Greg, Sasha,

Following up on this since it seems to have fallen through the cracks.

Lorenz

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Greg KH]

On Tue, Apr 11, 2023 at 04:08:32PM +0100, Lorenz Bauer wrote:
> On Tue, Mar 28, 2023 at 11:18 AM Lorenz Bauer <lmb@isovalent.com> wrote:
> >
> > On Mon, Mar 20, 2023 at 3:48 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > Why would it break?  Is that because the test is buggy, or the kernel is
> > > buggy?
> >
> > This test will be fine, but there have been several times when
> > selftests/bpf for stable kernel releases didn't actually compile due
> > to backported tests. This is because macros we're redefined, etc.
> > Unless those also get picked (seems like a sisyphean task) we'll keep
> > seeing broken selftests/bpf on stable.
> 
> Hi Greg, Sasha,
> 
> Following up on this since it seems to have fallen through the cracks.

I didn't see anything to do here.

And selftests should NOT be broken on stable releases, if so, something
is wrong as no other subsystem has that happen.

confused,

greg k-h

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Lorenz Bauer]

On Tue, Apr 11, 2023 at 4:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> I didn't see anything to do here.
>
> And selftests should NOT be broken on stable releases, if so, something
> is wrong as no other subsystem has that happen.

Sorry for the long delay in replying, I update the kernels we use for
CI only infrequently. Here is an example of the build failure I'm
seeing, from kernel.org 5.10 LTS:

In file included from
/work/build/5.10.180/tools/testing/selftests/bpf/verifier/tests.h:59,
                 from test_verifier.c:355:
/work/build/5.10.180/tools/testing/selftests/bpf/verifier/ref_tracking.c:935:3:
error: 'struct bpf_test' has no member named 'fixup_map_ringbuf'; did
you mean 'fixup_map_in_map'?
  935 |  .fixup_map_ringbuf = { 11 },
      |   ^~~~~~~~~~~~~~~~~
      |   fixup_map_in_map

This is just doing make -C tools/testing/selftests/bpf after compiling a kernel.

Best
Lorenz

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Greg KH]

On Wed, May 24, 2023 at 12:03:43PM +0100, Lorenz Bauer wrote:
> On Tue, Apr 11, 2023 at 4:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > I didn't see anything to do here.
> >
> > And selftests should NOT be broken on stable releases, if so, something
> > is wrong as no other subsystem has that happen.
> 
> Sorry for the long delay in replying, I update the kernels we use for
> CI only infrequently. Here is an example of the build failure I'm
> seeing, from kernel.org 5.10 LTS:
> 
> In file included from
> /work/build/5.10.180/tools/testing/selftests/bpf/verifier/tests.h:59,
>                  from test_verifier.c:355:
> /work/build/5.10.180/tools/testing/selftests/bpf/verifier/ref_tracking.c:935:3:
> error: 'struct bpf_test' has no member named 'fixup_map_ringbuf'; did
> you mean 'fixup_map_in_map'?
>   935 |  .fixup_map_ringbuf = { 11 },
>       |   ^~~~~~~~~~~~~~~~~
>       |   fixup_map_in_map
> 
> This is just doing make -C tools/testing/selftests/bpf after compiling a kernel.

Great, any specific commits that fix this issue would be appreciated to
be pointed at so we can apply them.

thanks,

greg k-h

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Lorenz Bauer]

On Wed, May 24, 2023 at 5:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Great, any specific commits that fix this issue would be appreciated to
> be pointed at so we can apply them.

The problem was introduced by commit f4b8c0710ab6 ("selftests/bpf: Add
verifier test for release_reference()") in your tree. Seems like
fixup_map_ringbuf was introduced in upstream commit 4237e9f4a962
("selftests/bpf: Add verifier test for PTR_TO_MEM spill") but that
wasn't backported.

To restate my original question: how can we avoid breaking BPF
selftests? From personal experience this happens somewhat regularly.

Best
Lorenz

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Greg KH]

On Wed, May 24, 2023 at 06:04:43PM +0100, Lorenz Bauer wrote:
> On Wed, May 24, 2023 at 5:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > Great, any specific commits that fix this issue would be appreciated to
> > be pointed at so we can apply them.
> 
> The problem was introduced by commit f4b8c0710ab6 ("selftests/bpf: Add
> verifier test for release_reference()") in your tree. Seems like
> fixup_map_ringbuf was introduced in upstream commit 4237e9f4a962
> ("selftests/bpf: Add verifier test for PTR_TO_MEM spill") but that
> wasn't backported.

So what tree(s) does this need to be backported to?  I'm confused, this
is a 6.2 email thread which is long end-of-life.

> To restate my original question: how can we avoid breaking BPF
> selftests? From personal experience this happens somewhat regularly.

It can be avoided by people testing and letting me know when things
break :)

thanks,

greg k-h

Re: [PATCH AUTOSEL 6.2 08/30] selftests/bpf: check that modifier resolves after pointer

[Lorenz Bauer]

On Fri, May 26, 2023 at 6:43 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> So what tree(s) does this need to be backported to?  I'm confused, this
> is a 6.2 email thread which is long end-of-life.

That would be 5.10 from what I can tell. Other LTS kernels have
working tools/testing/selftests/bpf.

I replied here because you asked for examples :)

> It can be avoided by people testing and letting me know when things
> break :)

Fair enough. Earlier you said:

> And selftests should NOT be broken on stable releases, if so, something
> is wrong as no other subsystem has that happen.

Why is bpf special in this regard then?

Best
Lorenz