mbox series

[0/2] Improve error handling of verifier tests

Message ID 20201128192502.88195-1-dev@der-flo.net
Headers show
Series Improve error handling of verifier tests | expand

Message

Florian Lehner Nov. 28, 2020, 7:25 p.m. UTC
These patches improve the error handling for verifier tests. With "Test
the 32bit narrow read" Krzesimir Nowak provided these patches first, but
they were never merged.
The improved error handling helps to implement and test BPF program types
that are not supported yet.

Florian Lehner (2):
  selftests/bpf: Avoid errno clobbering
  selftests/bpf: Print reason when a tester could not run a program

 tools/testing/selftests/bpf/test_verifier.c | 28 ++++++++++++++++-----
 1 file changed, 22 insertions(+), 6 deletions(-)

Comments

Andrii Nakryiko Dec. 2, 2020, 1:23 a.m. UTC | #1
On Sat, Nov 28, 2020 at 11:29 AM Florian Lehner <dev@der-flo.net> wrote:
>

> Print a message when the returned error is about a program type being

> not supported or because of permission problems.

> These messages are expected if the program to test was actually

> executed.

>

> Cc: Krzesimir Nowak <krzesimir@kinvolk.io>

> Signed-off-by: Florian Lehner <dev@der-flo.net>

> ---

>  tools/testing/selftests/bpf/test_verifier.c | 24 ++++++++++++++++-----

>  1 file changed, 19 insertions(+), 5 deletions(-)

>

> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c

> index ceea9409639e..bd95894b7ea0 100644

> --- a/tools/testing/selftests/bpf/test_verifier.c

> +++ b/tools/testing/selftests/bpf/test_verifier.c

> @@ -875,19 +875,33 @@ 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 err;

> +       int err, saved_errno;

>

>         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 && errno != 524/*ENOTSUPP*/ && errno != EPERM) {

> -               printf("Unexpected bpf_prog_test_run error ");

> -               return err;

> +

> +       if (err) {

> +               switch (errno) {


nit: stick to using saved_errno consistently, set_admin() does a lot
of things that can change 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;


This should be ok to ignore *only* in unpriv mode, no?

> +               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.28.0

>