[bpf-next v3 00/12] Test the 32bit narrow reads

[bpf-next v3 00/12] Test the 32bit narrow reads

[Krzesimir Nowak]

These patches try to test the fix made in commit e2f7fc0ac695 ("bpf:
fix undefined behavior in narrow load handling"). The problem existed
in the generated BPF bytecode that was doing a 32bit narrow read of a
64bit field, so to test it the code would need to be executed.
Currently the only such field exists in BPF_PROG_TYPE_PERF_EVENT,
which was not supported by bpf_prog_test_run().

I'm sending these patches to bpf-next now as they introduce a new
feature. But maybe some of those patches could go to the bpf branch?


There is a bit of yak shaving to do for the test to be run:

1. Print why the program could not be run (patch 1).

2. Some fixes for errno clobbering (patches 2 and 3).

3. Using bpf_prog_test_run_xattr, so I can pass ctx_in stuff too
   (patch 4).

4. Adding ctx stuff and data out size override to struct bpf_test, and
   use them for the perf event tests (patches 5 and 6).

5. Some tools headers syncing (patches 7 and 8).

6. Split out some useful functions for bpf_prog_test_run
   implementation out of the net/bpf/test_run.c (patch 9)

7. Implement bpf_prog_test_run for perf event programs and test it
   (patches 10 and 11).


The last point is where I'm least sure how things should be done
properly:

1. There is a bunch of stuff to prepare for the
   bpf_perf_prog_read_value to work, and that stuff is very hacky. I
   would welcome some hints about how to set up the perf_event and
   perf_sample_data structs in a way that is a bit more future-proof
   than just setting some fields in a specific way, so some other code
   won't use some other fields (like setting event.oncpu to -1 to
   avoid event.pmu to be used for reading the value of the event).

2. The tests try to see if the test run for perf event sets up the
   context properly, so they verify the struct pt_regs contents. They
   way it is currently written Works For Me, but surely it won't work
   on other arch. So what would be the way forward? Just put the test
   case inside #ifdef __x86_64__?

3. Another thing in tests - I'm trying to make sure that the
   bpf_perf_prog_read_value helper works as it seems to be the only
   bpf perf helper that takes the ctx as a parameter. Is that enough
   or I should test other helpers too?


About the test itself - I'm not sure if it will work on a big endian
machine. I think it should, but I don't have anything handy here to
verify it.

Krzesimir Nowak (12):
  selftests/bpf: Print a message when tester could not run a program
  selftests/bpf: Avoid a clobbering of errno
  selftests/bpf: Avoid another case of errno clobbering
  selftests/bpf: Use bpf_prog_test_run_xattr
  selftests/bpf: Allow passing more information to BPF prog test run
  selftests/bpf: Make sure that preexisting tests for perf event work
  tools headers: Adopt compiletime_assert from kernel sources
  tools headers: Sync struct bpf_perf_event_data
  bpf: Split out some helper functions
  bpf: Implement bpf_prog_test_run for perf event programs
  selftests/bpf: Add tests for bpf_prog_test_run for perf events progs
  selftests/bpf: Test correctness of narrow 32bit read on 64bit field

 include/linux/bpf.h                           |  28 ++
 kernel/bpf/Makefile                           |   1 +
 kernel/bpf/test_run.c                         | 212 ++++++++++++++
 kernel/trace/bpf_trace.c                      |  60 ++++
 net/bpf/test_run.c                            | 263 +++++-------------
 tools/include/linux/compiler.h                |  28 ++
 tools/include/uapi/linux/bpf_perf_event.h     |   1 +
 tools/testing/selftests/bpf/test_verifier.c   | 197 ++++++++++++-
 .../selftests/bpf/verifier/perf_event_run.c   |  96 +++++++
 .../bpf/verifier/perf_event_sample_period.c   |   4 +
 .../testing/selftests/bpf/verifier/var_off.c  |  21 ++
 11 files changed, 700 insertions(+), 211 deletions(-)
 create mode 100644 kernel/bpf/test_run.c
 create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c

-- 
2.20.1

[bpf-next v3 01/12] selftests/bpf: Print a message when tester could not run a program

[Krzesimir Nowak]

This prints a message when the error is about program type being not
supported by the test runner or because of permissions problem. This
is to see if the program we expected to run was actually executed.

The messages are open-coded because strerror(ENOTSUPP) returns
"Unknown error 524".

Changes since v2:
- Also print "FAIL" on an unexpected bpf_prog_test_run error, so there
  is a corresponding "FAIL" message for each failed test.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/testing/selftests/bpf/test_verifier.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c5514daf8865..b8d065623ead 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -831,11 +831,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
 				tmp, &size_tmp, &retval, NULL);
 	if (unpriv)
 		set_admin(false);
-	if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
-		printf("Unexpected bpf_prog_test_run error ");
-		return err;
+	if (err) {
+		switch (errno) {
+		case 524/*ENOTSUPP*/:
+			printf("Did not run the program (not supported) ");
+			return 0;
+		case EPERM:
+			printf("Did not run the program (no permission) ");
+			return 0;
+		default:
+			printf("FAIL: Unexpected bpf_prog_test_run error (%s) ", strerror(saved_errno));
+			return err;
+		}
 	}
-	if (!err && retval != expected_val &&
+	if (retval != expected_val &&
 	    expected_val != POINTER_VALUE) {
 		printf("FAIL retval %d != %d ", retval, expected_val);
 		return 1;
-- 
2.20.1

Re: [bpf-next v3 01/12] selftests/bpf: Print a message when tester could not run a program

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> This prints a message when the error is about program type being not
> supported by the test runner or because of permissions problem. This
> is to see if the program we expected to run was actually executed.
>
> The messages are open-coded because strerror(ENOTSUPP) returns
> "Unknown error 524".
>
> Changes since v2:
> - Also print "FAIL" on an unexpected bpf_prog_test_run error, so there
>   is a corresponding "FAIL" message for each failed test.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
>  tools/testing/selftests/bpf/test_verifier.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c5514daf8865..b8d065623ead 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -831,11 +831,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
>                                 tmp, &size_tmp, &retval, NULL);
>         if (unpriv)
>                 set_admin(false);
> -       if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
> -               printf("Unexpected bpf_prog_test_run error ");
> -               return err;
> +       if (err) {
> +               switch (errno) {
> +               case 524/*ENOTSUPP*/:
> +                       printf("Did not run the program (not supported) ");
> +                       return 0;
> +               case EPERM:
> +                       printf("Did not run the program (no permission) ");

Let's add "SKIP: " prefix to these?

> +                       return 0;
> +               default:
> +                       printf("FAIL: Unexpected bpf_prog_test_run error (%s) ", strerror(saved_errno));
> +                       return err;
> +               }
>         }
> -       if (!err && retval != expected_val &&
> +       if (retval != expected_val &&
>             expected_val != POINTER_VALUE) {
>                 printf("FAIL retval %d != %d ", retval, expected_val);
>                 return 1;
> --
> 2.20.1
>

Re: [bpf-next v3 01/12] selftests/bpf: Print a message when tester could not run a program

[Krzesimir Nowak]

On Thu, Jul 11, 2019 at 1:45 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> >
> > This prints a message when the error is about program type being not
> > supported by the test runner or because of permissions problem. This
> > is to see if the program we expected to run was actually executed.
> >
> > The messages are open-coded because strerror(ENOTSUPP) returns
> > "Unknown error 524".
> >
> > Changes since v2:
> > - Also print "FAIL" on an unexpected bpf_prog_test_run error, so there
> >   is a corresponding "FAIL" message for each failed test.
> >
> > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > ---
> >  tools/testing/selftests/bpf/test_verifier.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index c5514daf8865..b8d065623ead 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -831,11 +831,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> >                                 tmp, &size_tmp, &retval, NULL);
> >         if (unpriv)
> >                 set_admin(false);
> > -       if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
> > -               printf("Unexpected bpf_prog_test_run error ");
> > -               return err;
> > +       if (err) {
> > +               switch (errno) {
> > +               case 524/*ENOTSUPP*/:
> > +                       printf("Did not run the program (not supported) ");
> > +                       return 0;
> > +               case EPERM:
> > +                       printf("Did not run the program (no permission) ");
>
> Let's add "SKIP: " prefix to these?

Not sure about it. The important part of the test (the program being
verified by the kernel's verifier) was still executed, so the test is
not really skipped.


>
> > +                       return 0;
> > +               default:
> > +                       printf("FAIL: Unexpected bpf_prog_test_run error (%s) ", strerror(saved_errno));
> > +                       return err;
> > +               }
> >         }
> > -       if (!err && retval != expected_val &&
> > +       if (retval != expected_val &&
> >             expected_val != POINTER_VALUE) {
> >                 printf("FAIL retval %d != %d ", retval, expected_val);
> >                 return 1;
> > --
> > 2.20.1
> >



--
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000

Re: [bpf-next v3 01/12] selftests/bpf: Print a message when tester could not run a program

[Andrii Nakryiko]

On Thu, Jul 11, 2019 at 4:36 AM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> On Thu, Jul 11, 2019 at 1:45 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> > >
> > > This prints a message when the error is about program type being not
> > > supported by the test runner or because of permissions problem. This
> > > is to see if the program we expected to run was actually executed.
> > >
> > > The messages are open-coded because strerror(ENOTSUPP) returns
> > > "Unknown error 524".
> > >
> > > Changes since v2:
> > > - Also print "FAIL" on an unexpected bpf_prog_test_run error, so there
> > >   is a corresponding "FAIL" message for each failed test.
> > >
> > > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > > ---
> > >  tools/testing/selftests/bpf/test_verifier.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > index c5514daf8865..b8d065623ead 100644
> > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > @@ -831,11 +831,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> > >                                 tmp, &size_tmp, &retval, NULL);
> > >         if (unpriv)
> > >                 set_admin(false);
> > > -       if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
> > > -               printf("Unexpected bpf_prog_test_run error ");
> > > -               return err;
> > > +       if (err) {
> > > +               switch (errno) {
> > > +               case 524/*ENOTSUPP*/:
> > > +                       printf("Did not run the program (not supported) ");
> > > +                       return 0;
> > > +               case EPERM:
> > > +                       printf("Did not run the program (no permission) ");
> >
> > Let's add "SKIP: " prefix to these?
>
> Not sure about it. The important part of the test (the program being
> verified by the kernel's verifier) was still executed, so the test is
> not really skipped.


Ah, I see. So the program was loaded/verifierd, but wasn't test-run.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
>
> >
> > > +                       return 0;
> > > +               default:
> > > +                       printf("FAIL: Unexpected bpf_prog_test_run error (%s) ", strerror(saved_errno));
> > > +                       return err;
> > > +               }
> > >         }
> > > -       if (!err && retval != expected_val &&
> > > +       if (retval != expected_val &&
> > >             expected_val != POINTER_VALUE) {
> > >                 printf("FAIL retval %d != %d ", retval, expected_val);
> > >                 return 1;
> > > --
> > > 2.20.1
> > >
>
>
>
> --
> Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
> Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
> Registergericht/Court of registration: Amtsgericht Charlottenburg
> Registernummer/Registration number: HRB 171414 B
> Ust-ID-Nummer/VAT ID number: DE302207000

[bpf-next v3 02/12] selftests/bpf: Avoid a clobbering of errno

[Krzesimir Nowak]

Save errno right after bpf_prog_test_run returns, so we later check
the error code actually set by bpf_prog_test_run, not by some libcap
function.

Changes since v1:
- Fix the "Fixes:" tag to mention actual commit that introduced the
  bug

Changes since v2:
- Move the declaration so it fits the reverse christmas tree style.

Cc: Daniel Borkmann <daniel@iogearbox.net>
Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/testing/selftests/bpf/test_verifier.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index b8d065623ead..3fe126e0083b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -823,16 +823,18 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
 	__u8 tmp[TEST_DATA_LEN << 2];
 	__u32 size_tmp = sizeof(tmp);
 	uint32_t retval;
+	int saved_errno;
 	int err;
 
 	if (unpriv)
 		set_admin(true);
 	err = bpf_prog_test_run(fd_prog, 1, data, size_data,
 				tmp, &size_tmp, &retval, NULL);
+	saved_errno = errno;
 	if (unpriv)
 		set_admin(false);
 	if (err) {
-		switch (errno) {
+		switch (saved_errno) {
 		case 524/*ENOTSUPP*/:
 			printf("Did not run the program (not supported) ");
 			return 0;
-- 
2.20.1

Re: [bpf-next v3 02/12] selftests/bpf: Avoid a clobbering of errno

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> Save errno right after bpf_prog_test_run returns, so we later check
> the error code actually set by bpf_prog_test_run, not by some libcap
> function.
>
> Changes since v1:
> - Fix the "Fixes:" tag to mention actual commit that introduced the
>   bug
>
> Changes since v2:
> - Move the declaration so it fits the reverse christmas tree style.
>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
>  tools/testing/selftests/bpf/test_verifier.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index b8d065623ead..3fe126e0083b 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -823,16 +823,18 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
>         __u8 tmp[TEST_DATA_LEN << 2];
>         __u32 size_tmp = sizeof(tmp);
>         uint32_t retval;
> +       int saved_errno;
>         int err;
>
>         if (unpriv)
>                 set_admin(true);
>         err = bpf_prog_test_run(fd_prog, 1, data, size_data,
>                                 tmp, &size_tmp, &retval, NULL);

Given err is either 0 or -1, how about instead making err useful right
here without extra variable?

if (bpf_prog_test_run(...))
        err = errno;

> +       saved_errno = errno;
>         if (unpriv)
>                 set_admin(false);
>         if (err) {
> -               switch (errno) {
> +               switch (saved_errno) {
>                 case 524/*ENOTSUPP*/:

ENOTSUPP is defined in include/linux/errno.h, is there any problem
with using this in selftests?

>                         printf("Did not run the program (not supported) ");
>                         return 0;
> --
> 2.20.1
>

Re: [bpf-next v3 02/12] selftests/bpf: Avoid a clobbering of errno

[Krzesimir Nowak]

On Thu, Jul 11, 2019 at 1:52 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> >
> > Save errno right after bpf_prog_test_run returns, so we later check
> > the error code actually set by bpf_prog_test_run, not by some libcap
> > function.
> >
> > Changes since v1:
> > - Fix the "Fixes:" tag to mention actual commit that introduced the
> >   bug
> >
> > Changes since v2:
> > - Move the declaration so it fits the reverse christmas tree style.
> >
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")
> > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > ---
> >  tools/testing/selftests/bpf/test_verifier.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index b8d065623ead..3fe126e0083b 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -823,16 +823,18 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> >         __u8 tmp[TEST_DATA_LEN << 2];
> >         __u32 size_tmp = sizeof(tmp);
> >         uint32_t retval;
> > +       int saved_errno;
> >         int err;
> >
> >         if (unpriv)
> >                 set_admin(true);
> >         err = bpf_prog_test_run(fd_prog, 1, data, size_data,
> >                                 tmp, &size_tmp, &retval, NULL);
>
> Given err is either 0 or -1, how about instead making err useful right
> here without extra variable?
>
> if (bpf_prog_test_run(...))
>         err = errno;

I change it later to bpf_prog_test_run_xattr, which can also return
-EINVAL and then errno is not set. But this one probably should not be
triggered by the test code. So not sure, probably would be better to
keep it as is for consistency?

>
> > +       saved_errno = errno;
> >         if (unpriv)
> >                 set_admin(false);
> >         if (err) {
> > -               switch (errno) {
> > +               switch (saved_errno) {
> >                 case 524/*ENOTSUPP*/:
>
> ENOTSUPP is defined in include/linux/errno.h, is there any problem
> with using this in selftests?

I just used whatever there was earlier. Seems like <linux/errno.h> is
not copied to tools include directory.

>
> >                         printf("Did not run the program (not supported) ");
> >                         return 0;
> > --
> > 2.20.1
> >



-- 
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000

Re: [bpf-next v3 02/12] selftests/bpf: Avoid a clobbering of errno

[Andrii Nakryiko]

On Thu, Jul 11, 2019 at 5:04 AM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> On Thu, Jul 11, 2019 at 1:52 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> > >
> > > Save errno right after bpf_prog_test_run returns, so we later check
> > > the error code actually set by bpf_prog_test_run, not by some libcap
> > > function.
> > >
> > > Changes since v1:
> > > - Fix the "Fixes:" tag to mention actual commit that introduced the
> > >   bug
> > >
> > > Changes since v2:
> > > - Move the declaration so it fits the reverse christmas tree style.
> > >
> > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")
> > > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > > ---
> > >  tools/testing/selftests/bpf/test_verifier.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > index b8d065623ead..3fe126e0083b 100644
> > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > @@ -823,16 +823,18 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> > >         __u8 tmp[TEST_DATA_LEN << 2];
> > >         __u32 size_tmp = sizeof(tmp);
> > >         uint32_t retval;
> > > +       int saved_errno;
> > >         int err;
> > >
> > >         if (unpriv)
> > >                 set_admin(true);
> > >         err = bpf_prog_test_run(fd_prog, 1, data, size_data,
> > >                                 tmp, &size_tmp, &retval, NULL);
> >
> > Given err is either 0 or -1, how about instead making err useful right
> > here without extra variable?
> >
> > if (bpf_prog_test_run(...))
> >         err = errno;
>
> I change it later to bpf_prog_test_run_xattr, which can also return
> -EINVAL and then errno is not set. But this one probably should not be

This is wrong. bpf_prog_test_run/bpf_prog_test_run_xattr should either
always return -1 and set errno to actual error (like syscalls do), or
always use return code with proper error. Give they are pretending to
be just pure syscall, it's probably better to set errno to EINVAL and
return -1 on invalid input args?

> triggered by the test code. So not sure, probably would be better to
> keep it as is for consistency?
>
> >
> > > +       saved_errno = errno;
> > >         if (unpriv)
> > >                 set_admin(false);
> > >         if (err) {
> > > -               switch (errno) {
> > > +               switch (saved_errno) {
> > >                 case 524/*ENOTSUPP*/:
> >
> > ENOTSUPP is defined in include/linux/errno.h, is there any problem
> > with using this in selftests?
>
> I just used whatever there was earlier. Seems like <linux/errno.h> is
> not copied to tools include directory.

Ok, let's leave it as is, thanks!

>
> >
> > >                         printf("Did not run the program (not supported) ");
> > >                         return 0;
> > > --
> > > 2.20.1
> > >
>
>
>
> --
> Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
> Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
> Registergericht/Court of registration: Amtsgericht Charlottenburg
> Registernummer/Registration number: HRB 171414 B
> Ust-ID-Nummer/VAT ID number: DE302207000

Re: [bpf-next v3 02/12] selftests/bpf: Avoid a clobbering of errno

[Krzesimir Nowak]

On Fri, Jul 12, 2019 at 2:59 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jul 11, 2019 at 5:04 AM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> >
> > On Thu, Jul 11, 2019 at 1:52 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> > > >
> > > > Save errno right after bpf_prog_test_run returns, so we later check
> > > > the error code actually set by bpf_prog_test_run, not by some libcap
> > > > function.
> > > >
> > > > Changes since v1:
> > > > - Fix the "Fixes:" tag to mention actual commit that introduced the
> > > >   bug
> > > >
> > > > Changes since v2:
> > > > - Move the declaration so it fits the reverse christmas tree style.
> > > >
> > > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > > Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")
> > > > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > > > ---
> > > >  tools/testing/selftests/bpf/test_verifier.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > > index b8d065623ead..3fe126e0083b 100644
> > > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > > @@ -823,16 +823,18 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> > > >         __u8 tmp[TEST_DATA_LEN << 2];
> > > >         __u32 size_tmp = sizeof(tmp);
> > > >         uint32_t retval;
> > > > +       int saved_errno;
> > > >         int err;
> > > >
> > > >         if (unpriv)
> > > >                 set_admin(true);
> > > >         err = bpf_prog_test_run(fd_prog, 1, data, size_data,
> > > >                                 tmp, &size_tmp, &retval, NULL);
> > >
> > > Given err is either 0 or -1, how about instead making err useful right
> > > here without extra variable?
> > >
> > > if (bpf_prog_test_run(...))
> > >         err = errno;
> >
> > I change it later to bpf_prog_test_run_xattr, which can also return
> > -EINVAL and then errno is not set. But this one probably should not be
>
> This is wrong. bpf_prog_test_run/bpf_prog_test_run_xattr should either
> always return -1 and set errno to actual error (like syscalls do), or
> always use return code with proper error. Give they are pretending to
> be just pure syscall, it's probably better to set errno to EINVAL and
> return -1 on invalid input args?

Yeah, this is inconsistent at best. But seems to be kind of expected?
See tools/testing/selftests/bpf/prog_tests/prog_run_xattr.c.

>
> > triggered by the test code. So not sure, probably would be better to
> > keep it as is for consistency?
> >
> > >
> > > > +       saved_errno = errno;
> > > >         if (unpriv)
> > > >                 set_admin(false);
> > > >         if (err) {
> > > > -               switch (errno) {
> > > > +               switch (saved_errno) {
> > > >                 case 524/*ENOTSUPP*/:
> > >
> > > ENOTSUPP is defined in include/linux/errno.h, is there any problem
> > > with using this in selftests?
> >
> > I just used whatever there was earlier. Seems like <linux/errno.h> is
> > not copied to tools include directory.
>
> Ok, let's leave it as is, thanks!
>
> >
> > >
> > > >                         printf("Did not run the program (not supported) ");
> > > >                         return 0;
> > > > --
> > > > 2.20.1
> > > >
> >
> >
> >
> > --
> > Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
> > Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
> > Registergericht/Court of registration: Amtsgericht Charlottenburg
> > Registernummer/Registration number: HRB 171414 B
> > Ust-ID-Nummer/VAT ID number: DE302207000



-- 
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000

[bpf-next v3 03/12] selftests/bpf: Avoid another case of errno clobbering

[Krzesimir Nowak]

Commit 8184d44c9a57 ("selftests/bpf: skip verifier tests for
unsupported program types") added a check for an unsupported program
type. The function doing it changes errno, so test_verifier should
save it before calling it if test_verifier wants to print a reason why
verifying a BPF program of a supported type failed.

Changes since v2:
- Move the declaration to fit the reverse christmas tree style.

Fixes: 8184d44c9a57 ("selftests/bpf: skip verifier tests for unsupported program types")
Cc: Stanislav Fomichev <sdf@google.com>
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/testing/selftests/bpf/test_verifier.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 3fe126e0083b..c7541f572932 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -864,6 +864,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 	int run_errs, run_successes;
 	int map_fds[MAX_NR_MAPS];
 	const char *expected_err;
+	int saved_errno;
 	int fixup_skips;
 	__u32 pflags;
 	int i, err;
@@ -894,6 +895,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 		pflags |= BPF_F_ANY_ALIGNMENT;
 	fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
 				     "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
+	saved_errno = errno;
 	if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
 		printf("SKIP (unsupported program type %d)\n", prog_type);
 		skips++;
@@ -910,7 +912,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 	if (expected_ret == ACCEPT) {
 		if (fd_prog < 0) {
 			printf("FAIL\nFailed to load prog '%s'!\n",
-			       strerror(errno));
+			       strerror(saved_errno));
 			goto fail_log;
 		}
 #ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-- 
2.20.1

Re: [bpf-next v3 03/12] selftests/bpf: Avoid another case of errno clobbering

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 3:43 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> Commit 8184d44c9a57 ("selftests/bpf: skip verifier tests for
> unsupported program types") added a check for an unsupported program
> type. The function doing it changes errno, so test_verifier should
> save it before calling it if test_verifier wants to print a reason why
> verifying a BPF program of a supported type failed.
>
> Changes since v2:
> - Move the declaration to fit the reverse christmas tree style.
>
> Fixes: 8184d44c9a57 ("selftests/bpf: skip verifier tests for unsupported program types")
> Cc: Stanislav Fomichev <sdf@google.com>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/test_verifier.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 3fe126e0083b..c7541f572932 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -864,6 +864,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>         int run_errs, run_successes;
>         int map_fds[MAX_NR_MAPS];
>         const char *expected_err;
> +       int saved_errno;
>         int fixup_skips;

nit: combine those ints? or even with i and err below as well?

>         __u32 pflags;
>         int i, err;
> @@ -894,6 +895,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>                 pflags |= BPF_F_ANY_ALIGNMENT;
>         fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
>                                      "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
> +       saved_errno = errno;
>         if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
>                 printf("SKIP (unsupported program type %d)\n", prog_type);
>                 skips++;
> @@ -910,7 +912,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>         if (expected_ret == ACCEPT) {
>                 if (fd_prog < 0) {
>                         printf("FAIL\nFailed to load prog '%s'!\n",
> -                              strerror(errno));
> +                              strerror(saved_errno));
>                         goto fail_log;
>                 }
>  #ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> --
> 2.20.1
>

Re: [bpf-next v3 03/12] selftests/bpf: Avoid another case of errno clobbering

[Krzesimir Nowak]

On Thu, Jul 11, 2019 at 1:57 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 8, 2019 at 3:43 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> >
> > Commit 8184d44c9a57 ("selftests/bpf: skip verifier tests for
> > unsupported program types") added a check for an unsupported program
> > type. The function doing it changes errno, so test_verifier should
> > save it before calling it if test_verifier wants to print a reason why
> > verifying a BPF program of a supported type failed.
> >
> > Changes since v2:
> > - Move the declaration to fit the reverse christmas tree style.
> >
> > Fixes: 8184d44c9a57 ("selftests/bpf: skip verifier tests for unsupported program types")
> > Cc: Stanislav Fomichev <sdf@google.com>
> > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > ---
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> >  tools/testing/selftests/bpf/test_verifier.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 3fe126e0083b..c7541f572932 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -864,6 +864,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >         int run_errs, run_successes;
> >         int map_fds[MAX_NR_MAPS];
> >         const char *expected_err;
> > +       int saved_errno;
> >         int fixup_skips;
>
> nit: combine those ints? or even with i and err below as well?

Will do.

>
> >         __u32 pflags;
> >         int i, err;
> > @@ -894,6 +895,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >                 pflags |= BPF_F_ANY_ALIGNMENT;
> >         fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
> >                                      "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
> > +       saved_errno = errno;
> >         if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
> >                 printf("SKIP (unsupported program type %d)\n", prog_type);
> >                 skips++;
> > @@ -910,7 +912,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >         if (expected_ret == ACCEPT) {
> >                 if (fd_prog < 0) {
> >                         printf("FAIL\nFailed to load prog '%s'!\n",
> > -                              strerror(errno));
> > +                              strerror(saved_errno));
> >                         goto fail_log;
> >                 }
> >  #ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> > --
> > 2.20.1
> >



-- 
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000

[bpf-next v3 04/12] selftests/bpf: Use bpf_prog_test_run_xattr

[Krzesimir Nowak]

The bpf_prog_test_run_xattr function gives more options to set up a
test run of a BPF program than the bpf_prog_test_run function.

We will need this extra flexibility to pass ctx data later.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/testing/selftests/bpf/test_verifier.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c7541f572932..1640ba9f12c1 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -822,14 +822,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
 {
 	__u8 tmp[TEST_DATA_LEN << 2];
 	__u32 size_tmp = sizeof(tmp);
-	uint32_t retval;
 	int saved_errno;
 	int err;
+	struct bpf_prog_test_run_attr attr = {
+		.prog_fd = fd_prog,
+		.repeat = 1,
+		.data_in = data,
+		.data_size_in = size_data,
+		.data_out = tmp,
+		.data_size_out = size_tmp,
+	};
 
 	if (unpriv)
 		set_admin(true);
-	err = bpf_prog_test_run(fd_prog, 1, data, size_data,
-				tmp, &size_tmp, &retval, NULL);
+	err = bpf_prog_test_run_xattr(&attr);
 	saved_errno = errno;
 	if (unpriv)
 		set_admin(false);
@@ -846,9 +852,9 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
 			return err;
 		}
 	}
-	if (retval != expected_val &&
+	if (attr.retval != expected_val &&
 	    expected_val != POINTER_VALUE) {
-		printf("FAIL retval %d != %d ", retval, expected_val);
+		printf("FAIL retval %d != %d ", attr.retval, expected_val);
 		return 1;
 	}
 
-- 
2.20.1

Re: [bpf-next v3 04/12] selftests/bpf: Use bpf_prog_test_run_xattr

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 3:43 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> The bpf_prog_test_run_xattr function gives more options to set up a
> test run of a BPF program than the bpf_prog_test_run function.
>
> We will need this extra flexibility to pass ctx data later.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---

lgtm, with some nits below

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/test_verifier.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c7541f572932..1640ba9f12c1 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -822,14 +822,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
>  {
>         __u8 tmp[TEST_DATA_LEN << 2];
>         __u32 size_tmp = sizeof(tmp);

nit: this is now is not needed as a separate local variable, inline?

> -       uint32_t retval;
>         int saved_errno;
>         int err;
> +       struct bpf_prog_test_run_attr attr = {
> +               .prog_fd = fd_prog,
> +               .repeat = 1,
> +               .data_in = data,
> +               .data_size_in = size_data,
> +               .data_out = tmp,
> +               .data_size_out = size_tmp,
> +       };
>
>         if (unpriv)
>                 set_admin(true);
> -       err = bpf_prog_test_run(fd_prog, 1, data, size_data,
> -                               tmp, &size_tmp, &retval, NULL);
> +       err = bpf_prog_test_run_xattr(&attr);
>         saved_errno = errno;
>         if (unpriv)
>                 set_admin(false);
> @@ -846,9 +852,9 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
>                         return err;
>                 }
>         }
> -       if (retval != expected_val &&
> +       if (attr.retval != expected_val &&
>             expected_val != POINTER_VALUE) {

this if condition now fits one line, can you please combine? thanks!

> -               printf("FAIL retval %d != %d ", retval, expected_val);
> +               printf("FAIL retval %d != %d ", attr.retval, expected_val);
>                 return 1;
>         }
>
> --
> 2.20.1
>

Re: [bpf-next v3 04/12] selftests/bpf: Use bpf_prog_test_run_xattr

[Krzesimir Nowak]

On Thu, Jul 11, 2019 at 2:03 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 8, 2019 at 3:43 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> >
> > The bpf_prog_test_run_xattr function gives more options to set up a
> > test run of a BPF program than the bpf_prog_test_run function.
> >
> > We will need this extra flexibility to pass ctx data later.
> >
> > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > ---
>
> lgtm, with some nits below
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> >  tools/testing/selftests/bpf/test_verifier.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index c7541f572932..1640ba9f12c1 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -822,14 +822,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> >  {
> >         __u8 tmp[TEST_DATA_LEN << 2];
> >         __u32 size_tmp = sizeof(tmp);
>
> nit: this is now is not needed as a separate local variable, inline?

I think I'm using this variable in a followup commit, but I'll look closely.

>
> > -       uint32_t retval;
> >         int saved_errno;
> >         int err;
> > +       struct bpf_prog_test_run_attr attr = {
> > +               .prog_fd = fd_prog,
> > +               .repeat = 1,
> > +               .data_in = data,
> > +               .data_size_in = size_data,
> > +               .data_out = tmp,
> > +               .data_size_out = size_tmp,
> > +       };
> >
> >         if (unpriv)
> >                 set_admin(true);
> > -       err = bpf_prog_test_run(fd_prog, 1, data, size_data,
> > -                               tmp, &size_tmp, &retval, NULL);
> > +       err = bpf_prog_test_run_xattr(&attr);
> >         saved_errno = errno;
> >         if (unpriv)
> >                 set_admin(false);
> > @@ -846,9 +852,9 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> >                         return err;
> >                 }
> >         }
> > -       if (retval != expected_val &&
> > +       if (attr.retval != expected_val &&
> >             expected_val != POINTER_VALUE) {
>
> this if condition now fits one line, can you please combine? thanks!

Sure.

>
> > -               printf("FAIL retval %d != %d ", retval, expected_val);
> > +               printf("FAIL retval %d != %d ", attr.retval, expected_val);
> >                 return 1;
> >         }
> >
> > --
> > 2.20.1
> >



-- 
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000

[bpf-next v3 05/12] selftests/bpf: Allow passing more information to BPF prog test run

[Krzesimir Nowak]

The test case can now specify a custom length of the data member,
context data and its length, which will be passed to
bpf_prog_test_run_xattr. For backward compatilibity, if the data
length is 0 (which is what will happen when the field is left
unspecified in the designated initializer of a struct), then the
length passed to the bpf_prog_test_run_xattr is TEST_DATA_LEN.

Also for backward compatilibity, if context data length is 0, NULL is
passed as a context to bpf_prog_test_run_xattr. This is to avoid
breaking other tests, where context data being NULL and context data
length being 0 is handled differently from the case where context data
is not NULL and context data length is 0.

Custom lengths still can't be greater than hardcoded 64 bytes for data
and 192 for context data.

192 for context data was picked to allow passing struct
bpf_perf_event_data as a context for perf event programs. The struct
is quite large, because it contains struct pt_regs.

Test runs for perf event programs will not allow the copying the data
back to data_out buffer, so they require data_out_size to be zero and
data_out to be NULL. Since test_verifier hardcodes it, make it
possible to override the size. Overriding the size to zero will cause
the buffer to be NULL.

Changes since v2:
- Allow overriding the data out size and buffer.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/testing/selftests/bpf/test_verifier.c | 105 +++++++++++++++++---
 1 file changed, 93 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 1640ba9f12c1..6f124cc4ee34 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -54,6 +54,7 @@
 #define MAX_TEST_RUNS	8
 #define POINTER_VALUE	0xcafe4all
 #define TEST_DATA_LEN	64
+#define TEST_CTX_LEN	192
 
 #define F_NEEDS_EFFICIENT_UNALIGNED_ACCESS	(1 << 0)
 #define F_LOAD_WITH_STRICT_ALIGNMENT		(1 << 1)
@@ -96,7 +97,12 @@ struct bpf_test {
 	enum bpf_prog_type prog_type;
 	uint8_t flags;
 	__u8 data[TEST_DATA_LEN];
+	__u32 data_len;
+	__u8 ctx[TEST_CTX_LEN];
+	__u32 ctx_len;
 	void (*fill_helper)(struct bpf_test *self);
+	bool override_data_out_len;
+	__u32 overridden_data_out_len;
 	uint8_t runs;
 	struct {
 		uint32_t retval, retval_unpriv;
@@ -104,6 +110,9 @@ struct bpf_test {
 			__u8 data[TEST_DATA_LEN];
 			__u64 data64[TEST_DATA_LEN / 8];
 		};
+		__u32 data_len;
+		__u8 ctx[TEST_CTX_LEN];
+		__u32 ctx_len;
 	} retvals[MAX_TEST_RUNS];
 };
 
@@ -818,21 +827,35 @@ static int set_admin(bool admin)
 }
 
 static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
-			    void *data, size_t size_data)
+			    void *data, size_t size_data, void *ctx,
+			    size_t size_ctx, u32 *overridden_data_out_size)
 {
-	__u8 tmp[TEST_DATA_LEN << 2];
-	__u32 size_tmp = sizeof(tmp);
-	int saved_errno;
-	int err;
 	struct bpf_prog_test_run_attr attr = {
 		.prog_fd = fd_prog,
 		.repeat = 1,
 		.data_in = data,
 		.data_size_in = size_data,
-		.data_out = tmp,
-		.data_size_out = size_tmp,
+		.ctx_in = ctx,
+		.ctx_size_in = size_ctx,
 	};
+	__u8 tmp[TEST_DATA_LEN << 2];
+	__u32 size_tmp = sizeof(tmp);
+	__u32 size_buf = size_tmp;
+	__u8 *buf = tmp;
+	int saved_errno;
+	int err;
 
+	if (overridden_data_out_size)
+		size_buf = *overridden_data_out_size;
+	if (size_buf > size_tmp) {
+		printf("FAIL: out data size (%d) greater than a buffer size (%d) ",
+		       size_buf, size_tmp);
+		return -EINVAL;
+	}
+	if (!size_buf)
+		buf = NULL;
+	attr.data_size_out = size_buf;
+	attr.data_out = buf;
 	if (unpriv)
 		set_admin(true);
 	err = bpf_prog_test_run_xattr(&attr);
@@ -956,13 +979,45 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 	if (!alignment_prevented_execution && fd_prog >= 0) {
 		uint32_t expected_val;
 		int i;
+		__u32 size_data;
+		__u32 size_ctx;
+		bool bad_size;
+		void *ctx;
+		__u32 *overridden_data_out_size;
 
 		if (!test->runs) {
+			if (test->data_len > 0)
+				size_data = test->data_len;
+			else
+				size_data = sizeof(test->data);
+			if (test->override_data_out_len)
+				overridden_data_out_size = &test->overridden_data_out_len;
+			else
+				overridden_data_out_size = NULL;
+			size_ctx = test->ctx_len;
+			bad_size = false;
 			expected_val = unpriv && test->retval_unpriv ?
 				test->retval_unpriv : test->retval;
 
-			err = do_prog_test_run(fd_prog, unpriv, expected_val,
-					       test->data, sizeof(test->data));
+			if (size_data > sizeof(test->data)) {
+				printf("FAIL: data size (%u) greater than TEST_DATA_LEN (%lu) ", size_data, sizeof(test->data));
+				bad_size = true;
+			}
+			if (size_ctx > sizeof(test->ctx)) {
+				printf("FAIL: ctx size (%u) greater than TEST_CTX_LEN (%lu) ", size_ctx, sizeof(test->ctx));
+				bad_size = true;
+			}
+			if (size_ctx)
+				ctx = test->ctx;
+			else
+				ctx = NULL;
+			if (bad_size)
+				err = 1;
+			else
+				err = do_prog_test_run(fd_prog, unpriv, expected_val,
+						       test->data, size_data,
+						       ctx, size_ctx,
+						       overridden_data_out_size);
 			if (err)
 				run_errs++;
 			else
@@ -970,14 +1025,40 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 		}
 
 		for (i = 0; i < test->runs; i++) {
+			if (test->retvals[i].data_len > 0)
+				size_data = test->retvals[i].data_len;
+			else
+				size_data = sizeof(test->retvals[i].data);
+			if (test->override_data_out_len)
+				overridden_data_out_size = &test->overridden_data_out_len;
+			else
+				overridden_data_out_size = NULL;
+			size_ctx = test->retvals[i].ctx_len;
+			bad_size = false;
 			if (unpriv && test->retvals[i].retval_unpriv)
 				expected_val = test->retvals[i].retval_unpriv;
 			else
 				expected_val = test->retvals[i].retval;
 
-			err = do_prog_test_run(fd_prog, unpriv, expected_val,
-					       test->retvals[i].data,
-					       sizeof(test->retvals[i].data));
+			if (size_data > sizeof(test->retvals[i].data)) {
+				printf("FAIL: data size (%u) at run %i greater than TEST_DATA_LEN (%lu) ", size_data, i + 1, sizeof(test->retvals[i].data));
+				bad_size = true;
+			}
+			if (size_ctx > sizeof(test->retvals[i].ctx)) {
+				printf("FAIL: ctx size (%u) at run %i greater than TEST_CTX_LEN (%lu) ", size_ctx, i + 1, sizeof(test->retvals[i].ctx));
+				bad_size = true;
+			}
+			if (size_ctx)
+				ctx = test->retvals[i].ctx;
+			else
+				ctx = NULL;
+			if (bad_size)
+				err = 1;
+			else
+				err = do_prog_test_run(fd_prog, unpriv, expected_val,
+						       test->retvals[i].data, size_data,
+						       ctx, size_ctx,
+						       overridden_data_out_size);
 			if (err) {
 				printf("(run %d/%d) ", i + 1, test->runs);
 				run_errs++;
-- 
2.20.1

Re: [bpf-next v3 05/12] selftests/bpf: Allow passing more information to BPF prog test run

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> The test case can now specify a custom length of the data member,
> context data and its length, which will be passed to
> bpf_prog_test_run_xattr. For backward compatilibity, if the data
> length is 0 (which is what will happen when the field is left
> unspecified in the designated initializer of a struct), then the
> length passed to the bpf_prog_test_run_xattr is TEST_DATA_LEN.
>
> Also for backward compatilibity, if context data length is 0, NULL is
> passed as a context to bpf_prog_test_run_xattr. This is to avoid
> breaking other tests, where context data being NULL and context data
> length being 0 is handled differently from the case where context data
> is not NULL and context data length is 0.
>
> Custom lengths still can't be greater than hardcoded 64 bytes for data
> and 192 for context data.
>
> 192 for context data was picked to allow passing struct
> bpf_perf_event_data as a context for perf event programs. The struct
> is quite large, because it contains struct pt_regs.
>
> Test runs for perf event programs will not allow the copying the data
> back to data_out buffer, so they require data_out_size to be zero and
> data_out to be NULL. Since test_verifier hardcodes it, make it
> possible to override the size. Overriding the size to zero will cause
> the buffer to be NULL.
>
> Changes since v2:
> - Allow overriding the data out size and buffer.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
>  tools/testing/selftests/bpf/test_verifier.c | 105 +++++++++++++++++---
>  1 file changed, 93 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 1640ba9f12c1..6f124cc4ee34 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -54,6 +54,7 @@
>  #define MAX_TEST_RUNS  8
>  #define POINTER_VALUE  0xcafe4all
>  #define TEST_DATA_LEN  64
> +#define TEST_CTX_LEN   192
>
>  #define F_NEEDS_EFFICIENT_UNALIGNED_ACCESS     (1 << 0)
>  #define F_LOAD_WITH_STRICT_ALIGNMENT           (1 << 1)
> @@ -96,7 +97,12 @@ struct bpf_test {
>         enum bpf_prog_type prog_type;
>         uint8_t flags;
>         __u8 data[TEST_DATA_LEN];
> +       __u32 data_len;
> +       __u8 ctx[TEST_CTX_LEN];
> +       __u32 ctx_len;
>         void (*fill_helper)(struct bpf_test *self);
> +       bool override_data_out_len;
> +       __u32 overridden_data_out_len;
>         uint8_t runs;
>         struct {
>                 uint32_t retval, retval_unpriv;
> @@ -104,6 +110,9 @@ struct bpf_test {
>                         __u8 data[TEST_DATA_LEN];
>                         __u64 data64[TEST_DATA_LEN / 8];
>                 };
> +               __u32 data_len;
> +               __u8 ctx[TEST_CTX_LEN];
> +               __u32 ctx_len;
>         } retvals[MAX_TEST_RUNS];
>  };
>
> @@ -818,21 +827,35 @@ static int set_admin(bool admin)
>  }
>
>  static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> -                           void *data, size_t size_data)
> +                           void *data, size_t size_data, void *ctx,
> +                           size_t size_ctx, u32 *overridden_data_out_size)
>  {
> -       __u8 tmp[TEST_DATA_LEN << 2];
> -       __u32 size_tmp = sizeof(tmp);
> -       int saved_errno;
> -       int err;
>         struct bpf_prog_test_run_attr attr = {
>                 .prog_fd = fd_prog,
>                 .repeat = 1,
>                 .data_in = data,
>                 .data_size_in = size_data,
> -               .data_out = tmp,
> -               .data_size_out = size_tmp,
> +               .ctx_in = ctx,
> +               .ctx_size_in = size_ctx,
>         };
> +       __u8 tmp[TEST_DATA_LEN << 2];
> +       __u32 size_tmp = sizeof(tmp);
> +       __u32 size_buf = size_tmp;
> +       __u8 *buf = tmp;
> +       int saved_errno;
> +       int err;
>
> +       if (overridden_data_out_size)
> +               size_buf = *overridden_data_out_size;
> +       if (size_buf > size_tmp) {
> +               printf("FAIL: out data size (%d) greater than a buffer size (%d) ",
> +                      size_buf, size_tmp);
> +               return -EINVAL;
> +       }
> +       if (!size_buf)
> +               buf = NULL;
> +       attr.data_size_out = size_buf;
> +       attr.data_out = buf;
>         if (unpriv)
>                 set_admin(true);
>         err = bpf_prog_test_run_xattr(&attr);
> @@ -956,13 +979,45 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>         if (!alignment_prevented_execution && fd_prog >= 0) {
>                 uint32_t expected_val;
>                 int i;
> +               __u32 size_data;
> +               __u32 size_ctx;
> +               bool bad_size;
> +               void *ctx;
> +               __u32 *overridden_data_out_size;
>
>                 if (!test->runs) {
> +                       if (test->data_len > 0)
> +                               size_data = test->data_len;
> +                       else
> +                               size_data = sizeof(test->data);
> +                       if (test->override_data_out_len)
> +                               overridden_data_out_size = &test->overridden_data_out_len;
> +                       else
> +                               overridden_data_out_size = NULL;
> +                       size_ctx = test->ctx_len;
> +                       bad_size = false;

I hated all this duplication of logic, which with this patch becomes
even more expansive, so I removed it. Please see [0]. Can you please
apply that patch and add all this new logic only once?

  [0] https://patchwork.ozlabs.org/patch/1130601/

>                         expected_val = unpriv && test->retval_unpriv ?
>                                 test->retval_unpriv : test->retval;
>
> -                       err = do_prog_test_run(fd_prog, unpriv, expected_val,
> -                                              test->data, sizeof(test->data));
> +                       if (size_data > sizeof(test->data)) {
> +                               printf("FAIL: data size (%u) greater than TEST_DATA_LEN (%lu) ", size_data, sizeof(test->data));
> +                               bad_size = true;
> +                       }
> +                       if (size_ctx > sizeof(test->ctx)) {
> +                               printf("FAIL: ctx size (%u) greater than TEST_CTX_LEN (%lu) ", size_ctx, sizeof(test->ctx));

These look like way too long lines, wrap them?

> +                               bad_size = true;
> +                       }
> +                       if (size_ctx)
> +                               ctx = test->ctx;
> +                       else
> +                               ctx = NULL;

nit: single line:

ctx = size_ctx ? test->ctx : NULL;

> +                       if (bad_size)
> +                               err = 1;
> +                       else
> +                               err = do_prog_test_run(fd_prog, unpriv, expected_val,
> +                                                      test->data, size_data,
> +                                                      ctx, size_ctx,
> +                                                      overridden_data_out_size);
>                         if (err)
>                                 run_errs++;
>                         else
> @@ -970,14 +1025,40 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>                 }
>
>                 for (i = 0; i < test->runs; i++) {
> +                       if (test->retvals[i].data_len > 0)
> +                               size_data = test->retvals[i].data_len;
> +                       else
> +                               size_data = sizeof(test->retvals[i].data);
> +                       if (test->override_data_out_len)
> +                               overridden_data_out_size = &test->overridden_data_out_len;
> +                       else
> +                               overridden_data_out_size = NULL;
> +                       size_ctx = test->retvals[i].ctx_len;
> +                       bad_size = false;
>                         if (unpriv && test->retvals[i].retval_unpriv)
>                                 expected_val = test->retvals[i].retval_unpriv;
>                         else
>                                 expected_val = test->retvals[i].retval;
>
> -                       err = do_prog_test_run(fd_prog, unpriv, expected_val,
> -                                              test->retvals[i].data,
> -                                              sizeof(test->retvals[i].data));
> +                       if (size_data > sizeof(test->retvals[i].data)) {
> +                               printf("FAIL: data size (%u) at run %i greater than TEST_DATA_LEN (%lu) ", size_data, i + 1, sizeof(test->retvals[i].data));
> +                               bad_size = true;
> +                       }
> +                       if (size_ctx > sizeof(test->retvals[i].ctx)) {
> +                               printf("FAIL: ctx size (%u) at run %i greater than TEST_CTX_LEN (%lu) ", size_ctx, i + 1, sizeof(test->retvals[i].ctx));
> +                               bad_size = true;
> +                       }
> +                       if (size_ctx)
> +                               ctx = test->retvals[i].ctx;
> +                       else
> +                               ctx = NULL;
> +                       if (bad_size)
> +                               err = 1;
> +                       else
> +                               err = do_prog_test_run(fd_prog, unpriv, expected_val,
> +                                                      test->retvals[i].data, size_data,
> +                                                      ctx, size_ctx,
> +                                                      overridden_data_out_size);
>                         if (err) {
>                                 printf("(run %d/%d) ", i + 1, test->runs);
>                                 run_errs++;
> --
> 2.20.1
>

Re: [bpf-next v3 05/12] selftests/bpf: Allow passing more information to BPF prog test run

[Krzesimir Nowak]

On Thu, Jul 11, 2019 at 3:17 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> >
> > The test case can now specify a custom length of the data member,
> > context data and its length, which will be passed to
> > bpf_prog_test_run_xattr. For backward compatilibity, if the data
> > length is 0 (which is what will happen when the field is left
> > unspecified in the designated initializer of a struct), then the
> > length passed to the bpf_prog_test_run_xattr is TEST_DATA_LEN.
> >
> > Also for backward compatilibity, if context data length is 0, NULL is
> > passed as a context to bpf_prog_test_run_xattr. This is to avoid
> > breaking other tests, where context data being NULL and context data
> > length being 0 is handled differently from the case where context data
> > is not NULL and context data length is 0.
> >
> > Custom lengths still can't be greater than hardcoded 64 bytes for data
> > and 192 for context data.
> >
> > 192 for context data was picked to allow passing struct
> > bpf_perf_event_data as a context for perf event programs. The struct
> > is quite large, because it contains struct pt_regs.
> >
> > Test runs for perf event programs will not allow the copying the data
> > back to data_out buffer, so they require data_out_size to be zero and
> > data_out to be NULL. Since test_verifier hardcodes it, make it
> > possible to override the size. Overriding the size to zero will cause
> > the buffer to be NULL.
> >
> > Changes since v2:
> > - Allow overriding the data out size and buffer.
> >
> > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > ---
> >  tools/testing/selftests/bpf/test_verifier.c | 105 +++++++++++++++++---
> >  1 file changed, 93 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 1640ba9f12c1..6f124cc4ee34 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -54,6 +54,7 @@
> >  #define MAX_TEST_RUNS  8
> >  #define POINTER_VALUE  0xcafe4all
> >  #define TEST_DATA_LEN  64
> > +#define TEST_CTX_LEN   192
> >
> >  #define F_NEEDS_EFFICIENT_UNALIGNED_ACCESS     (1 << 0)
> >  #define F_LOAD_WITH_STRICT_ALIGNMENT           (1 << 1)
> > @@ -96,7 +97,12 @@ struct bpf_test {
> >         enum bpf_prog_type prog_type;
> >         uint8_t flags;
> >         __u8 data[TEST_DATA_LEN];
> > +       __u32 data_len;
> > +       __u8 ctx[TEST_CTX_LEN];
> > +       __u32 ctx_len;
> >         void (*fill_helper)(struct bpf_test *self);
> > +       bool override_data_out_len;
> > +       __u32 overridden_data_out_len;
> >         uint8_t runs;
> >         struct {
> >                 uint32_t retval, retval_unpriv;
> > @@ -104,6 +110,9 @@ struct bpf_test {
> >                         __u8 data[TEST_DATA_LEN];
> >                         __u64 data64[TEST_DATA_LEN / 8];
> >                 };
> > +               __u32 data_len;
> > +               __u8 ctx[TEST_CTX_LEN];
> > +               __u32 ctx_len;
> >         } retvals[MAX_TEST_RUNS];
> >  };
> >
> > @@ -818,21 +827,35 @@ static int set_admin(bool admin)
> >  }
> >
> >  static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> > -                           void *data, size_t size_data)
> > +                           void *data, size_t size_data, void *ctx,
> > +                           size_t size_ctx, u32 *overridden_data_out_size)
> >  {
> > -       __u8 tmp[TEST_DATA_LEN << 2];
> > -       __u32 size_tmp = sizeof(tmp);
> > -       int saved_errno;
> > -       int err;
> >         struct bpf_prog_test_run_attr attr = {
> >                 .prog_fd = fd_prog,
> >                 .repeat = 1,
> >                 .data_in = data,
> >                 .data_size_in = size_data,
> > -               .data_out = tmp,
> > -               .data_size_out = size_tmp,
> > +               .ctx_in = ctx,
> > +               .ctx_size_in = size_ctx,
> >         };
> > +       __u8 tmp[TEST_DATA_LEN << 2];
> > +       __u32 size_tmp = sizeof(tmp);
> > +       __u32 size_buf = size_tmp;
> > +       __u8 *buf = tmp;
> > +       int saved_errno;
> > +       int err;
> >
> > +       if (overridden_data_out_size)
> > +               size_buf = *overridden_data_out_size;
> > +       if (size_buf > size_tmp) {
> > +               printf("FAIL: out data size (%d) greater than a buffer size (%d) ",
> > +                      size_buf, size_tmp);
> > +               return -EINVAL;
> > +       }
> > +       if (!size_buf)
> > +               buf = NULL;
> > +       attr.data_size_out = size_buf;
> > +       attr.data_out = buf;
> >         if (unpriv)
> >                 set_admin(true);
> >         err = bpf_prog_test_run_xattr(&attr);
> > @@ -956,13 +979,45 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >         if (!alignment_prevented_execution && fd_prog >= 0) {
> >                 uint32_t expected_val;
> >                 int i;
> > +               __u32 size_data;
> > +               __u32 size_ctx;
> > +               bool bad_size;
> > +               void *ctx;
> > +               __u32 *overridden_data_out_size;
> >
> >                 if (!test->runs) {
> > +                       if (test->data_len > 0)
> > +                               size_data = test->data_len;
> > +                       else
> > +                               size_data = sizeof(test->data);
> > +                       if (test->override_data_out_len)
> > +                               overridden_data_out_size = &test->overridden_data_out_len;
> > +                       else
> > +                               overridden_data_out_size = NULL;
> > +                       size_ctx = test->ctx_len;
> > +                       bad_size = false;
>
> I hated all this duplication of logic, which with this patch becomes
> even more expansive, so I removed it. Please see [0]. Can you please
> apply that patch and add all this new logic only once?
>
>   [0] https://patchwork.ozlabs.org/patch/1130601/

Will do.

>
> >                         expected_val = unpriv && test->retval_unpriv ?
> >                                 test->retval_unpriv : test->retval;
> >
> > -                       err = do_prog_test_run(fd_prog, unpriv, expected_val,
> > -                                              test->data, sizeof(test->data));
> > +                       if (size_data > sizeof(test->data)) {
> > +                               printf("FAIL: data size (%u) greater than TEST_DATA_LEN (%lu) ", size_data, sizeof(test->data));
> > +                               bad_size = true;
> > +                       }
> > +                       if (size_ctx > sizeof(test->ctx)) {
> > +                               printf("FAIL: ctx size (%u) greater than TEST_CTX_LEN (%lu) ", size_ctx, sizeof(test->ctx));
>
> These look like way too long lines, wrap them?

Ah, yeah, these can be wrapped easily. Will do.

>
> > +                               bad_size = true;
> > +                       }
> > +                       if (size_ctx)
> > +                               ctx = test->ctx;
> > +                       else
> > +                               ctx = NULL;
>
> nit: single line:
>
> ctx = size_ctx ? test->ctx : NULL;
>
> > +                       if (bad_size)
> > +                               err = 1;
> > +                       else
> > +                               err = do_prog_test_run(fd_prog, unpriv, expected_val,
> > +                                                      test->data, size_data,
> > +                                                      ctx, size_ctx,
> > +                                                      overridden_data_out_size);
> >                         if (err)
> >                                 run_errs++;
> >                         else
> > @@ -970,14 +1025,40 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >                 }
> >
> >                 for (i = 0; i < test->runs; i++) {
> > +                       if (test->retvals[i].data_len > 0)
> > +                               size_data = test->retvals[i].data_len;
> > +                       else
> > +                               size_data = sizeof(test->retvals[i].data);
> > +                       if (test->override_data_out_len)
> > +                               overridden_data_out_size = &test->overridden_data_out_len;
> > +                       else
> > +                               overridden_data_out_size = NULL;
> > +                       size_ctx = test->retvals[i].ctx_len;
> > +                       bad_size = false;
> >                         if (unpriv && test->retvals[i].retval_unpriv)
> >                                 expected_val = test->retvals[i].retval_unpriv;
> >                         else
> >                                 expected_val = test->retvals[i].retval;
> >
> > -                       err = do_prog_test_run(fd_prog, unpriv, expected_val,
> > -                                              test->retvals[i].data,
> > -                                              sizeof(test->retvals[i].data));
> > +                       if (size_data > sizeof(test->retvals[i].data)) {
> > +                               printf("FAIL: data size (%u) at run %i greater than TEST_DATA_LEN (%lu) ", size_data, i + 1, sizeof(test->retvals[i].data));
> > +                               bad_size = true;
> > +                       }
> > +                       if (size_ctx > sizeof(test->retvals[i].ctx)) {
> > +                               printf("FAIL: ctx size (%u) at run %i greater than TEST_CTX_LEN (%lu) ", size_ctx, i + 1, sizeof(test->retvals[i].ctx));
> > +                               bad_size = true;
> > +                       }
> > +                       if (size_ctx)
> > +                               ctx = test->retvals[i].ctx;
> > +                       else
> > +                               ctx = NULL;
> > +                       if (bad_size)
> > +                               err = 1;
> > +                       else
> > +                               err = do_prog_test_run(fd_prog, unpriv, expected_val,
> > +                                                      test->retvals[i].data, size_data,
> > +                                                      ctx, size_ctx,
> > +                                                      overridden_data_out_size);
> >                         if (err) {
> >                                 printf("(run %d/%d) ", i + 1, test->runs);
> >                                 run_errs++;
> > --
> > 2.20.1
> >



-- 
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000

[bpf-next v3 06/12] selftests/bpf: Make sure that preexisting tests for perf event work

[Krzesimir Nowak]

We are going to introduce a test run implementation for perf event in
a later commit and it will not allow passing any data out or ctx out
to it, and requires their sizes to be specified to zero. To avoid test
failures when the feature is introduced, override the data out size to
zero. That will also cause NULL buffer to be sent to the kernel.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 .../testing/selftests/bpf/verifier/perf_event_sample_period.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
index 471c1a5950d8..19f5d824b275 100644
--- a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
+++ b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
@@ -13,6 +13,7 @@
 	},
 	.result = ACCEPT,
 	.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	.override_data_out_len = true,
 },
 {
 	"check bpf_perf_event_data->sample_period half load permitted",
@@ -29,6 +30,7 @@
 	},
 	.result = ACCEPT,
 	.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	.override_data_out_len = true,
 },
 {
 	"check bpf_perf_event_data->sample_period word load permitted",
@@ -45,6 +47,7 @@
 	},
 	.result = ACCEPT,
 	.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	.override_data_out_len = true,
 },
 {
 	"check bpf_perf_event_data->sample_period dword load permitted",
@@ -56,4 +59,5 @@
 	},
 	.result = ACCEPT,
 	.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	.override_data_out_len = true,
 },
-- 
2.20.1

[bpf-next v3 07/12] tools headers: Adopt compiletime_assert from kernel sources

[Krzesimir Nowak]

This will come in handy to verify that the hardcoded size of the
context data in bpf_test struct is high enough to hold some struct.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/include/linux/compiler.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 1827c2f973f9..b4e97751000a 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -172,4 +172,32 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 # define __fallthrough
 #endif
 
+
+#ifdef __OPTIMIZE__
+# define __compiletime_assert(condition, msg, prefix, suffix)		\
+	do {								\
+		extern void prefix ## suffix(void) __compiletime_error(msg); \
+		if (!(condition))					\
+			prefix ## suffix();				\
+	} while (0)
+#else
+# define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
+#endif
+
+#define _compiletime_assert(condition, msg, prefix, suffix) \
+	__compiletime_assert(condition, msg, prefix, suffix)
+
+/**
+ * compiletime_assert - break build and emit msg if condition is false
+ * @condition: a compile-time constant condition to check
+ * @msg:       a message to emit if condition is false
+ *
+ * In tradition of POSIX assert, this macro will break the build if the
+ * supplied condition is *false*, emitting the supplied error message if the
+ * compiler has support to do so.
+ */
+#define compiletime_assert(condition, msg) \
+	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
+
+
 #endif /* _TOOLS_LINUX_COMPILER_H */
-- 
2.20.1

Re: [bpf-next v3 07/12] tools headers: Adopt compiletime_assert from kernel sources

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> This will come in handy to verify that the hardcoded size of the
> context data in bpf_test struct is high enough to hold some struct.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>


>  tools/include/linux/compiler.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
> index 1827c2f973f9..b4e97751000a 100644
> --- a/tools/include/linux/compiler.h
> +++ b/tools/include/linux/compiler.h

[...]

[bpf-next v3 08/12] tools headers: Sync struct bpf_perf_event_data

[Krzesimir Nowak]

struct bpf_perf_event_data in kernel headers has the addr field, which
is missing in the tools version of the struct. This will be important
for the bpf prog test run implementation for perf events as it will
expect data to be an instance of struct bpf_perf_event_data, so the
size of the data needs to match sizeof(bpf_perf_event_data).

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/include/uapi/linux/bpf_perf_event.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/include/uapi/linux/bpf_perf_event.h b/tools/include/uapi/linux/bpf_perf_event.h
index 8f95303f9d80..eb1b9d21250c 100644
--- a/tools/include/uapi/linux/bpf_perf_event.h
+++ b/tools/include/uapi/linux/bpf_perf_event.h
@@ -13,6 +13,7 @@
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
 	__u64 sample_period;
+	__u64 addr;
 };
 
 #endif /* _UAPI__LINUX_BPF_PERF_EVENT_H__ */
-- 
2.20.1

Re: [bpf-next v3 08/12] tools headers: Sync struct bpf_perf_event_data

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> struct bpf_perf_event_data in kernel headers has the addr field, which
> is missing in the tools version of the struct. This will be important
> for the bpf prog test run implementation for perf events as it will
> expect data to be an instance of struct bpf_perf_event_data, so the
> size of the data needs to match sizeof(bpf_perf_event_data).
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/include/uapi/linux/bpf_perf_event.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/include/uapi/linux/bpf_perf_event.h b/tools/include/uapi/linux/bpf_perf_event.h
> index 8f95303f9d80..eb1b9d21250c 100644
> --- a/tools/include/uapi/linux/bpf_perf_event.h
> +++ b/tools/include/uapi/linux/bpf_perf_event.h
> @@ -13,6 +13,7 @@
>  struct bpf_perf_event_data {
>         bpf_user_pt_regs_t regs;
>         __u64 sample_period;
> +       __u64 addr;
>  };
>
>  #endif /* _UAPI__LINUX_BPF_PERF_EVENT_H__ */
> --
> 2.20.1
>

[bpf-next v3 09/12] bpf: Split out some helper functions

[Krzesimir Nowak]

The moved functions are generally useful for implementing
bpf_prog_test_run for other types of BPF programs - they don't have
any network-specific stuff in them, so I can use them in a test run
implementation for perf event BPF program too.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 include/linux/bpf.h   |  28 +++++
 kernel/bpf/Makefile   |   1 +
 kernel/bpf/test_run.c | 212 ++++++++++++++++++++++++++++++++++
 net/bpf/test_run.c    | 263 +++++++++++-------------------------------
 4 files changed, 308 insertions(+), 196 deletions(-)
 create mode 100644 kernel/bpf/test_run.c

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 18f4cc2c6acd..28db8ba57bc3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1143,4 +1143,32 @@ static inline u32 bpf_xdp_sock_convert_ctx_access(enum bpf_access_type type,
 }
 #endif /* CONFIG_INET */
 
+/* Helper functions for bpf_prog_test_run implementations */
+typedef u32 bpf_prog_run_helper_t(struct bpf_prog *prog, void *ctx,
+				   void *private_data);
+
+enum bpf_test_run_flags {
+	BPF_TEST_RUN_PLAIN = 0,
+	BPF_TEST_RUN_SETUP_CGROUP_STORAGE = 1 << 0,
+};
+
+int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
+		 u32 *retval, u32 *duration);
+
+int bpf_test_run_cb(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
+		    bpf_prog_run_helper_t run_prog, void *private_data,
+		    u32 *retval, u32 *duration);
+
+int bpf_test_finish(union bpf_attr __user *uattr, u32 retval, u32 duration);
+
+void *bpf_receive_ctx(const union bpf_attr *kattr, u32 max_size);
+
+int bpf_send_ctx(const union bpf_attr *kattr, union bpf_attr __user *uattr,
+		 const void *data, u32 size);
+
+void *bpf_receive_data(const union bpf_attr *kattr, u32 max_size);
+
+int bpf_send_data(const union bpf_attr *kattr, union bpf_attr __user *uattr,
+		  const void *data, u32 size);
+
 #endif /* _LINUX_BPF_H */
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index 29d781061cd5..570fd40288f4 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -22,3 +22,4 @@ obj-$(CONFIG_CGROUP_BPF) += cgroup.o
 ifeq ($(CONFIG_INET),y)
 obj-$(CONFIG_BPF_SYSCALL) += reuseport_array.o
 endif
+obj-$(CONFIG_BPF_SYSCALL) += test_run.o
diff --git a/kernel/bpf/test_run.c b/kernel/bpf/test_run.c
new file mode 100644
index 000000000000..0481373da8be
--- /dev/null
+++ b/kernel/bpf/test_run.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2017 Facebook
+ * Copyright (c) 2019 Tigera, Inc
+ */
+
+#include <asm/div64.h>
+
+#include <linux/bpf-cgroup.h>
+#include <linux/bpf.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/filter.h>
+#include <linux/gfp.h>
+#include <linux/kernel.h>
+#include <linux/limits.h>
+#include <linux/preempt.h>
+#include <linux/rcupdate.h>
+#include <linux/sched.h>
+#include <linux/sched/signal.h>
+#include <linux/slab.h>
+#include <linux/timekeeping.h>
+#include <linux/uaccess.h>
+
+static void teardown_cgroup_storage(struct bpf_cgroup_storage **storage)
+{
+	enum bpf_cgroup_storage_type stype;
+
+	if (!storage)
+		return;
+	for_each_cgroup_storage_type(stype)
+		bpf_cgroup_storage_free(storage[stype]);
+	kfree(storage);
+}
+
+static struct bpf_cgroup_storage **setup_cgroup_storage(struct bpf_prog *prog)
+{
+	enum bpf_cgroup_storage_type stype;
+	struct bpf_cgroup_storage **storage;
+	size_t size = MAX_BPF_CGROUP_STORAGE_TYPE;
+
+	size *= sizeof(struct bpf_cgroup_storage *);
+	storage = kzalloc(size, GFP_KERNEL);
+	for_each_cgroup_storage_type(stype) {
+		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
+		if (IS_ERR(storage[stype])) {
+			storage[stype] = NULL;
+			teardown_cgroup_storage(storage);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+	return storage;
+}
+
+static u32 run_bpf_prog(struct bpf_prog *prog, void *ctx, void *private_data)
+{
+	return BPF_PROG_RUN(prog, ctx);
+}
+
+int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
+		 u32 *retval, u32 *duration)
+{
+	return bpf_test_run_cb(prog, ctx, repeat, flags, run_bpf_prog, NULL,
+			       retval, duration);
+}
+
+int bpf_test_run_cb(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
+		    bpf_prog_run_helper_t run_prog, void *private_data,
+		    u32 *retval, u32 *duration)
+{
+	struct bpf_cgroup_storage **storage = NULL;
+	u64 time_start, time_spent = 0;
+	int ret = 0;
+	u32 i;
+
+	if (flags & BPF_TEST_RUN_SETUP_CGROUP_STORAGE) {
+		storage = setup_cgroup_storage(prog);
+		if (IS_ERR(storage))
+			return PTR_ERR(storage);
+	}
+
+	if (!repeat)
+		repeat = 1;
+
+	rcu_read_lock();
+	preempt_disable();
+	time_start = ktime_get_ns();
+	for (i = 0; i < repeat; i++) {
+		if (storage)
+			bpf_cgroup_storage_set(storage);
+		*retval = run_prog(prog, ctx, private_data);
+
+		if (signal_pending(current)) {
+			preempt_enable();
+			rcu_read_unlock();
+			teardown_cgroup_storage(storage);
+			return -EINTR;
+		}
+
+		if (need_resched()) {
+			time_spent += ktime_get_ns() - time_start;
+			preempt_enable();
+			rcu_read_unlock();
+
+			cond_resched();
+
+			rcu_read_lock();
+			preempt_disable();
+			time_start = ktime_get_ns();
+		}
+	}
+	time_spent += ktime_get_ns() - time_start;
+	preempt_enable();
+	rcu_read_unlock();
+
+	do_div(time_spent, repeat);
+	*duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
+
+	teardown_cgroup_storage(storage);
+
+	return ret;
+}
+
+int bpf_test_finish(union bpf_attr __user *uattr, u32 retval, u32 duration)
+{
+	if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
+		return -EFAULT;
+	if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
+		return -EFAULT;
+	return 0;
+}
+
+static void *bpf_receive_mem(u64 in, u32 in_size, u32 max_size)
+{
+	void __user *data_in = u64_to_user_ptr(in);
+	void *data;
+	int err;
+
+	if (!data_in && in_size)
+		return ERR_PTR(-EINVAL);
+	data = kzalloc(max_size, GFP_USER);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	if (data_in) {
+		err = bpf_check_uarg_tail_zero(data_in, max_size, in_size);
+		if (err) {
+			kfree(data);
+			return ERR_PTR(err);
+		}
+
+		in_size = min_t(u32, max_size, in_size);
+		if (copy_from_user(data, data_in, in_size)) {
+			kfree(data);
+			return ERR_PTR(-EFAULT);
+		}
+	}
+	return data;
+}
+
+static int bpf_send_mem(u64 out, u32 out_size, u32 *out_size_write,
+                        const void *data, u32 data_size)
+{
+	void __user *data_out = u64_to_user_ptr(out);
+	int err = -EFAULT;
+	u32 copy_size = data_size;
+
+	if (!data_out && out_size)
+		return -EINVAL;
+
+	if (!data || !data_out)
+		return 0;
+
+	if (copy_size > out_size) {
+		copy_size = out_size;
+		err = -ENOSPC;
+	}
+
+	if (copy_to_user(data_out, data, copy_size))
+		goto out;
+	if (copy_to_user(out_size_write, &data_size, sizeof(data_size)))
+		goto out;
+	if (err != -ENOSPC)
+		err = 0;
+out:
+	return err;
+}
+
+void *bpf_receive_data(const union bpf_attr *kattr, u32 max_size)
+{
+	return bpf_receive_mem(kattr->test.data_in, kattr->test.data_size_in,
+                               max_size);
+}
+
+int bpf_send_data(const union bpf_attr *kattr, union bpf_attr __user *uattr,
+                  const void *data, u32 size)
+{
+	return bpf_send_mem(kattr->test.data_out, kattr->test.data_size_out,
+			    &uattr->test.data_size_out, data, size);
+}
+
+void *bpf_receive_ctx(const union bpf_attr *kattr, u32 max_size)
+{
+	return bpf_receive_mem(kattr->test.ctx_in, kattr->test.ctx_size_in,
+                               max_size);
+}
+
+int bpf_send_ctx(const union bpf_attr *kattr, union bpf_attr __user *uattr,
+                 const void *data, u32 size)
+{
+        return bpf_send_mem(kattr->test.ctx_out, kattr->test.ctx_size_out,
+                            &uattr->test.ctx_size_out, data, size);
+}
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 80e6f3a6864d..fe6b7b1af0cc 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -14,97 +14,6 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/bpf_test_run.h>
 
-static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
-			u32 *retval, u32 *time)
-{
-	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
-	enum bpf_cgroup_storage_type stype;
-	u64 time_start, time_spent = 0;
-	int ret = 0;
-	u32 i;
-
-	for_each_cgroup_storage_type(stype) {
-		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
-		if (IS_ERR(storage[stype])) {
-			storage[stype] = NULL;
-			for_each_cgroup_storage_type(stype)
-				bpf_cgroup_storage_free(storage[stype]);
-			return -ENOMEM;
-		}
-	}
-
-	if (!repeat)
-		repeat = 1;
-
-	rcu_read_lock();
-	preempt_disable();
-	time_start = ktime_get_ns();
-	for (i = 0; i < repeat; i++) {
-		bpf_cgroup_storage_set(storage);
-		*retval = BPF_PROG_RUN(prog, ctx);
-
-		if (signal_pending(current)) {
-			ret = -EINTR;
-			break;
-		}
-
-		if (need_resched()) {
-			time_spent += ktime_get_ns() - time_start;
-			preempt_enable();
-			rcu_read_unlock();
-
-			cond_resched();
-
-			rcu_read_lock();
-			preempt_disable();
-			time_start = ktime_get_ns();
-		}
-	}
-	time_spent += ktime_get_ns() - time_start;
-	preempt_enable();
-	rcu_read_unlock();
-
-	do_div(time_spent, repeat);
-	*time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
-
-	for_each_cgroup_storage_type(stype)
-		bpf_cgroup_storage_free(storage[stype]);
-
-	return ret;
-}
-
-static int bpf_test_finish(const union bpf_attr *kattr,
-			   union bpf_attr __user *uattr, const void *data,
-			   u32 size, u32 retval, u32 duration)
-{
-	void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
-	int err = -EFAULT;
-	u32 copy_size = size;
-
-	/* Clamp copy if the user has provided a size hint, but copy the full
-	 * buffer if not to retain old behaviour.
-	 */
-	if (kattr->test.data_size_out &&
-	    copy_size > kattr->test.data_size_out) {
-		copy_size = kattr->test.data_size_out;
-		err = -ENOSPC;
-	}
-
-	if (data_out && copy_to_user(data_out, data, copy_size))
-		goto out;
-	if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
-		goto out;
-	if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
-		goto out;
-	if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
-		goto out;
-	if (err != -ENOSPC)
-		err = 0;
-out:
-	trace_bpf_test_finish(&err);
-	return err;
-}
-
 static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
 			   u32 headroom, u32 tailroom)
 {
@@ -125,63 +34,6 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
 	return data;
 }
 
-static void *bpf_ctx_init(const union bpf_attr *kattr, u32 max_size)
-{
-	void __user *data_in = u64_to_user_ptr(kattr->test.ctx_in);
-	void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
-	u32 size = kattr->test.ctx_size_in;
-	void *data;
-	int err;
-
-	if (!data_in && !data_out)
-		return NULL;
-
-	data = kzalloc(max_size, GFP_USER);
-	if (!data)
-		return ERR_PTR(-ENOMEM);
-
-	if (data_in) {
-		err = bpf_check_uarg_tail_zero(data_in, max_size, size);
-		if (err) {
-			kfree(data);
-			return ERR_PTR(err);
-		}
-
-		size = min_t(u32, max_size, size);
-		if (copy_from_user(data, data_in, size)) {
-			kfree(data);
-			return ERR_PTR(-EFAULT);
-		}
-	}
-	return data;
-}
-
-static int bpf_ctx_finish(const union bpf_attr *kattr,
-			  union bpf_attr __user *uattr, const void *data,
-			  u32 size)
-{
-	void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
-	int err = -EFAULT;
-	u32 copy_size = size;
-
-	if (!data || !data_out)
-		return 0;
-
-	if (copy_size > kattr->test.ctx_size_out) {
-		copy_size = kattr->test.ctx_size_out;
-		err = -ENOSPC;
-	}
-
-	if (copy_to_user(data_out, data, copy_size))
-		goto out;
-	if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size)))
-		goto out;
-	if (err != -ENOSPC)
-		err = 0;
-out:
-	return err;
-}
-
 /**
  * range_is_zero - test whether buffer is initialized
  * @buf: buffer to check
@@ -238,6 +90,36 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
 	memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
 }
 
+static int bpf_net_prog_test_run_finish(const union bpf_attr *kattr,
+                                        union bpf_attr __user *uattr,
+                                        const void *data, u32 data_size,
+                                        const void *ctx, u32 ctx_size,
+                                        u32 retval, u32 duration)
+{
+	int ret;
+	union bpf_attr fixed_kattr;
+	const union bpf_attr *kattr_ptr = kattr;
+
+	/* Clamp copy (in bpf_send_mem) if the user has provided a
+	 * size hint, but copy the full buffer if not to retain old
+	 * behaviour.
+	 */
+	if (!kattr->test.data_size_out && kattr->test.data_out) {
+		fixed_kattr = *kattr;
+		fixed_kattr.test.data_size_out = U32_MAX;
+		kattr_ptr = &fixed_kattr;
+	}
+
+	ret = bpf_send_data(kattr_ptr, uattr, data, data_size);
+	if (!ret) {
+		ret = bpf_test_finish(uattr, retval, duration);
+		if (!ret && ctx)
+			ret = bpf_send_ctx(kattr_ptr, uattr, ctx, ctx_size);
+	}
+	trace_bpf_test_finish(&ret);
+	return ret;
+}
+
 int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 			  union bpf_attr __user *uattr)
 {
@@ -257,7 +139,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
-	ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
+	ctx = bpf_receive_ctx(kattr, sizeof(struct __sk_buff));
 	if (IS_ERR(ctx)) {
 		kfree(data);
 		return PTR_ERR(ctx);
@@ -307,7 +189,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	ret = convert___skb_to_skb(skb, ctx);
 	if (ret)
 		goto out;
-	ret = bpf_test_run(prog, skb, repeat, &retval, &duration);
+	ret = bpf_test_run(prog, skb, repeat, BPF_TEST_RUN_SETUP_CGROUP_STORAGE,
+			   &retval, &duration);
 	if (ret)
 		goto out;
 	if (!is_l2) {
@@ -327,10 +210,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	/* bpf program can never convert linear skb to non-linear */
 	if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
 		size = skb_headlen(skb);
-	ret = bpf_test_finish(kattr, uattr, skb->data, size, retval, duration);
-	if (!ret)
-		ret = bpf_ctx_finish(kattr, uattr, ctx,
-				     sizeof(struct __sk_buff));
+	ret = bpf_net_prog_test_run_finish(kattr, uattr, skb->data, size,
+					   ctx, sizeof(struct __sk_buff),
+					   retval, duration);
 out:
 	kfree_skb(skb);
 	bpf_sk_storage_free(sk);
@@ -365,32 +247,48 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
 	rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0);
 	xdp.rxq = &rxqueue->xdp_rxq;
 
-	ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration);
+	ret = bpf_test_run(prog, &xdp, repeat,
+			   BPF_TEST_RUN_SETUP_CGROUP_STORAGE,
+			   &retval, &duration);
 	if (ret)
 		goto out;
 	if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN ||
 	    xdp.data_end != xdp.data + size)
 		size = xdp.data_end - xdp.data;
-	ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration);
+	ret = bpf_net_prog_test_run_finish(kattr, uattr, xdp.data, size,
+					   NULL, 0, retval, duration);
 out:
 	kfree(data);
 	return ret;
 }
 
+struct bpf_flow_dissect_run_data {
+	__be16 proto;
+	int nhoff;
+	int hlen;
+};
+
+static u32 bpf_flow_dissect_run(struct bpf_prog *prog, void *ctx,
+				void *private_data)
+{
+	struct bpf_flow_dissect_run_data *data = private_data;
+
+	return bpf_flow_dissect(prog, ctx, data->proto, data->nhoff, data->hlen);
+}
+
 int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
 				     const union bpf_attr *kattr,
 				     union bpf_attr __user *uattr)
 {
+	struct bpf_flow_dissect_run_data run_data = {};
 	u32 size = kattr->test.data_size_in;
 	struct bpf_flow_dissector ctx = {};
 	u32 repeat = kattr->test.repeat;
 	struct bpf_flow_keys flow_keys;
-	u64 time_start, time_spent = 0;
 	const struct ethhdr *eth;
 	u32 retval, duration;
 	void *data;
 	int ret;
-	u32 i;
 
 	if (prog->type != BPF_PROG_TYPE_FLOW_DISSECTOR)
 		return -EINVAL;
@@ -407,49 +305,22 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
 
 	eth = (struct ethhdr *)data;
 
-	if (!repeat)
-		repeat = 1;
-
 	ctx.flow_keys = &flow_keys;
 	ctx.data = data;
 	ctx.data_end = (__u8 *)data + size;
 
-	rcu_read_lock();
-	preempt_disable();
-	time_start = ktime_get_ns();
-	for (i = 0; i < repeat; i++) {
-		retval = bpf_flow_dissect(prog, &ctx, eth->h_proto, ETH_HLEN,
-					  size);
-
-		if (signal_pending(current)) {
-			preempt_enable();
-			rcu_read_unlock();
-
-			ret = -EINTR;
-			goto out;
-		}
-
-		if (need_resched()) {
-			time_spent += ktime_get_ns() - time_start;
-			preempt_enable();
-			rcu_read_unlock();
-
-			cond_resched();
-
-			rcu_read_lock();
-			preempt_disable();
-			time_start = ktime_get_ns();
-		}
-	}
-	time_spent += ktime_get_ns() - time_start;
-	preempt_enable();
-	rcu_read_unlock();
-
-	do_div(time_spent, repeat);
-	duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
+	run_data.proto = eth->h_proto;
+	run_data.nhoff = ETH_HLEN;
+	run_data.hlen = size;
+	ret = bpf_test_run_cb(prog, &ctx, repeat, BPF_TEST_RUN_PLAIN,
+			      bpf_flow_dissect_run, &run_data,
+			      &retval, &duration);
+	if (!ret)
+		goto out;
 
-	ret = bpf_test_finish(kattr, uattr, &flow_keys, sizeof(flow_keys),
-			      retval, duration);
+	ret = bpf_net_prog_test_run_finish(kattr, uattr, &flow_keys,
+					   sizeof(flow_keys), NULL, 0,
+					   retval, duration);
 
 out:
 	kfree(data);
-- 
2.20.1

Re: [bpf-next v3 09/12] bpf: Split out some helper functions

[Krzesimir Nowak]

On Mon, Jul 8, 2019 at 6:31 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> The moved functions are generally useful for implementing
> bpf_prog_test_run for other types of BPF programs - they don't have
> any network-specific stuff in them, so I can use them in a test run
> implementation for perf event BPF program too.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

This patch has some whitespace issues (indentation with spaces instead
of tabs in some places), will fix them for v4. Sorry about that.

> ---
>  include/linux/bpf.h   |  28 +++++
>  kernel/bpf/Makefile   |   1 +
>  kernel/bpf/test_run.c | 212 ++++++++++++++++++++++++++++++++++
>  net/bpf/test_run.c    | 263 +++++++++++-------------------------------
>  4 files changed, 308 insertions(+), 196 deletions(-)
>  create mode 100644 kernel/bpf/test_run.c
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 18f4cc2c6acd..28db8ba57bc3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1143,4 +1143,32 @@ static inline u32 bpf_xdp_sock_convert_ctx_access(enum bpf_access_type type,
>  }
>  #endif /* CONFIG_INET */
>
> +/* Helper functions for bpf_prog_test_run implementations */
> +typedef u32 bpf_prog_run_helper_t(struct bpf_prog *prog, void *ctx,
> +                                  void *private_data);
> +
> +enum bpf_test_run_flags {
> +       BPF_TEST_RUN_PLAIN = 0,
> +       BPF_TEST_RUN_SETUP_CGROUP_STORAGE = 1 << 0,
> +};
> +
> +int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> +                u32 *retval, u32 *duration);
> +
> +int bpf_test_run_cb(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> +                   bpf_prog_run_helper_t run_prog, void *private_data,
> +                   u32 *retval, u32 *duration);
> +
> +int bpf_test_finish(union bpf_attr __user *uattr, u32 retval, u32 duration);
> +
> +void *bpf_receive_ctx(const union bpf_attr *kattr, u32 max_size);
> +
> +int bpf_send_ctx(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> +                const void *data, u32 size);
> +
> +void *bpf_receive_data(const union bpf_attr *kattr, u32 max_size);
> +
> +int bpf_send_data(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> +                 const void *data, u32 size);
> +
>  #endif /* _LINUX_BPF_H */
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 29d781061cd5..570fd40288f4 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -22,3 +22,4 @@ obj-$(CONFIG_CGROUP_BPF) += cgroup.o
>  ifeq ($(CONFIG_INET),y)
>  obj-$(CONFIG_BPF_SYSCALL) += reuseport_array.o
>  endif
> +obj-$(CONFIG_BPF_SYSCALL) += test_run.o
> diff --git a/kernel/bpf/test_run.c b/kernel/bpf/test_run.c
> new file mode 100644
> index 000000000000..0481373da8be
> --- /dev/null
> +++ b/kernel/bpf/test_run.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2017 Facebook
> + * Copyright (c) 2019 Tigera, Inc
> + */
> +
> +#include <asm/div64.h>
> +
> +#include <linux/bpf-cgroup.h>
> +#include <linux/bpf.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/filter.h>
> +#include <linux/gfp.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/preempt.h>
> +#include <linux/rcupdate.h>
> +#include <linux/sched.h>
> +#include <linux/sched/signal.h>
> +#include <linux/slab.h>
> +#include <linux/timekeeping.h>
> +#include <linux/uaccess.h>
> +
> +static void teardown_cgroup_storage(struct bpf_cgroup_storage **storage)
> +{
> +       enum bpf_cgroup_storage_type stype;
> +
> +       if (!storage)
> +               return;
> +       for_each_cgroup_storage_type(stype)
> +               bpf_cgroup_storage_free(storage[stype]);
> +       kfree(storage);
> +}
> +
> +static struct bpf_cgroup_storage **setup_cgroup_storage(struct bpf_prog *prog)
> +{
> +       enum bpf_cgroup_storage_type stype;
> +       struct bpf_cgroup_storage **storage;
> +       size_t size = MAX_BPF_CGROUP_STORAGE_TYPE;
> +
> +       size *= sizeof(struct bpf_cgroup_storage *);
> +       storage = kzalloc(size, GFP_KERNEL);
> +       for_each_cgroup_storage_type(stype) {
> +               storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
> +               if (IS_ERR(storage[stype])) {
> +                       storage[stype] = NULL;
> +                       teardown_cgroup_storage(storage);
> +                       return ERR_PTR(-ENOMEM);
> +               }
> +       }
> +       return storage;
> +}
> +
> +static u32 run_bpf_prog(struct bpf_prog *prog, void *ctx, void *private_data)
> +{
> +       return BPF_PROG_RUN(prog, ctx);
> +}
> +
> +int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> +                u32 *retval, u32 *duration)
> +{
> +       return bpf_test_run_cb(prog, ctx, repeat, flags, run_bpf_prog, NULL,
> +                              retval, duration);
> +}
> +
> +int bpf_test_run_cb(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> +                   bpf_prog_run_helper_t run_prog, void *private_data,
> +                   u32 *retval, u32 *duration)
> +{
> +       struct bpf_cgroup_storage **storage = NULL;
> +       u64 time_start, time_spent = 0;
> +       int ret = 0;
> +       u32 i;
> +
> +       if (flags & BPF_TEST_RUN_SETUP_CGROUP_STORAGE) {
> +               storage = setup_cgroup_storage(prog);
> +               if (IS_ERR(storage))
> +                       return PTR_ERR(storage);
> +       }
> +
> +       if (!repeat)
> +               repeat = 1;
> +
> +       rcu_read_lock();
> +       preempt_disable();
> +       time_start = ktime_get_ns();
> +       for (i = 0; i < repeat; i++) {
> +               if (storage)
> +                       bpf_cgroup_storage_set(storage);
> +               *retval = run_prog(prog, ctx, private_data);
> +
> +               if (signal_pending(current)) {
> +                       preempt_enable();
> +                       rcu_read_unlock();
> +                       teardown_cgroup_storage(storage);
> +                       return -EINTR;
> +               }
> +
> +               if (need_resched()) {
> +                       time_spent += ktime_get_ns() - time_start;
> +                       preempt_enable();
> +                       rcu_read_unlock();
> +
> +                       cond_resched();
> +
> +                       rcu_read_lock();
> +                       preempt_disable();
> +                       time_start = ktime_get_ns();
> +               }
> +       }
> +       time_spent += ktime_get_ns() - time_start;
> +       preempt_enable();
> +       rcu_read_unlock();
> +
> +       do_div(time_spent, repeat);
> +       *duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> +
> +       teardown_cgroup_storage(storage);
> +
> +       return ret;
> +}
> +
> +int bpf_test_finish(union bpf_attr __user *uattr, u32 retval, u32 duration)
> +{
> +       if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
> +               return -EFAULT;
> +       if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
> +               return -EFAULT;
> +       return 0;
> +}
> +
> +static void *bpf_receive_mem(u64 in, u32 in_size, u32 max_size)
> +{
> +       void __user *data_in = u64_to_user_ptr(in);
> +       void *data;
> +       int err;
> +
> +       if (!data_in && in_size)
> +               return ERR_PTR(-EINVAL);
> +       data = kzalloc(max_size, GFP_USER);
> +       if (!data)
> +               return ERR_PTR(-ENOMEM);
> +
> +       if (data_in) {
> +               err = bpf_check_uarg_tail_zero(data_in, max_size, in_size);
> +               if (err) {
> +                       kfree(data);
> +                       return ERR_PTR(err);
> +               }
> +
> +               in_size = min_t(u32, max_size, in_size);
> +               if (copy_from_user(data, data_in, in_size)) {
> +                       kfree(data);
> +                       return ERR_PTR(-EFAULT);
> +               }
> +       }
> +       return data;
> +}
> +
> +static int bpf_send_mem(u64 out, u32 out_size, u32 *out_size_write,
> +                        const void *data, u32 data_size)
> +{
> +       void __user *data_out = u64_to_user_ptr(out);
> +       int err = -EFAULT;
> +       u32 copy_size = data_size;
> +
> +       if (!data_out && out_size)
> +               return -EINVAL;
> +
> +       if (!data || !data_out)
> +               return 0;
> +
> +       if (copy_size > out_size) {
> +               copy_size = out_size;
> +               err = -ENOSPC;
> +       }
> +
> +       if (copy_to_user(data_out, data, copy_size))
> +               goto out;
> +       if (copy_to_user(out_size_write, &data_size, sizeof(data_size)))
> +               goto out;
> +       if (err != -ENOSPC)
> +               err = 0;
> +out:
> +       return err;
> +}
> +
> +void *bpf_receive_data(const union bpf_attr *kattr, u32 max_size)
> +{
> +       return bpf_receive_mem(kattr->test.data_in, kattr->test.data_size_in,
> +                               max_size);
> +}
> +
> +int bpf_send_data(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> +                  const void *data, u32 size)
> +{
> +       return bpf_send_mem(kattr->test.data_out, kattr->test.data_size_out,
> +                           &uattr->test.data_size_out, data, size);
> +}
> +
> +void *bpf_receive_ctx(const union bpf_attr *kattr, u32 max_size)
> +{
> +       return bpf_receive_mem(kattr->test.ctx_in, kattr->test.ctx_size_in,
> +                               max_size);
> +}
> +
> +int bpf_send_ctx(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> +                 const void *data, u32 size)
> +{
> +        return bpf_send_mem(kattr->test.ctx_out, kattr->test.ctx_size_out,
> +                            &uattr->test.ctx_size_out, data, size);
> +}
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 80e6f3a6864d..fe6b7b1af0cc 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -14,97 +14,6 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/bpf_test_run.h>
>
> -static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
> -                       u32 *retval, u32 *time)
> -{
> -       struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
> -       enum bpf_cgroup_storage_type stype;
> -       u64 time_start, time_spent = 0;
> -       int ret = 0;
> -       u32 i;
> -
> -       for_each_cgroup_storage_type(stype) {
> -               storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
> -               if (IS_ERR(storage[stype])) {
> -                       storage[stype] = NULL;
> -                       for_each_cgroup_storage_type(stype)
> -                               bpf_cgroup_storage_free(storage[stype]);
> -                       return -ENOMEM;
> -               }
> -       }
> -
> -       if (!repeat)
> -               repeat = 1;
> -
> -       rcu_read_lock();
> -       preempt_disable();
> -       time_start = ktime_get_ns();
> -       for (i = 0; i < repeat; i++) {
> -               bpf_cgroup_storage_set(storage);
> -               *retval = BPF_PROG_RUN(prog, ctx);
> -
> -               if (signal_pending(current)) {
> -                       ret = -EINTR;
> -                       break;
> -               }
> -
> -               if (need_resched()) {
> -                       time_spent += ktime_get_ns() - time_start;
> -                       preempt_enable();
> -                       rcu_read_unlock();
> -
> -                       cond_resched();
> -
> -                       rcu_read_lock();
> -                       preempt_disable();
> -                       time_start = ktime_get_ns();
> -               }
> -       }
> -       time_spent += ktime_get_ns() - time_start;
> -       preempt_enable();
> -       rcu_read_unlock();
> -
> -       do_div(time_spent, repeat);
> -       *time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> -
> -       for_each_cgroup_storage_type(stype)
> -               bpf_cgroup_storage_free(storage[stype]);
> -
> -       return ret;
> -}
> -
> -static int bpf_test_finish(const union bpf_attr *kattr,
> -                          union bpf_attr __user *uattr, const void *data,
> -                          u32 size, u32 retval, u32 duration)
> -{
> -       void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
> -       int err = -EFAULT;
> -       u32 copy_size = size;
> -
> -       /* Clamp copy if the user has provided a size hint, but copy the full
> -        * buffer if not to retain old behaviour.
> -        */
> -       if (kattr->test.data_size_out &&
> -           copy_size > kattr->test.data_size_out) {
> -               copy_size = kattr->test.data_size_out;
> -               err = -ENOSPC;
> -       }
> -
> -       if (data_out && copy_to_user(data_out, data, copy_size))
> -               goto out;
> -       if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
> -               goto out;
> -       if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
> -               goto out;
> -       if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
> -               goto out;
> -       if (err != -ENOSPC)
> -               err = 0;
> -out:
> -       trace_bpf_test_finish(&err);
> -       return err;
> -}
> -
>  static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
>                            u32 headroom, u32 tailroom)
>  {
> @@ -125,63 +34,6 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
>         return data;
>  }
>
> -static void *bpf_ctx_init(const union bpf_attr *kattr, u32 max_size)
> -{
> -       void __user *data_in = u64_to_user_ptr(kattr->test.ctx_in);
> -       void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
> -       u32 size = kattr->test.ctx_size_in;
> -       void *data;
> -       int err;
> -
> -       if (!data_in && !data_out)
> -               return NULL;
> -
> -       data = kzalloc(max_size, GFP_USER);
> -       if (!data)
> -               return ERR_PTR(-ENOMEM);
> -
> -       if (data_in) {
> -               err = bpf_check_uarg_tail_zero(data_in, max_size, size);
> -               if (err) {
> -                       kfree(data);
> -                       return ERR_PTR(err);
> -               }
> -
> -               size = min_t(u32, max_size, size);
> -               if (copy_from_user(data, data_in, size)) {
> -                       kfree(data);
> -                       return ERR_PTR(-EFAULT);
> -               }
> -       }
> -       return data;
> -}
> -
> -static int bpf_ctx_finish(const union bpf_attr *kattr,
> -                         union bpf_attr __user *uattr, const void *data,
> -                         u32 size)
> -{
> -       void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
> -       int err = -EFAULT;
> -       u32 copy_size = size;
> -
> -       if (!data || !data_out)
> -               return 0;
> -
> -       if (copy_size > kattr->test.ctx_size_out) {
> -               copy_size = kattr->test.ctx_size_out;
> -               err = -ENOSPC;
> -       }
> -
> -       if (copy_to_user(data_out, data, copy_size))
> -               goto out;
> -       if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size)))
> -               goto out;
> -       if (err != -ENOSPC)
> -               err = 0;
> -out:
> -       return err;
> -}
> -
>  /**
>   * range_is_zero - test whether buffer is initialized
>   * @buf: buffer to check
> @@ -238,6 +90,36 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
>         memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
>  }
>
> +static int bpf_net_prog_test_run_finish(const union bpf_attr *kattr,
> +                                        union bpf_attr __user *uattr,
> +                                        const void *data, u32 data_size,
> +                                        const void *ctx, u32 ctx_size,
> +                                        u32 retval, u32 duration)
> +{
> +       int ret;
> +       union bpf_attr fixed_kattr;
> +       const union bpf_attr *kattr_ptr = kattr;
> +
> +       /* Clamp copy (in bpf_send_mem) if the user has provided a
> +        * size hint, but copy the full buffer if not to retain old
> +        * behaviour.
> +        */
> +       if (!kattr->test.data_size_out && kattr->test.data_out) {
> +               fixed_kattr = *kattr;
> +               fixed_kattr.test.data_size_out = U32_MAX;
> +               kattr_ptr = &fixed_kattr;
> +       }
> +
> +       ret = bpf_send_data(kattr_ptr, uattr, data, data_size);
> +       if (!ret) {
> +               ret = bpf_test_finish(uattr, retval, duration);
> +               if (!ret && ctx)
> +                       ret = bpf_send_ctx(kattr_ptr, uattr, ctx, ctx_size);
> +       }
> +       trace_bpf_test_finish(&ret);
> +       return ret;
> +}
> +
>  int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>                           union bpf_attr __user *uattr)
>  {
> @@ -257,7 +139,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>         if (IS_ERR(data))
>                 return PTR_ERR(data);
>
> -       ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
> +       ctx = bpf_receive_ctx(kattr, sizeof(struct __sk_buff));
>         if (IS_ERR(ctx)) {
>                 kfree(data);
>                 return PTR_ERR(ctx);
> @@ -307,7 +189,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>         ret = convert___skb_to_skb(skb, ctx);
>         if (ret)
>                 goto out;
> -       ret = bpf_test_run(prog, skb, repeat, &retval, &duration);
> +       ret = bpf_test_run(prog, skb, repeat, BPF_TEST_RUN_SETUP_CGROUP_STORAGE,
> +                          &retval, &duration);
>         if (ret)
>                 goto out;
>         if (!is_l2) {
> @@ -327,10 +210,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>         /* bpf program can never convert linear skb to non-linear */
>         if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
>                 size = skb_headlen(skb);
> -       ret = bpf_test_finish(kattr, uattr, skb->data, size, retval, duration);
> -       if (!ret)
> -               ret = bpf_ctx_finish(kattr, uattr, ctx,
> -                                    sizeof(struct __sk_buff));
> +       ret = bpf_net_prog_test_run_finish(kattr, uattr, skb->data, size,
> +                                          ctx, sizeof(struct __sk_buff),
> +                                          retval, duration);
>  out:
>         kfree_skb(skb);
>         bpf_sk_storage_free(sk);
> @@ -365,32 +247,48 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
>         rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0);
>         xdp.rxq = &rxqueue->xdp_rxq;
>
> -       ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration);
> +       ret = bpf_test_run(prog, &xdp, repeat,
> +                          BPF_TEST_RUN_SETUP_CGROUP_STORAGE,
> +                          &retval, &duration);
>         if (ret)
>                 goto out;
>         if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN ||
>             xdp.data_end != xdp.data + size)
>                 size = xdp.data_end - xdp.data;
> -       ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration);
> +       ret = bpf_net_prog_test_run_finish(kattr, uattr, xdp.data, size,
> +                                          NULL, 0, retval, duration);
>  out:
>         kfree(data);
>         return ret;
>  }
>
> +struct bpf_flow_dissect_run_data {
> +       __be16 proto;
> +       int nhoff;
> +       int hlen;
> +};
> +
> +static u32 bpf_flow_dissect_run(struct bpf_prog *prog, void *ctx,
> +                               void *private_data)
> +{
> +       struct bpf_flow_dissect_run_data *data = private_data;
> +
> +       return bpf_flow_dissect(prog, ctx, data->proto, data->nhoff, data->hlen);
> +}
> +
>  int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
>                                      const union bpf_attr *kattr,
>                                      union bpf_attr __user *uattr)
>  {
> +       struct bpf_flow_dissect_run_data run_data = {};
>         u32 size = kattr->test.data_size_in;
>         struct bpf_flow_dissector ctx = {};
>         u32 repeat = kattr->test.repeat;
>         struct bpf_flow_keys flow_keys;
> -       u64 time_start, time_spent = 0;
>         const struct ethhdr *eth;
>         u32 retval, duration;
>         void *data;
>         int ret;
> -       u32 i;
>
>         if (prog->type != BPF_PROG_TYPE_FLOW_DISSECTOR)
>                 return -EINVAL;
> @@ -407,49 +305,22 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
>
>         eth = (struct ethhdr *)data;
>
> -       if (!repeat)
> -               repeat = 1;
> -
>         ctx.flow_keys = &flow_keys;
>         ctx.data = data;
>         ctx.data_end = (__u8 *)data + size;
>
> -       rcu_read_lock();
> -       preempt_disable();
> -       time_start = ktime_get_ns();
> -       for (i = 0; i < repeat; i++) {
> -               retval = bpf_flow_dissect(prog, &ctx, eth->h_proto, ETH_HLEN,
> -                                         size);
> -
> -               if (signal_pending(current)) {
> -                       preempt_enable();
> -                       rcu_read_unlock();
> -
> -                       ret = -EINTR;
> -                       goto out;
> -               }
> -
> -               if (need_resched()) {
> -                       time_spent += ktime_get_ns() - time_start;
> -                       preempt_enable();
> -                       rcu_read_unlock();
> -
> -                       cond_resched();
> -
> -                       rcu_read_lock();
> -                       preempt_disable();
> -                       time_start = ktime_get_ns();
> -               }
> -       }
> -       time_spent += ktime_get_ns() - time_start;
> -       preempt_enable();
> -       rcu_read_unlock();
> -
> -       do_div(time_spent, repeat);
> -       duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> +       run_data.proto = eth->h_proto;
> +       run_data.nhoff = ETH_HLEN;
> +       run_data.hlen = size;
> +       ret = bpf_test_run_cb(prog, &ctx, repeat, BPF_TEST_RUN_PLAIN,
> +                             bpf_flow_dissect_run, &run_data,
> +                             &retval, &duration);
> +       if (!ret)
> +               goto out;
>
> -       ret = bpf_test_finish(kattr, uattr, &flow_keys, sizeof(flow_keys),
> -                             retval, duration);
> +       ret = bpf_net_prog_test_run_finish(kattr, uattr, &flow_keys,
> +                                          sizeof(flow_keys), NULL, 0,
> +                                          retval, duration);
>
>  out:
>         kfree(data);
> --
> 2.20.1
>


-- 
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000

Re: [bpf-next v3 09/12] bpf: Split out some helper functions

[Stanislav Fomichev]

On 07/08, Krzesimir Nowak wrote:
> The moved functions are generally useful for implementing
> bpf_prog_test_run for other types of BPF programs - they don't have
> any network-specific stuff in them, so I can use them in a test run
> implementation for perf event BPF program too.
It's a bit hard to follow. Maybe split into multiple patches?
First one moves the relevant parts as is.
Second one renames (though, I'm not sure we need to rename, but up to
you/maintainers).
Third one removes duplication from bpf_prog_test_run_flow_dissector.

Also see possible suggestion on BPF_TEST_RUN_SETUP_CGROUP_STORAGE below.

> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
>  include/linux/bpf.h   |  28 +++++
>  kernel/bpf/Makefile   |   1 +
>  kernel/bpf/test_run.c | 212 ++++++++++++++++++++++++++++++++++
>  net/bpf/test_run.c    | 263 +++++++++++-------------------------------
>  4 files changed, 308 insertions(+), 196 deletions(-)
>  create mode 100644 kernel/bpf/test_run.c
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 18f4cc2c6acd..28db8ba57bc3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1143,4 +1143,32 @@ static inline u32 bpf_xdp_sock_convert_ctx_access(enum bpf_access_type type,
>  }
>  #endif /* CONFIG_INET */
>  
> +/* Helper functions for bpf_prog_test_run implementations */
> +typedef u32 bpf_prog_run_helper_t(struct bpf_prog *prog, void *ctx,
> +				   void *private_data);
> +
> +enum bpf_test_run_flags {
> +	BPF_TEST_RUN_PLAIN = 0,
> +	BPF_TEST_RUN_SETUP_CGROUP_STORAGE = 1 << 0,
> +};
> +
> +int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> +		 u32 *retval, u32 *duration);
> +
> +int bpf_test_run_cb(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> +		    bpf_prog_run_helper_t run_prog, void *private_data,
> +		    u32 *retval, u32 *duration);
> +
> +int bpf_test_finish(union bpf_attr __user *uattr, u32 retval, u32 duration);
> +
> +void *bpf_receive_ctx(const union bpf_attr *kattr, u32 max_size);
> +
> +int bpf_send_ctx(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> +		 const void *data, u32 size);
> +
> +void *bpf_receive_data(const union bpf_attr *kattr, u32 max_size);
> +
> +int bpf_send_data(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> +		  const void *data, u32 size);
> +
>  #endif /* _LINUX_BPF_H */
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 29d781061cd5..570fd40288f4 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -22,3 +22,4 @@ obj-$(CONFIG_CGROUP_BPF) += cgroup.o
>  ifeq ($(CONFIG_INET),y)
>  obj-$(CONFIG_BPF_SYSCALL) += reuseport_array.o
>  endif
> +obj-$(CONFIG_BPF_SYSCALL) += test_run.o
> diff --git a/kernel/bpf/test_run.c b/kernel/bpf/test_run.c
> new file mode 100644
> index 000000000000..0481373da8be
> --- /dev/null
> +++ b/kernel/bpf/test_run.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2017 Facebook
> + * Copyright (c) 2019 Tigera, Inc
> + */
> +
> +#include <asm/div64.h>
> +
> +#include <linux/bpf-cgroup.h>
> +#include <linux/bpf.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/filter.h>
> +#include <linux/gfp.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/preempt.h>
> +#include <linux/rcupdate.h>
> +#include <linux/sched.h>
> +#include <linux/sched/signal.h>
> +#include <linux/slab.h>
> +#include <linux/timekeeping.h>
> +#include <linux/uaccess.h>
> +
> +static void teardown_cgroup_storage(struct bpf_cgroup_storage **storage)
> +{
> +	enum bpf_cgroup_storage_type stype;
> +
> +	if (!storage)
> +		return;
> +	for_each_cgroup_storage_type(stype)
> +		bpf_cgroup_storage_free(storage[stype]);
> +	kfree(storage);
> +}
> +
> +static struct bpf_cgroup_storage **setup_cgroup_storage(struct bpf_prog *prog)
> +{
> +	enum bpf_cgroup_storage_type stype;
> +	struct bpf_cgroup_storage **storage;
> +	size_t size = MAX_BPF_CGROUP_STORAGE_TYPE;
> +
> +	size *= sizeof(struct bpf_cgroup_storage *);
> +	storage = kzalloc(size, GFP_KERNEL);
> +	for_each_cgroup_storage_type(stype) {
> +		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
> +		if (IS_ERR(storage[stype])) {
> +			storage[stype] = NULL;
> +			teardown_cgroup_storage(storage);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +	return storage;
> +}
> +
> +static u32 run_bpf_prog(struct bpf_prog *prog, void *ctx, void *private_data)
> +{
> +	return BPF_PROG_RUN(prog, ctx);
> +}
> +
> +int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> +		 u32 *retval, u32 *duration)
> +{
> +	return bpf_test_run_cb(prog, ctx, repeat, flags, run_bpf_prog, NULL,
> +			       retval, duration);
> +}
> +
> +int bpf_test_run_cb(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> +		    bpf_prog_run_helper_t run_prog, void *private_data,
> +		    u32 *retval, u32 *duration)
> +{
> +	struct bpf_cgroup_storage **storage = NULL;
> +	u64 time_start, time_spent = 0;
> +	int ret = 0;
> +	u32 i;
> +
> +	if (flags & BPF_TEST_RUN_SETUP_CGROUP_STORAGE) {
You can maybe get away without a flag. prog->aux->cgroup_storage[x] has
a non-zero pointer if verifier found out that the prog is using cgroup
storage. bpf_cgroup_storage_alloc returns NULL is the program doesn't
use cgroup storage. So you should be able to dynamically figure out
whether you need to call bpf_cgroup_storage_set or not.

> +		storage = setup_cgroup_storage(prog);
> +		if (IS_ERR(storage))
> +			return PTR_ERR(storage);
> +	}
> +
> +	if (!repeat)
> +		repeat = 1;
> +
> +	rcu_read_lock();
> +	preempt_disable();
> +	time_start = ktime_get_ns();
> +	for (i = 0; i < repeat; i++) {
> +		if (storage)
> +			bpf_cgroup_storage_set(storage);
> +		*retval = run_prog(prog, ctx, private_data);
> +
> +		if (signal_pending(current)) {
> +			preempt_enable();
> +			rcu_read_unlock();
> +			teardown_cgroup_storage(storage);
> +			return -EINTR;
> +		}
> +
> +		if (need_resched()) {
> +			time_spent += ktime_get_ns() - time_start;
> +			preempt_enable();
> +			rcu_read_unlock();
> +
> +			cond_resched();
> +
> +			rcu_read_lock();
> +			preempt_disable();
> +			time_start = ktime_get_ns();
> +		}
> +	}
> +	time_spent += ktime_get_ns() - time_start;
> +	preempt_enable();
> +	rcu_read_unlock();
> +
> +	do_div(time_spent, repeat);
> +	*duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> +
> +	teardown_cgroup_storage(storage);
> +
> +	return ret;
> +}
> +
> +int bpf_test_finish(union bpf_attr __user *uattr, u32 retval, u32 duration)
> +{
> +	if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
> +		return -EFAULT;
> +	if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static void *bpf_receive_mem(u64 in, u32 in_size, u32 max_size)
> +{
> +	void __user *data_in = u64_to_user_ptr(in);
> +	void *data;
> +	int err;
> +
> +	if (!data_in && in_size)
> +		return ERR_PTR(-EINVAL);
> +	data = kzalloc(max_size, GFP_USER);
> +	if (!data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (data_in) {
> +		err = bpf_check_uarg_tail_zero(data_in, max_size, in_size);
> +		if (err) {
> +			kfree(data);
> +			return ERR_PTR(err);
> +		}
> +
> +		in_size = min_t(u32, max_size, in_size);
> +		if (copy_from_user(data, data_in, in_size)) {
> +			kfree(data);
> +			return ERR_PTR(-EFAULT);
> +		}
> +	}
> +	return data;
> +}
> +
> +static int bpf_send_mem(u64 out, u32 out_size, u32 *out_size_write,
> +                        const void *data, u32 data_size)
> +{
> +	void __user *data_out = u64_to_user_ptr(out);
> +	int err = -EFAULT;
> +	u32 copy_size = data_size;
> +
> +	if (!data_out && out_size)
> +		return -EINVAL;
> +
> +	if (!data || !data_out)
> +		return 0;
> +
> +	if (copy_size > out_size) {
> +		copy_size = out_size;
> +		err = -ENOSPC;
> +	}
> +
> +	if (copy_to_user(data_out, data, copy_size))
> +		goto out;
> +	if (copy_to_user(out_size_write, &data_size, sizeof(data_size)))
> +		goto out;
> +	if (err != -ENOSPC)
> +		err = 0;
> +out:
> +	return err;
> +}
> +
> +void *bpf_receive_data(const union bpf_attr *kattr, u32 max_size)
> +{
> +	return bpf_receive_mem(kattr->test.data_in, kattr->test.data_size_in,
> +                               max_size);
> +}
> +
> +int bpf_send_data(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> +                  const void *data, u32 size)
> +{
> +	return bpf_send_mem(kattr->test.data_out, kattr->test.data_size_out,
> +			    &uattr->test.data_size_out, data, size);
> +}
> +
> +void *bpf_receive_ctx(const union bpf_attr *kattr, u32 max_size)
> +{
> +	return bpf_receive_mem(kattr->test.ctx_in, kattr->test.ctx_size_in,
> +                               max_size);
> +}
> +
> +int bpf_send_ctx(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> +                 const void *data, u32 size)
> +{
> +        return bpf_send_mem(kattr->test.ctx_out, kattr->test.ctx_size_out,
> +                            &uattr->test.ctx_size_out, data, size);
> +}
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 80e6f3a6864d..fe6b7b1af0cc 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -14,97 +14,6 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/bpf_test_run.h>
>  
> -static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
> -			u32 *retval, u32 *time)
> -{
> -	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
> -	enum bpf_cgroup_storage_type stype;
> -	u64 time_start, time_spent = 0;
> -	int ret = 0;
> -	u32 i;
> -
> -	for_each_cgroup_storage_type(stype) {
> -		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
> -		if (IS_ERR(storage[stype])) {
> -			storage[stype] = NULL;
> -			for_each_cgroup_storage_type(stype)
> -				bpf_cgroup_storage_free(storage[stype]);
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	if (!repeat)
> -		repeat = 1;
> -
> -	rcu_read_lock();
> -	preempt_disable();
> -	time_start = ktime_get_ns();
> -	for (i = 0; i < repeat; i++) {
> -		bpf_cgroup_storage_set(storage);
> -		*retval = BPF_PROG_RUN(prog, ctx);
> -
> -		if (signal_pending(current)) {
> -			ret = -EINTR;
> -			break;
> -		}
> -
> -		if (need_resched()) {
> -			time_spent += ktime_get_ns() - time_start;
> -			preempt_enable();
> -			rcu_read_unlock();
> -
> -			cond_resched();
> -
> -			rcu_read_lock();
> -			preempt_disable();
> -			time_start = ktime_get_ns();
> -		}
> -	}
> -	time_spent += ktime_get_ns() - time_start;
> -	preempt_enable();
> -	rcu_read_unlock();
> -
> -	do_div(time_spent, repeat);
> -	*time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> -
> -	for_each_cgroup_storage_type(stype)
> -		bpf_cgroup_storage_free(storage[stype]);
> -
> -	return ret;
> -}
> -
> -static int bpf_test_finish(const union bpf_attr *kattr,
> -			   union bpf_attr __user *uattr, const void *data,
> -			   u32 size, u32 retval, u32 duration)
> -{
> -	void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
> -	int err = -EFAULT;
> -	u32 copy_size = size;
> -
> -	/* Clamp copy if the user has provided a size hint, but copy the full
> -	 * buffer if not to retain old behaviour.
> -	 */
> -	if (kattr->test.data_size_out &&
> -	    copy_size > kattr->test.data_size_out) {
> -		copy_size = kattr->test.data_size_out;
> -		err = -ENOSPC;
> -	}
> -
> -	if (data_out && copy_to_user(data_out, data, copy_size))
> -		goto out;
> -	if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
> -		goto out;
> -	if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
> -		goto out;
> -	if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
> -		goto out;
> -	if (err != -ENOSPC)
> -		err = 0;
> -out:
> -	trace_bpf_test_finish(&err);
> -	return err;
> -}
> -
>  static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
>  			   u32 headroom, u32 tailroom)
>  {
> @@ -125,63 +34,6 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
>  	return data;
>  }
>  
> -static void *bpf_ctx_init(const union bpf_attr *kattr, u32 max_size)
> -{
> -	void __user *data_in = u64_to_user_ptr(kattr->test.ctx_in);
> -	void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
> -	u32 size = kattr->test.ctx_size_in;
> -	void *data;
> -	int err;
> -
> -	if (!data_in && !data_out)
> -		return NULL;
> -
> -	data = kzalloc(max_size, GFP_USER);
> -	if (!data)
> -		return ERR_PTR(-ENOMEM);
> -
> -	if (data_in) {
> -		err = bpf_check_uarg_tail_zero(data_in, max_size, size);
> -		if (err) {
> -			kfree(data);
> -			return ERR_PTR(err);
> -		}
> -
> -		size = min_t(u32, max_size, size);
> -		if (copy_from_user(data, data_in, size)) {
> -			kfree(data);
> -			return ERR_PTR(-EFAULT);
> -		}
> -	}
> -	return data;
> -}
> -
> -static int bpf_ctx_finish(const union bpf_attr *kattr,
> -			  union bpf_attr __user *uattr, const void *data,
> -			  u32 size)
> -{
> -	void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
> -	int err = -EFAULT;
> -	u32 copy_size = size;
> -
> -	if (!data || !data_out)
> -		return 0;
> -
> -	if (copy_size > kattr->test.ctx_size_out) {
> -		copy_size = kattr->test.ctx_size_out;
> -		err = -ENOSPC;
> -	}
> -
> -	if (copy_to_user(data_out, data, copy_size))
> -		goto out;
> -	if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size)))
> -		goto out;
> -	if (err != -ENOSPC)
> -		err = 0;
> -out:
> -	return err;
> -}
> -
>  /**
>   * range_is_zero - test whether buffer is initialized
>   * @buf: buffer to check
> @@ -238,6 +90,36 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
>  	memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
>  }
>  
> +static int bpf_net_prog_test_run_finish(const union bpf_attr *kattr,
> +                                        union bpf_attr __user *uattr,
> +                                        const void *data, u32 data_size,
> +                                        const void *ctx, u32 ctx_size,
> +                                        u32 retval, u32 duration)
> +{
> +	int ret;
> +	union bpf_attr fixed_kattr;
> +	const union bpf_attr *kattr_ptr = kattr;
> +
> +	/* Clamp copy (in bpf_send_mem) if the user has provided a
> +	 * size hint, but copy the full buffer if not to retain old
> +	 * behaviour.
> +	 */
> +	if (!kattr->test.data_size_out && kattr->test.data_out) {
> +		fixed_kattr = *kattr;
> +		fixed_kattr.test.data_size_out = U32_MAX;
> +		kattr_ptr = &fixed_kattr;
> +	}
> +
> +	ret = bpf_send_data(kattr_ptr, uattr, data, data_size);
> +	if (!ret) {
> +		ret = bpf_test_finish(uattr, retval, duration);
> +		if (!ret && ctx)
> +			ret = bpf_send_ctx(kattr_ptr, uattr, ctx, ctx_size);
> +	}
> +	trace_bpf_test_finish(&ret);
> +	return ret;
> +}
> +
>  int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  			  union bpf_attr __user *uattr)
>  {
> @@ -257,7 +139,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  	if (IS_ERR(data))
>  		return PTR_ERR(data);
>  
> -	ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
> +	ctx = bpf_receive_ctx(kattr, sizeof(struct __sk_buff));
>  	if (IS_ERR(ctx)) {
>  		kfree(data);
>  		return PTR_ERR(ctx);
> @@ -307,7 +189,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  	ret = convert___skb_to_skb(skb, ctx);
>  	if (ret)
>  		goto out;
> -	ret = bpf_test_run(prog, skb, repeat, &retval, &duration);
> +	ret = bpf_test_run(prog, skb, repeat, BPF_TEST_RUN_SETUP_CGROUP_STORAGE,
> +			   &retval, &duration);
>  	if (ret)
>  		goto out;
>  	if (!is_l2) {
> @@ -327,10 +210,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  	/* bpf program can never convert linear skb to non-linear */
>  	if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
>  		size = skb_headlen(skb);
> -	ret = bpf_test_finish(kattr, uattr, skb->data, size, retval, duration);
> -	if (!ret)
> -		ret = bpf_ctx_finish(kattr, uattr, ctx,
> -				     sizeof(struct __sk_buff));
> +	ret = bpf_net_prog_test_run_finish(kattr, uattr, skb->data, size,
> +					   ctx, sizeof(struct __sk_buff),
> +					   retval, duration);
>  out:
>  	kfree_skb(skb);
>  	bpf_sk_storage_free(sk);
> @@ -365,32 +247,48 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
>  	rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0);
>  	xdp.rxq = &rxqueue->xdp_rxq;
>  
> -	ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration);
> +	ret = bpf_test_run(prog, &xdp, repeat,
> +			   BPF_TEST_RUN_SETUP_CGROUP_STORAGE,
> +			   &retval, &duration);
>  	if (ret)
>  		goto out;
>  	if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN ||
>  	    xdp.data_end != xdp.data + size)
>  		size = xdp.data_end - xdp.data;
> -	ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration);
> +	ret = bpf_net_prog_test_run_finish(kattr, uattr, xdp.data, size,
> +					   NULL, 0, retval, duration);
>  out:
>  	kfree(data);
>  	return ret;
>  }
>  
> +struct bpf_flow_dissect_run_data {
> +	__be16 proto;
> +	int nhoff;
> +	int hlen;
> +};
> +
> +static u32 bpf_flow_dissect_run(struct bpf_prog *prog, void *ctx,
> +				void *private_data)
> +{
> +	struct bpf_flow_dissect_run_data *data = private_data;
> +
> +	return bpf_flow_dissect(prog, ctx, data->proto, data->nhoff, data->hlen);
> +}
> +
>  int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
>  				     const union bpf_attr *kattr,
>  				     union bpf_attr __user *uattr)
>  {
> +	struct bpf_flow_dissect_run_data run_data = {};
>  	u32 size = kattr->test.data_size_in;
>  	struct bpf_flow_dissector ctx = {};
>  	u32 repeat = kattr->test.repeat;
>  	struct bpf_flow_keys flow_keys;
> -	u64 time_start, time_spent = 0;
>  	const struct ethhdr *eth;
>  	u32 retval, duration;
>  	void *data;
>  	int ret;
> -	u32 i;
>  
>  	if (prog->type != BPF_PROG_TYPE_FLOW_DISSECTOR)
>  		return -EINVAL;
> @@ -407,49 +305,22 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
>  
>  	eth = (struct ethhdr *)data;
>  
> -	if (!repeat)
> -		repeat = 1;
> -
>  	ctx.flow_keys = &flow_keys;
>  	ctx.data = data;
>  	ctx.data_end = (__u8 *)data + size;
>  
> -	rcu_read_lock();
> -	preempt_disable();
> -	time_start = ktime_get_ns();
> -	for (i = 0; i < repeat; i++) {
> -		retval = bpf_flow_dissect(prog, &ctx, eth->h_proto, ETH_HLEN,
> -					  size);
> -
> -		if (signal_pending(current)) {
> -			preempt_enable();
> -			rcu_read_unlock();
> -
> -			ret = -EINTR;
> -			goto out;
> -		}
> -
> -		if (need_resched()) {
> -			time_spent += ktime_get_ns() - time_start;
> -			preempt_enable();
> -			rcu_read_unlock();
> -
> -			cond_resched();
> -
> -			rcu_read_lock();
> -			preempt_disable();
> -			time_start = ktime_get_ns();
> -		}
> -	}
> -	time_spent += ktime_get_ns() - time_start;
> -	preempt_enable();
> -	rcu_read_unlock();
> -
> -	do_div(time_spent, repeat);
> -	duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> +	run_data.proto = eth->h_proto;
> +	run_data.nhoff = ETH_HLEN;
> +	run_data.hlen = size;
> +	ret = bpf_test_run_cb(prog, &ctx, repeat, BPF_TEST_RUN_PLAIN,
> +			      bpf_flow_dissect_run, &run_data,
> +			      &retval, &duration);
> +	if (!ret)
> +		goto out;
>  
> -	ret = bpf_test_finish(kattr, uattr, &flow_keys, sizeof(flow_keys),
> -			      retval, duration);
> +	ret = bpf_net_prog_test_run_finish(kattr, uattr, &flow_keys,
> +					   sizeof(flow_keys), NULL, 0,
> +					   retval, duration);
>  
>  out:
>  	kfree(data);
> -- 
> 2.20.1
> 

[bpf-next v3 10/12] bpf: Implement bpf_prog_test_run for perf event programs

[Krzesimir Nowak]

As an input, test run for perf event program takes struct
bpf_perf_event_data as ctx_in and struct bpf_perf_event_value as
data_in. For an output, it basically ignores ctx_out and data_out.

The implementation sets an instance of struct bpf_perf_event_data_kern
in such a way that the BPF program reading data from context will
receive what we passed to the bpf prog test run in ctx_in. Also BPF
program can call bpf_perf_prog_read_value to receive what was passed
in data_in.

Changes since v2:
- drop the changes in perf event verifier test - they are not needed
  anymore after reworked ctx size handling

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 kernel/trace/bpf_trace.c | 60 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index ca1255d14576..b870fc2314d0 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -19,6 +19,8 @@
 #include "trace_probe.h"
 #include "trace.h"
 
+#include <trace/events/bpf_test_run.h>
+
 #define bpf_event_rcu_dereference(p)					\
 	rcu_dereference_protected(p, lockdep_is_held(&bpf_event_mutex))
 
@@ -1160,7 +1162,65 @@ const struct bpf_verifier_ops perf_event_verifier_ops = {
 	.convert_ctx_access	= pe_prog_convert_ctx_access,
 };
 
+static int pe_prog_test_run(struct bpf_prog *prog,
+			    const union bpf_attr *kattr,
+			    union bpf_attr __user *uattr)
+{
+	struct bpf_perf_event_data_kern real_ctx = {0, };
+	struct perf_sample_data sample_data = {0, };
+	struct bpf_perf_event_data *fake_ctx;
+	struct bpf_perf_event_value *value;
+	struct perf_event event = {0, };
+	u32 retval = 0, duration = 0;
+	int err;
+
+	if (kattr->test.data_size_out || kattr->test.data_out)
+		return -EINVAL;
+	if (kattr->test.ctx_size_out || kattr->test.ctx_out)
+		return -EINVAL;
+
+	fake_ctx = bpf_receive_ctx(kattr, sizeof(struct bpf_perf_event_data));
+	if (IS_ERR(fake_ctx))
+		return PTR_ERR(fake_ctx);
+
+	value = bpf_receive_data(kattr, sizeof(struct bpf_perf_event_value));
+	if (IS_ERR(value)) {
+		kfree(fake_ctx);
+		return PTR_ERR(value);
+	}
+
+	real_ctx.regs = &fake_ctx->regs;
+	real_ctx.data = &sample_data;
+	real_ctx.event = &event;
+	perf_sample_data_init(&sample_data, fake_ctx->addr,
+			      fake_ctx->sample_period);
+	event.cpu = smp_processor_id();
+	event.oncpu = -1;
+	event.state = PERF_EVENT_STATE_OFF;
+	local64_set(&event.count, value->counter);
+	event.total_time_enabled = value->enabled;
+	event.total_time_running = value->running;
+	/* make self as a leader - it is used only for checking the
+	 * state field
+	 */
+	event.group_leader = &event;
+	err = bpf_test_run(prog, &real_ctx, kattr->test.repeat,
+			   BPF_TEST_RUN_PLAIN, &retval, &duration);
+	if (err) {
+		kfree(value);
+		kfree(fake_ctx);
+		return err;
+	}
+
+	err = bpf_test_finish(uattr, retval, duration);
+	trace_bpf_test_finish(&err);
+	kfree(value);
+	kfree(fake_ctx);
+	return err;
+}
+
 const struct bpf_prog_ops perf_event_prog_ops = {
+	.test_run	= pe_prog_test_run,
 };
 
 static DEFINE_MUTEX(bpf_event_mutex);
-- 
2.20.1

Re: [bpf-next v3 10/12] bpf: Implement bpf_prog_test_run for perf event programs

[Stanislav Fomichev]

On 07/08, Krzesimir Nowak wrote:
> As an input, test run for perf event program takes struct
> bpf_perf_event_data as ctx_in and struct bpf_perf_event_value as
> data_in. For an output, it basically ignores ctx_out and data_out.
> 
> The implementation sets an instance of struct bpf_perf_event_data_kern
> in such a way that the BPF program reading data from context will
> receive what we passed to the bpf prog test run in ctx_in. Also BPF
> program can call bpf_perf_prog_read_value to receive what was passed
> in data_in.
> 
> Changes since v2:
> - drop the changes in perf event verifier test - they are not needed
>   anymore after reworked ctx size handling
> 
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
>  kernel/trace/bpf_trace.c | 60 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index ca1255d14576..b870fc2314d0 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -19,6 +19,8 @@
>  #include "trace_probe.h"
>  #include "trace.h"
>  
> +#include <trace/events/bpf_test_run.h>
> +
>  #define bpf_event_rcu_dereference(p)					\
>  	rcu_dereference_protected(p, lockdep_is_held(&bpf_event_mutex))
>  
> @@ -1160,7 +1162,65 @@ const struct bpf_verifier_ops perf_event_verifier_ops = {
>  	.convert_ctx_access	= pe_prog_convert_ctx_access,
>  };
>  
> +static int pe_prog_test_run(struct bpf_prog *prog,
> +			    const union bpf_attr *kattr,
> +			    union bpf_attr __user *uattr)
> +{
> +	struct bpf_perf_event_data_kern real_ctx = {0, };
> +	struct perf_sample_data sample_data = {0, };
> +	struct bpf_perf_event_data *fake_ctx;
> +	struct bpf_perf_event_value *value;
> +	struct perf_event event = {0, };
> +	u32 retval = 0, duration = 0;
> +	int err;
> +
> +	if (kattr->test.data_size_out || kattr->test.data_out)
> +		return -EINVAL;
> +	if (kattr->test.ctx_size_out || kattr->test.ctx_out)
> +		return -EINVAL;
> +
> +	fake_ctx = bpf_receive_ctx(kattr, sizeof(struct bpf_perf_event_data));
> +	if (IS_ERR(fake_ctx))
> +		return PTR_ERR(fake_ctx);
> +
> +	value = bpf_receive_data(kattr, sizeof(struct bpf_perf_event_value));
> +	if (IS_ERR(value)) {
> +		kfree(fake_ctx);
> +		return PTR_ERR(value);
> +	}
nit: maybe use bpf_test_ prefix for receive_ctx/data:
* bpf_test_receive_ctx
* bpf_test_receive_data

? To signify that they are used for tests only.

> +
> +	real_ctx.regs = &fake_ctx->regs;
> +	real_ctx.data = &sample_data;
> +	real_ctx.event = &event;
> +	perf_sample_data_init(&sample_data, fake_ctx->addr,
> +			      fake_ctx->sample_period);
> +	event.cpu = smp_processor_id();
> +	event.oncpu = -1;
> +	event.state = PERF_EVENT_STATE_OFF;
> +	local64_set(&event.count, value->counter);
> +	event.total_time_enabled = value->enabled;
> +	event.total_time_running = value->running;
> +	/* make self as a leader - it is used only for checking the
> +	 * state field
> +	 */
> +	event.group_leader = &event;
> +	err = bpf_test_run(prog, &real_ctx, kattr->test.repeat,
> +			   BPF_TEST_RUN_PLAIN, &retval, &duration);
> +	if (err) {
> +		kfree(value);
> +		kfree(fake_ctx);
> +		return err;
> +	}
> +
> +	err = bpf_test_finish(uattr, retval, duration);
> +	trace_bpf_test_finish(&err);
Can probably do:

	err = bpf_test_run(...)
	if (!err) {
		err = bpf_test_finish(uattr, retval, duration);
		trace_bpf_test_finish(&err);
	}
	kfree(..);
	kfree(..);
	return err;

So you don't have to copy-paste the error handling.

> +	kfree(value);
> +	kfree(fake_ctx);
> +	return err;
> +}
> +
>  const struct bpf_prog_ops perf_event_prog_ops = {
> +	.test_run	= pe_prog_test_run,
>  };
>  
>  static DEFINE_MUTEX(bpf_event_mutex);
> -- 
> 2.20.1
> 

[bpf-next v3 11/12] selftests/bpf: Add tests for bpf_prog_test_run for perf events progs

[Krzesimir Nowak]

The tests check if ctx and data are correctly prepared from ctx_in and
data_in, so accessing the ctx and using the bpf_perf_prog_read_value
work as expected.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/testing/selftests/bpf/test_verifier.c   | 48 ++++++++++
 .../selftests/bpf/verifier/perf_event_run.c   | 96 +++++++++++++++++++
 2 files changed, 144 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 6f124cc4ee34..484ea8842b06 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -295,6 +295,54 @@ static void bpf_fill_scale(struct bpf_test *self)
 	}
 }
 
+static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
+{
+	compiletime_assert(
+		sizeof(struct bpf_perf_event_data) <= TEST_CTX_LEN,
+		"buffer for ctx is too short to fit struct bpf_perf_event_data");
+	compiletime_assert(
+		sizeof(struct bpf_perf_event_value) <= TEST_DATA_LEN,
+		"buffer for data is too short to fit struct bpf_perf_event_value");
+
+	struct bpf_perf_event_data ctx = {
+		.regs = (bpf_user_pt_regs_t) {
+			.r15 = 1,
+			.r14 = 2,
+			.r13 = 3,
+			.r12 = 4,
+			.rbp = 5,
+			.rbx = 6,
+			.r11 = 7,
+			.r10 = 8,
+			.r9 = 9,
+			.r8 = 10,
+			.rax = 11,
+			.rcx = 12,
+			.rdx = 13,
+			.rsi = 14,
+			.rdi = 15,
+			.orig_rax = 16,
+			.rip = 17,
+			.cs = 18,
+			.eflags = 19,
+			.rsp = 20,
+			.ss = 21,
+		},
+		.sample_period = 1,
+		.addr = 2,
+	};
+	struct bpf_perf_event_value data = {
+		.counter = 1,
+		.enabled = 2,
+		.running = 3,
+	};
+
+	memcpy(self->ctx, &ctx, sizeof(ctx));
+	memcpy(self->data, &data, sizeof(data));
+	free(self->fill_insns);
+	self->fill_insns = NULL;
+}
+
 /* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
 #define BPF_SK_LOOKUP(func)						\
 	/* struct bpf_sock_tuple tuple = {} */				\
diff --git a/tools/testing/selftests/bpf/verifier/perf_event_run.c b/tools/testing/selftests/bpf/verifier/perf_event_run.c
new file mode 100644
index 000000000000..3f877458a7f8
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/perf_event_run.c
@@ -0,0 +1,96 @@
+#define PER_LOAD_AND_CHECK_PTREG(PT_REG_FIELD, VALUE)			\
+	PER_LOAD_AND_CHECK_CTX(offsetof(bpf_user_pt_regs_t, PT_REG_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_EVENT(PED_FIELD, VALUE)			\
+	PER_LOAD_AND_CHECK_CTX(offsetof(struct bpf_perf_event_data, PED_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_CTX(OFFSET, VALUE)				\
+	PER_LOAD_AND_CHECK_64(BPF_REG_4, BPF_REG_1, OFFSET, VALUE)
+#define PER_LOAD_AND_CHECK_VALUE(PEV_FIELD, VALUE)			\
+	PER_LOAD_AND_CHECK_64(BPF_REG_7, BPF_REG_6, offsetof(struct bpf_perf_event_value, PEV_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_64(DST, SRC, OFFSET, VALUE)			\
+	BPF_LDX_MEM(BPF_DW, DST, SRC, OFFSET),				\
+	BPF_JMP_IMM(BPF_JEQ, DST, VALUE, 2),				\
+	BPF_MOV64_IMM(BPF_REG_0, VALUE),				\
+	BPF_EXIT_INSN()
+
+{
+	"check if regs contain expected values",
+	.insns = {
+	PER_LOAD_AND_CHECK_PTREG(r15, 1),
+	PER_LOAD_AND_CHECK_PTREG(r14, 2),
+	PER_LOAD_AND_CHECK_PTREG(r13, 3),
+	PER_LOAD_AND_CHECK_PTREG(r12, 4),
+	PER_LOAD_AND_CHECK_PTREG(rbp, 5),
+	PER_LOAD_AND_CHECK_PTREG(rbx, 6),
+	PER_LOAD_AND_CHECK_PTREG(r11, 7),
+	PER_LOAD_AND_CHECK_PTREG(r10, 8),
+	PER_LOAD_AND_CHECK_PTREG(r9, 9),
+	PER_LOAD_AND_CHECK_PTREG(r8, 10),
+	PER_LOAD_AND_CHECK_PTREG(rax, 11),
+	PER_LOAD_AND_CHECK_PTREG(rcx, 12),
+	PER_LOAD_AND_CHECK_PTREG(rdx, 13),
+	PER_LOAD_AND_CHECK_PTREG(rsi, 14),
+	PER_LOAD_AND_CHECK_PTREG(rdi, 15),
+	PER_LOAD_AND_CHECK_PTREG(orig_rax, 16),
+	PER_LOAD_AND_CHECK_PTREG(rip, 17),
+	PER_LOAD_AND_CHECK_PTREG(cs, 18),
+	PER_LOAD_AND_CHECK_PTREG(eflags, 19),
+	PER_LOAD_AND_CHECK_PTREG(rsp, 20),
+	PER_LOAD_AND_CHECK_PTREG(ss, 21),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	.ctx_len = sizeof(struct bpf_perf_event_data),
+	.data_len = sizeof(struct bpf_perf_event_value),
+	.fill_helper = bpf_fill_perf_event_test_run_check,
+	.override_data_out_len = true,
+},
+{
+	"check if sample period and addr contain expected values",
+	.insns = {
+	PER_LOAD_AND_CHECK_EVENT(sample_period, 1),
+	PER_LOAD_AND_CHECK_EVENT(addr, 2),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	.ctx_len = sizeof(struct bpf_perf_event_data),
+	.data_len = sizeof(struct bpf_perf_event_value),
+	.fill_helper = bpf_fill_perf_event_test_run_check,
+	.override_data_out_len = true,
+},
+{
+	"check if bpf_perf_prog_read_value returns expected data",
+	.insns = {
+	// allocate space for a struct bpf_perf_event_value
+	BPF_MOV64_REG(BPF_REG_6, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, -(int)sizeof(struct bpf_perf_event_value)),
+	// prepare parameters for bpf_perf_prog_read_value(ctx, struct bpf_perf_event_value*, u32)
+	// BPF_REG_1 already contains the context
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
+	BPF_MOV64_IMM(BPF_REG_3, sizeof(struct bpf_perf_event_value)),
+	BPF_EMIT_CALL(BPF_FUNC_perf_prog_read_value),
+	// check the return value
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	// check if the fields match the expected values
+	PER_LOAD_AND_CHECK_VALUE(counter, 1),
+	PER_LOAD_AND_CHECK_VALUE(enabled, 2),
+	PER_LOAD_AND_CHECK_VALUE(running, 3),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	.ctx_len = sizeof(struct bpf_perf_event_data),
+	.data_len = sizeof(struct bpf_perf_event_value),
+	.fill_helper = bpf_fill_perf_event_test_run_check,
+	.override_data_out_len = true,
+},
+#undef PER_LOAD_AND_CHECK_64
+#undef PER_LOAD_AND_CHECK_VALUE
+#undef PER_LOAD_AND_CHECK_CTX
+#undef PER_LOAD_AND_CHECK_EVENT
+#undef PER_LOAD_AND_CHECK_PTREG
-- 
2.20.1

Re: [bpf-next v3 11/12] selftests/bpf: Add tests for bpf_prog_test_run for perf events progs

[Andrii Nakryiko]

On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> The tests check if ctx and data are correctly prepared from ctx_in and
> data_in, so accessing the ctx and using the bpf_perf_prog_read_value
> work as expected.
>

These are x86_64-specific tests, aren't they? Should probably guard
them behind #ifdef's.

> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
>  tools/testing/selftests/bpf/test_verifier.c   | 48 ++++++++++
>  .../selftests/bpf/verifier/perf_event_run.c   | 96 +++++++++++++++++++
>  2 files changed, 144 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 6f124cc4ee34..484ea8842b06 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -295,6 +295,54 @@ static void bpf_fill_scale(struct bpf_test *self)
>         }
>  }
>
> +static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
> +{
> +       compiletime_assert(
> +               sizeof(struct bpf_perf_event_data) <= TEST_CTX_LEN,
> +               "buffer for ctx is too short to fit struct bpf_perf_event_data");
> +       compiletime_assert(
> +               sizeof(struct bpf_perf_event_value) <= TEST_DATA_LEN,
> +               "buffer for data is too short to fit struct bpf_perf_event_value");
> +
> +       struct bpf_perf_event_data ctx = {
> +               .regs = (bpf_user_pt_regs_t) {
> +                       .r15 = 1,
> +                       .r14 = 2,
> +                       .r13 = 3,
> +                       .r12 = 4,
> +                       .rbp = 5,
> +                       .rbx = 6,
> +                       .r11 = 7,
> +                       .r10 = 8,
> +                       .r9 = 9,
> +                       .r8 = 10,
> +                       .rax = 11,
> +                       .rcx = 12,
> +                       .rdx = 13,
> +                       .rsi = 14,
> +                       .rdi = 15,
> +                       .orig_rax = 16,
> +                       .rip = 17,
> +                       .cs = 18,
> +                       .eflags = 19,
> +                       .rsp = 20,
> +                       .ss = 21,
> +               },
> +               .sample_period = 1,
> +               .addr = 2,
> +       };
> +       struct bpf_perf_event_value data = {
> +               .counter = 1,
> +               .enabled = 2,
> +               .running = 3,
> +       };
> +
> +       memcpy(self->ctx, &ctx, sizeof(ctx));
> +       memcpy(self->data, &data, sizeof(data));

Just curious, just assignment didn't work?

> +       free(self->fill_insns);
> +       self->fill_insns = NULL;
> +}
> +
>  /* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
>  #define BPF_SK_LOOKUP(func)                                            \
>         /* struct bpf_sock_tuple tuple = {} */                          \
> diff --git a/tools/testing/selftests/bpf/verifier/perf_event_run.c b/tools/testing/selftests/bpf/verifier/perf_event_run.c
> new file mode 100644
> index 000000000000..3f877458a7f8
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/perf_event_run.c
> @@ -0,0 +1,96 @@
> +#define PER_LOAD_AND_CHECK_PTREG(PT_REG_FIELD, VALUE)                  \
> +       PER_LOAD_AND_CHECK_CTX(offsetof(bpf_user_pt_regs_t, PT_REG_FIELD), VALUE)
> +#define PER_LOAD_AND_CHECK_EVENT(PED_FIELD, VALUE)                     \
> +       PER_LOAD_AND_CHECK_CTX(offsetof(struct bpf_perf_event_data, PED_FIELD), VALUE)
> +#define PER_LOAD_AND_CHECK_CTX(OFFSET, VALUE)                          \
> +       PER_LOAD_AND_CHECK_64(BPF_REG_4, BPF_REG_1, OFFSET, VALUE)
> +#define PER_LOAD_AND_CHECK_VALUE(PEV_FIELD, VALUE)                     \
> +       PER_LOAD_AND_CHECK_64(BPF_REG_7, BPF_REG_6, offsetof(struct bpf_perf_event_value, PEV_FIELD), VALUE)

Wrap long lines? Try also running scripts/checkpatch.pl again these
files you are modifying.

> +#define PER_LOAD_AND_CHECK_64(DST, SRC, OFFSET, VALUE)                 \
> +       BPF_LDX_MEM(BPF_DW, DST, SRC, OFFSET),                          \
> +       BPF_JMP_IMM(BPF_JEQ, DST, VALUE, 2),                            \
> +       BPF_MOV64_IMM(BPF_REG_0, VALUE),                                \
> +       BPF_EXIT_INSN()
> +
> +{
> +       "check if regs contain expected values",
> +       .insns = {
> +       PER_LOAD_AND_CHECK_PTREG(r15, 1),
> +       PER_LOAD_AND_CHECK_PTREG(r14, 2),
> +       PER_LOAD_AND_CHECK_PTREG(r13, 3),
> +       PER_LOAD_AND_CHECK_PTREG(r12, 4),
> +       PER_LOAD_AND_CHECK_PTREG(rbp, 5),
> +       PER_LOAD_AND_CHECK_PTREG(rbx, 6),
> +       PER_LOAD_AND_CHECK_PTREG(r11, 7),
> +       PER_LOAD_AND_CHECK_PTREG(r10, 8),
> +       PER_LOAD_AND_CHECK_PTREG(r9, 9),
> +       PER_LOAD_AND_CHECK_PTREG(r8, 10),
> +       PER_LOAD_AND_CHECK_PTREG(rax, 11),
> +       PER_LOAD_AND_CHECK_PTREG(rcx, 12),
> +       PER_LOAD_AND_CHECK_PTREG(rdx, 13),
> +       PER_LOAD_AND_CHECK_PTREG(rsi, 14),
> +       PER_LOAD_AND_CHECK_PTREG(rdi, 15),
> +       PER_LOAD_AND_CHECK_PTREG(orig_rax, 16),
> +       PER_LOAD_AND_CHECK_PTREG(rip, 17),
> +       PER_LOAD_AND_CHECK_PTREG(cs, 18),
> +       PER_LOAD_AND_CHECK_PTREG(eflags, 19),
> +       PER_LOAD_AND_CHECK_PTREG(rsp, 20),
> +       PER_LOAD_AND_CHECK_PTREG(ss, 21),
> +       BPF_MOV64_IMM(BPF_REG_0, 0),
> +       BPF_EXIT_INSN(),
> +       },
> +       .result = ACCEPT,
> +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> +       .ctx_len = sizeof(struct bpf_perf_event_data),
> +       .data_len = sizeof(struct bpf_perf_event_value),
> +       .fill_helper = bpf_fill_perf_event_test_run_check,
> +       .override_data_out_len = true,
> +},
> +{
> +       "check if sample period and addr contain expected values",
> +       .insns = {
> +       PER_LOAD_AND_CHECK_EVENT(sample_period, 1),
> +       PER_LOAD_AND_CHECK_EVENT(addr, 2),
> +       BPF_MOV64_IMM(BPF_REG_0, 0),
> +       BPF_EXIT_INSN(),
> +       },
> +       .result = ACCEPT,
> +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> +       .ctx_len = sizeof(struct bpf_perf_event_data),
> +       .data_len = sizeof(struct bpf_perf_event_value),
> +       .fill_helper = bpf_fill_perf_event_test_run_check,
> +       .override_data_out_len = true,
> +},
> +{
> +       "check if bpf_perf_prog_read_value returns expected data",
> +       .insns = {
> +       // allocate space for a struct bpf_perf_event_value
> +       BPF_MOV64_REG(BPF_REG_6, BPF_REG_10),
> +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, -(int)sizeof(struct bpf_perf_event_value)),
> +       // prepare parameters for bpf_perf_prog_read_value(ctx, struct bpf_perf_event_value*, u32)
> +       // BPF_REG_1 already contains the context
> +       BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
> +       BPF_MOV64_IMM(BPF_REG_3, sizeof(struct bpf_perf_event_value)),
> +       BPF_EMIT_CALL(BPF_FUNC_perf_prog_read_value),
> +       // check the return value
> +       BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
> +       BPF_EXIT_INSN(),
> +       // check if the fields match the expected values

Use /* */ comments.

> +       PER_LOAD_AND_CHECK_VALUE(counter, 1),
> +       PER_LOAD_AND_CHECK_VALUE(enabled, 2),
> +       PER_LOAD_AND_CHECK_VALUE(running, 3),
> +       BPF_MOV64_IMM(BPF_REG_0, 0),
> +       BPF_EXIT_INSN(),
> +       },
> +       .result = ACCEPT,
> +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> +       .ctx_len = sizeof(struct bpf_perf_event_data),
> +       .data_len = sizeof(struct bpf_perf_event_value),
> +       .fill_helper = bpf_fill_perf_event_test_run_check,
> +       .override_data_out_len = true,
> +},
> +#undef PER_LOAD_AND_CHECK_64
> +#undef PER_LOAD_AND_CHECK_VALUE
> +#undef PER_LOAD_AND_CHECK_CTX
> +#undef PER_LOAD_AND_CHECK_EVENT
> +#undef PER_LOAD_AND_CHECK_PTREG
> --
> 2.20.1
>

Re: [bpf-next v3 11/12] selftests/bpf: Add tests for bpf_prog_test_run for perf events progs

[Krzesimir Nowak]

On Fri, Jul 12, 2019 at 2:38 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> >
> > The tests check if ctx and data are correctly prepared from ctx_in and
> > data_in, so accessing the ctx and using the bpf_perf_prog_read_value
> > work as expected.
> >
>
> These are x86_64-specific tests, aren't they? Should probably guard
> them behind #ifdef's.

Yeah, they are x86_64 specific, because pt_regs are arch specific. I
was wondering what to do here in the cover letter. Ifdef? Ifdef and
cover also other arches (please no)? Do some weird tricks with
overriding the definition of pt_regs? Else?

>
> > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > ---
> >  tools/testing/selftests/bpf/test_verifier.c   | 48 ++++++++++
> >  .../selftests/bpf/verifier/perf_event_run.c   | 96 +++++++++++++++++++
> >  2 files changed, 144 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 6f124cc4ee34..484ea8842b06 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -295,6 +295,54 @@ static void bpf_fill_scale(struct bpf_test *self)
> >         }
> >  }
> >
> > +static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
> > +{
> > +       compiletime_assert(
> > +               sizeof(struct bpf_perf_event_data) <= TEST_CTX_LEN,
> > +               "buffer for ctx is too short to fit struct bpf_perf_event_data");
> > +       compiletime_assert(
> > +               sizeof(struct bpf_perf_event_value) <= TEST_DATA_LEN,
> > +               "buffer for data is too short to fit struct bpf_perf_event_value");
> > +
> > +       struct bpf_perf_event_data ctx = {
> > +               .regs = (bpf_user_pt_regs_t) {
> > +                       .r15 = 1,
> > +                       .r14 = 2,
> > +                       .r13 = 3,
> > +                       .r12 = 4,
> > +                       .rbp = 5,
> > +                       .rbx = 6,
> > +                       .r11 = 7,
> > +                       .r10 = 8,
> > +                       .r9 = 9,
> > +                       .r8 = 10,
> > +                       .rax = 11,
> > +                       .rcx = 12,
> > +                       .rdx = 13,
> > +                       .rsi = 14,
> > +                       .rdi = 15,
> > +                       .orig_rax = 16,
> > +                       .rip = 17,
> > +                       .cs = 18,
> > +                       .eflags = 19,
> > +                       .rsp = 20,
> > +                       .ss = 21,
> > +               },
> > +               .sample_period = 1,
> > +               .addr = 2,
> > +       };
> > +       struct bpf_perf_event_value data = {
> > +               .counter = 1,
> > +               .enabled = 2,
> > +               .running = 3,
> > +       };
> > +
> > +       memcpy(self->ctx, &ctx, sizeof(ctx));
> > +       memcpy(self->data, &data, sizeof(data));
>
> Just curious, just assignment didn't work?
>
> > +       free(self->fill_insns);
> > +       self->fill_insns = NULL;
> > +}
> > +
> >  /* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
> >  #define BPF_SK_LOOKUP(func)                                            \
> >         /* struct bpf_sock_tuple tuple = {} */                          \
> > diff --git a/tools/testing/selftests/bpf/verifier/perf_event_run.c b/tools/testing/selftests/bpf/verifier/perf_event_run.c
> > new file mode 100644
> > index 000000000000..3f877458a7f8
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/verifier/perf_event_run.c
> > @@ -0,0 +1,96 @@
> > +#define PER_LOAD_AND_CHECK_PTREG(PT_REG_FIELD, VALUE)                  \
> > +       PER_LOAD_AND_CHECK_CTX(offsetof(bpf_user_pt_regs_t, PT_REG_FIELD), VALUE)
> > +#define PER_LOAD_AND_CHECK_EVENT(PED_FIELD, VALUE)                     \
> > +       PER_LOAD_AND_CHECK_CTX(offsetof(struct bpf_perf_event_data, PED_FIELD), VALUE)
> > +#define PER_LOAD_AND_CHECK_CTX(OFFSET, VALUE)                          \
> > +       PER_LOAD_AND_CHECK_64(BPF_REG_4, BPF_REG_1, OFFSET, VALUE)
> > +#define PER_LOAD_AND_CHECK_VALUE(PEV_FIELD, VALUE)                     \
> > +       PER_LOAD_AND_CHECK_64(BPF_REG_7, BPF_REG_6, offsetof(struct bpf_perf_event_value, PEV_FIELD), VALUE)
>
> Wrap long lines? Try also running scripts/checkpatch.pl again these
> files you are modifying.

Will wrap. Checkpatch was also complaining about complex macro not
being inside parens, but I can't see how to wrap it in parens and keep
it working at the same time.

>
> > +#define PER_LOAD_AND_CHECK_64(DST, SRC, OFFSET, VALUE)                 \
> > +       BPF_LDX_MEM(BPF_DW, DST, SRC, OFFSET),                          \
> > +       BPF_JMP_IMM(BPF_JEQ, DST, VALUE, 2),                            \
> > +       BPF_MOV64_IMM(BPF_REG_0, VALUE),                                \
> > +       BPF_EXIT_INSN()
> > +
> > +{
> > +       "check if regs contain expected values",
> > +       .insns = {
> > +       PER_LOAD_AND_CHECK_PTREG(r15, 1),
> > +       PER_LOAD_AND_CHECK_PTREG(r14, 2),
> > +       PER_LOAD_AND_CHECK_PTREG(r13, 3),
> > +       PER_LOAD_AND_CHECK_PTREG(r12, 4),
> > +       PER_LOAD_AND_CHECK_PTREG(rbp, 5),
> > +       PER_LOAD_AND_CHECK_PTREG(rbx, 6),
> > +       PER_LOAD_AND_CHECK_PTREG(r11, 7),
> > +       PER_LOAD_AND_CHECK_PTREG(r10, 8),
> > +       PER_LOAD_AND_CHECK_PTREG(r9, 9),
> > +       PER_LOAD_AND_CHECK_PTREG(r8, 10),
> > +       PER_LOAD_AND_CHECK_PTREG(rax, 11),
> > +       PER_LOAD_AND_CHECK_PTREG(rcx, 12),
> > +       PER_LOAD_AND_CHECK_PTREG(rdx, 13),
> > +       PER_LOAD_AND_CHECK_PTREG(rsi, 14),
> > +       PER_LOAD_AND_CHECK_PTREG(rdi, 15),
> > +       PER_LOAD_AND_CHECK_PTREG(orig_rax, 16),
> > +       PER_LOAD_AND_CHECK_PTREG(rip, 17),
> > +       PER_LOAD_AND_CHECK_PTREG(cs, 18),
> > +       PER_LOAD_AND_CHECK_PTREG(eflags, 19),
> > +       PER_LOAD_AND_CHECK_PTREG(rsp, 20),
> > +       PER_LOAD_AND_CHECK_PTREG(ss, 21),
> > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > +       BPF_EXIT_INSN(),
> > +       },
> > +       .result = ACCEPT,
> > +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > +       .ctx_len = sizeof(struct bpf_perf_event_data),
> > +       .data_len = sizeof(struct bpf_perf_event_value),
> > +       .fill_helper = bpf_fill_perf_event_test_run_check,
> > +       .override_data_out_len = true,
> > +},
> > +{
> > +       "check if sample period and addr contain expected values",
> > +       .insns = {
> > +       PER_LOAD_AND_CHECK_EVENT(sample_period, 1),
> > +       PER_LOAD_AND_CHECK_EVENT(addr, 2),
> > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > +       BPF_EXIT_INSN(),
> > +       },
> > +       .result = ACCEPT,
> > +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > +       .ctx_len = sizeof(struct bpf_perf_event_data),
> > +       .data_len = sizeof(struct bpf_perf_event_value),
> > +       .fill_helper = bpf_fill_perf_event_test_run_check,
> > +       .override_data_out_len = true,
> > +},
> > +{
> > +       "check if bpf_perf_prog_read_value returns expected data",
> > +       .insns = {
> > +       // allocate space for a struct bpf_perf_event_value
> > +       BPF_MOV64_REG(BPF_REG_6, BPF_REG_10),
> > +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, -(int)sizeof(struct bpf_perf_event_value)),
> > +       // prepare parameters for bpf_perf_prog_read_value(ctx, struct bpf_perf_event_value*, u32)
> > +       // BPF_REG_1 already contains the context
> > +       BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
> > +       BPF_MOV64_IMM(BPF_REG_3, sizeof(struct bpf_perf_event_value)),
> > +       BPF_EMIT_CALL(BPF_FUNC_perf_prog_read_value),
> > +       // check the return value
> > +       BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
> > +       BPF_EXIT_INSN(),
> > +       // check if the fields match the expected values
>
> Use /* */ comments.

Oops. Will fix.

>
> > +       PER_LOAD_AND_CHECK_VALUE(counter, 1),
> > +       PER_LOAD_AND_CHECK_VALUE(enabled, 2),
> > +       PER_LOAD_AND_CHECK_VALUE(running, 3),
> > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > +       BPF_EXIT_INSN(),
> > +       },
> > +       .result = ACCEPT,
> > +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > +       .ctx_len = sizeof(struct bpf_perf_event_data),
> > +       .data_len = sizeof(struct bpf_perf_event_value),
> > +       .fill_helper = bpf_fill_perf_event_test_run_check,
> > +       .override_data_out_len = true,
> > +},
> > +#undef PER_LOAD_AND_CHECK_64
> > +#undef PER_LOAD_AND_CHECK_VALUE
> > +#undef PER_LOAD_AND_CHECK_CTX
> > +#undef PER_LOAD_AND_CHECK_EVENT
> > +#undef PER_LOAD_AND_CHECK_PTREG
> > --
> > 2.20.1
> >



--
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000

Re: [bpf-next v3 11/12] selftests/bpf: Add tests for bpf_prog_test_run for perf events progs

[Andrii Nakryiko]

On Fri, Jul 12, 2019 at 10:37 AM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> On Fri, Jul 12, 2019 at 2:38 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
> > >
> > > The tests check if ctx and data are correctly prepared from ctx_in and
> > > data_in, so accessing the ctx and using the bpf_perf_prog_read_value
> > > work as expected.
> > >
> >
> > These are x86_64-specific tests, aren't they? Should probably guard
> > them behind #ifdef's.
>
> Yeah, they are x86_64 specific, because pt_regs are arch specific. I
> was wondering what to do here in the cover letter. Ifdef? Ifdef and
> cover also other arches (please no)? Do some weird tricks with
> overriding the definition of pt_regs? Else?

So one way to go about this would be to use bpf_helpers.h's
PT_REGS_PARM{1-5} and PT_REGS_RC, which seem to be define for all
"supported" platforms. You won't be testing all possible registers,
but those that are most commonly used by BPF programs (to get input
params and func result) would be tested, which is probably the most
important one. That way your test will be arch-agnostic.

>
> >
> > > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > > ---
> > >  tools/testing/selftests/bpf/test_verifier.c   | 48 ++++++++++
> > >  .../selftests/bpf/verifier/perf_event_run.c   | 96 +++++++++++++++++++
> > >  2 files changed, 144 insertions(+)
> > >  create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > index 6f124cc4ee34..484ea8842b06 100644
> > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > @@ -295,6 +295,54 @@ static void bpf_fill_scale(struct bpf_test *self)
> > >         }
> > >  }
> > >
> > > +static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
> > > +{
> > > +       compiletime_assert(
> > > +               sizeof(struct bpf_perf_event_data) <= TEST_CTX_LEN,
> > > +               "buffer for ctx is too short to fit struct bpf_perf_event_data");
> > > +       compiletime_assert(
> > > +               sizeof(struct bpf_perf_event_value) <= TEST_DATA_LEN,
> > > +               "buffer for data is too short to fit struct bpf_perf_event_value");
> > > +
> > > +       struct bpf_perf_event_data ctx = {
> > > +               .regs = (bpf_user_pt_regs_t) {
> > > +                       .r15 = 1,
> > > +                       .r14 = 2,
> > > +                       .r13 = 3,
> > > +                       .r12 = 4,
> > > +                       .rbp = 5,
> > > +                       .rbx = 6,
> > > +                       .r11 = 7,
> > > +                       .r10 = 8,
> > > +                       .r9 = 9,
> > > +                       .r8 = 10,
> > > +                       .rax = 11,
> > > +                       .rcx = 12,
> > > +                       .rdx = 13,
> > > +                       .rsi = 14,
> > > +                       .rdi = 15,
> > > +                       .orig_rax = 16,
> > > +                       .rip = 17,
> > > +                       .cs = 18,
> > > +                       .eflags = 19,
> > > +                       .rsp = 20,
> > > +                       .ss = 21,
> > > +               },
> > > +               .sample_period = 1,
> > > +               .addr = 2,
> > > +       };
> > > +       struct bpf_perf_event_value data = {
> > > +               .counter = 1,
> > > +               .enabled = 2,
> > > +               .running = 3,
> > > +       };
> > > +
> > > +       memcpy(self->ctx, &ctx, sizeof(ctx));
> > > +       memcpy(self->data, &data, sizeof(data));
> >
> > Just curious, just assignment didn't work?
> >
> > > +       free(self->fill_insns);
> > > +       self->fill_insns = NULL;
> > > +}
> > > +
> > >  /* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
> > >  #define BPF_SK_LOOKUP(func)                                            \
> > >         /* struct bpf_sock_tuple tuple = {} */                          \
> > > diff --git a/tools/testing/selftests/bpf/verifier/perf_event_run.c b/tools/testing/selftests/bpf/verifier/perf_event_run.c
> > > new file mode 100644
> > > index 000000000000..3f877458a7f8
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/verifier/perf_event_run.c
> > > @@ -0,0 +1,96 @@
> > > +#define PER_LOAD_AND_CHECK_PTREG(PT_REG_FIELD, VALUE)                  \
> > > +       PER_LOAD_AND_CHECK_CTX(offsetof(bpf_user_pt_regs_t, PT_REG_FIELD), VALUE)
> > > +#define PER_LOAD_AND_CHECK_EVENT(PED_FIELD, VALUE)                     \
> > > +       PER_LOAD_AND_CHECK_CTX(offsetof(struct bpf_perf_event_data, PED_FIELD), VALUE)
> > > +#define PER_LOAD_AND_CHECK_CTX(OFFSET, VALUE)                          \
> > > +       PER_LOAD_AND_CHECK_64(BPF_REG_4, BPF_REG_1, OFFSET, VALUE)
> > > +#define PER_LOAD_AND_CHECK_VALUE(PEV_FIELD, VALUE)                     \
> > > +       PER_LOAD_AND_CHECK_64(BPF_REG_7, BPF_REG_6, offsetof(struct bpf_perf_event_value, PEV_FIELD), VALUE)
> >
> > Wrap long lines? Try also running scripts/checkpatch.pl again these
> > files you are modifying.
>
> Will wrap. Checkpatch was also complaining about complex macro not
> being inside parens, but I can't see how to wrap it in parens and keep
> it working at the same time.
>
> >
> > > +#define PER_LOAD_AND_CHECK_64(DST, SRC, OFFSET, VALUE)                 \
> > > +       BPF_LDX_MEM(BPF_DW, DST, SRC, OFFSET),                          \
> > > +       BPF_JMP_IMM(BPF_JEQ, DST, VALUE, 2),                            \
> > > +       BPF_MOV64_IMM(BPF_REG_0, VALUE),                                \
> > > +       BPF_EXIT_INSN()
> > > +
> > > +{
> > > +       "check if regs contain expected values",
> > > +       .insns = {
> > > +       PER_LOAD_AND_CHECK_PTREG(r15, 1),
> > > +       PER_LOAD_AND_CHECK_PTREG(r14, 2),
> > > +       PER_LOAD_AND_CHECK_PTREG(r13, 3),
> > > +       PER_LOAD_AND_CHECK_PTREG(r12, 4),
> > > +       PER_LOAD_AND_CHECK_PTREG(rbp, 5),
> > > +       PER_LOAD_AND_CHECK_PTREG(rbx, 6),
> > > +       PER_LOAD_AND_CHECK_PTREG(r11, 7),
> > > +       PER_LOAD_AND_CHECK_PTREG(r10, 8),
> > > +       PER_LOAD_AND_CHECK_PTREG(r9, 9),
> > > +       PER_LOAD_AND_CHECK_PTREG(r8, 10),
> > > +       PER_LOAD_AND_CHECK_PTREG(rax, 11),
> > > +       PER_LOAD_AND_CHECK_PTREG(rcx, 12),
> > > +       PER_LOAD_AND_CHECK_PTREG(rdx, 13),
> > > +       PER_LOAD_AND_CHECK_PTREG(rsi, 14),
> > > +       PER_LOAD_AND_CHECK_PTREG(rdi, 15),
> > > +       PER_LOAD_AND_CHECK_PTREG(orig_rax, 16),
> > > +       PER_LOAD_AND_CHECK_PTREG(rip, 17),
> > > +       PER_LOAD_AND_CHECK_PTREG(cs, 18),
> > > +       PER_LOAD_AND_CHECK_PTREG(eflags, 19),
> > > +       PER_LOAD_AND_CHECK_PTREG(rsp, 20),
> > > +       PER_LOAD_AND_CHECK_PTREG(ss, 21),
> > > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > > +       BPF_EXIT_INSN(),
> > > +       },
> > > +       .result = ACCEPT,
> > > +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > +       .ctx_len = sizeof(struct bpf_perf_event_data),
> > > +       .data_len = sizeof(struct bpf_perf_event_value),
> > > +       .fill_helper = bpf_fill_perf_event_test_run_check,
> > > +       .override_data_out_len = true,
> > > +},
> > > +{
> > > +       "check if sample period and addr contain expected values",
> > > +       .insns = {
> > > +       PER_LOAD_AND_CHECK_EVENT(sample_period, 1),
> > > +       PER_LOAD_AND_CHECK_EVENT(addr, 2),
> > > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > > +       BPF_EXIT_INSN(),
> > > +       },
> > > +       .result = ACCEPT,
> > > +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > +       .ctx_len = sizeof(struct bpf_perf_event_data),
> > > +       .data_len = sizeof(struct bpf_perf_event_value),
> > > +       .fill_helper = bpf_fill_perf_event_test_run_check,
> > > +       .override_data_out_len = true,
> > > +},
> > > +{
> > > +       "check if bpf_perf_prog_read_value returns expected data",
> > > +       .insns = {
> > > +       // allocate space for a struct bpf_perf_event_value
> > > +       BPF_MOV64_REG(BPF_REG_6, BPF_REG_10),
> > > +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, -(int)sizeof(struct bpf_perf_event_value)),
> > > +       // prepare parameters for bpf_perf_prog_read_value(ctx, struct bpf_perf_event_value*, u32)
> > > +       // BPF_REG_1 already contains the context
> > > +       BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
> > > +       BPF_MOV64_IMM(BPF_REG_3, sizeof(struct bpf_perf_event_value)),
> > > +       BPF_EMIT_CALL(BPF_FUNC_perf_prog_read_value),
> > > +       // check the return value
> > > +       BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
> > > +       BPF_EXIT_INSN(),
> > > +       // check if the fields match the expected values
> >
> > Use /* */ comments.
>
> Oops. Will fix.
>
> >
> > > +       PER_LOAD_AND_CHECK_VALUE(counter, 1),
> > > +       PER_LOAD_AND_CHECK_VALUE(enabled, 2),
> > > +       PER_LOAD_AND_CHECK_VALUE(running, 3),
> > > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > > +       BPF_EXIT_INSN(),
> > > +       },
> > > +       .result = ACCEPT,
> > > +       .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > +       .ctx_len = sizeof(struct bpf_perf_event_data),
> > > +       .data_len = sizeof(struct bpf_perf_event_value),
> > > +       .fill_helper = bpf_fill_perf_event_test_run_check,
> > > +       .override_data_out_len = true,
> > > +},
> > > +#undef PER_LOAD_AND_CHECK_64
> > > +#undef PER_LOAD_AND_CHECK_VALUE
> > > +#undef PER_LOAD_AND_CHECK_CTX
> > > +#undef PER_LOAD_AND_CHECK_EVENT
> > > +#undef PER_LOAD_AND_CHECK_PTREG
> > > --
> > > 2.20.1
> > >
>
>
>
> --
> Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
> Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
> Registergericht/Court of registration: Amtsgericht Charlottenburg
> Registernummer/Registration number: HRB 171414 B
> Ust-ID-Nummer/VAT ID number: DE302207000

[bpf-next v3 12/12] selftests/bpf: Test correctness of narrow 32bit read on 64bit field

[Krzesimir Nowak]

Test the correctness of the 32bit narrow reads by reading both halves
of the 64 bit field and doing a binary or on them to see if we get the
original value.

It succeeds as it should, but with the commit e2f7fc0ac695 ("bpf: fix
undefined behavior in narrow load handling") reverted, the test fails
with a following message:

> $ sudo ./test_verifier
> ...
> #967/p 32bit loads of a 64bit field (both least and most significant words) FAIL retval -1985229329 != 0
> verification time 17 usec
> stack depth 0
> processed 8 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> ...
> Summary: 1519 PASSED, 0 SKIPPED, 1 FAILED

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
 tools/testing/selftests/bpf/test_verifier.c   | 19 +++++++++++++++++
 .../testing/selftests/bpf/verifier/var_off.c  | 21 +++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 484ea8842b06..2a20280a4a44 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -24,6 +24,7 @@
 
 #include <sys/capability.h>
 
+#include <linux/compiler.h>
 #include <linux/unistd.h>
 #include <linux/filter.h>
 #include <linux/bpf_perf_event.h>
@@ -343,6 +344,24 @@ static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
 	self->fill_insns = NULL;
 }
 
+static void bpf_fill_32bit_loads(struct bpf_test *self)
+{
+	compiletime_assert(
+		sizeof(struct bpf_perf_event_data) <= TEST_CTX_LEN,
+		"buffer for ctx is too short to fit struct bpf_perf_event_data");
+	compiletime_assert(
+		sizeof(struct bpf_perf_event_value) <= TEST_DATA_LEN,
+		"buffer for data is too short to fit struct bpf_perf_event_value");
+
+	struct bpf_perf_event_data ctx = {
+		.sample_period = 0x0123456789abcdef,
+	};
+
+	memcpy(self->ctx, &ctx, sizeof(ctx));
+	free(self->fill_insns);
+	self->fill_insns = NULL;
+}
+
 /* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
 #define BPF_SK_LOOKUP(func)						\
 	/* struct bpf_sock_tuple tuple = {} */				\
diff --git a/tools/testing/selftests/bpf/verifier/var_off.c b/tools/testing/selftests/bpf/verifier/var_off.c
index 8504ac937809..3f8bee0a50ef 100644
--- a/tools/testing/selftests/bpf/verifier/var_off.c
+++ b/tools/testing/selftests/bpf/verifier/var_off.c
@@ -246,3 +246,24 @@
 	.result = ACCEPT,
 	.prog_type = BPF_PROG_TYPE_LWT_IN,
 },
+{
+	"32bit loads of a 64bit field (both least and most significant words)",
+	.insns = {
+	BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period)),
+	BPF_LDX_MEM(BPF_W, BPF_REG_5, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period) + 4),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period)),
+	BPF_ALU64_IMM(BPF_LSH, BPF_REG_5, 32),
+	BPF_ALU64_REG(BPF_OR, BPF_REG_4, BPF_REG_5),
+	BPF_ALU64_REG(BPF_XOR, BPF_REG_4, BPF_REG_6),
+	BPF_MOV64_REG(BPF_REG_0, BPF_REG_4),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	.ctx = { 0, },
+	.ctx_len = sizeof(struct bpf_perf_event_data),
+	.data = { 0, },
+	.data_len = sizeof(struct bpf_perf_event_value),
+	.fill_helper = bpf_fill_32bit_loads,
+	.override_data_out_len = true,
+},
-- 
2.20.1