bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf v1 0/3] Test the 32bit narrow reads
@ 2019-05-15 13:47 Krzesimir Nowak
  2019-05-15 13:47 ` [PATCH bpf v1 1/3] selftests/bpf: Test correctness of narrow 32bit read on 64bit field Krzesimir Nowak
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Krzesimir Nowak @ 2019-05-15 13:47 UTC (permalink / raw)
  To: bpf
  Cc: iago, alban, Krzesimir Nowak, Shuah Khan, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrey Ignatov, Jiong Wang, Jakub Kicinski, linux-kselftest,
	netdev, linux-kernel

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 is not supported by bpf_prog_test_run().

First commit adds the test, but since I can't run it, I'm not sure if
it is even valid (because endianness, for example). Second commit adds
a message to test_verifier when it couldn't run the program because of
lack permissions or program type being not supported. Third commit
fixes a possible problem with errno.

With these patches, I get the following output on my test:

/kernel/tools/testing/selftests/bpf$ sudo ./test_verifier 920 920
#920/p 32bit loads of a 64bit field (both least and most significant words) Did not run the program (not supported) OK
Summary: 1 PASSED, 0 SKIPPED, 0 FAILED

So it looks like I need to pick a different approach.

Krzesimir Nowak (3):
  selftests/bpf: Test correctness of narrow 32bit read on 64bit field
  selftests/bpf: Print a message when tester could not run a program
  selftests/bpf: Avoid a clobbering of errno

 tools/testing/selftests/bpf/test_verifier.c   | 19 +++++++++++++++----
 .../testing/selftests/bpf/verifier/var_off.c  | 15 +++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH bpf v1 1/3] selftests/bpf: Test correctness of narrow 32bit read on 64bit field
  2019-05-15 13:47 [PATCH bpf v1 0/3] Test the 32bit narrow reads Krzesimir Nowak
@ 2019-05-15 13:47 ` Krzesimir Nowak
  2019-05-23 16:02   ` Daniel Borkmann
  2019-05-15 13:47 ` [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program Krzesimir Nowak
  2019-05-15 13:47 ` [PATCH bpf v1 3/3] selftests/bpf: Avoid a clobbering of errno Krzesimir Nowak
  2 siblings, 1 reply; 12+ messages in thread
From: Krzesimir Nowak @ 2019-05-15 13:47 UTC (permalink / raw)
  To: bpf
  Cc: iago, alban, Krzesimir Nowak, Shuah Khan, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrey Ignatov, Jiong Wang, Jakub Kicinski, linux-kselftest,
	netdev, linux-kernel

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.

This isn't really tested - the program is not being run, because
BPF_PROG_TYPE_PERF_EVENT is not supported by bpf_test_run_prog.

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

diff --git a/tools/testing/selftests/bpf/verifier/var_off.c b/tools/testing/selftests/bpf/verifier/var_off.c
index 8504ac937809..2668819dcc85 100644
--- a/tools/testing/selftests/bpf/verifier/var_off.c
+++ b/tools/testing/selftests/bpf/verifier/var_off.c
@@ -246,3 +246,18 @@
 	.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_4, 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,
+},
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program
  2019-05-15 13:47 [PATCH bpf v1 0/3] Test the 32bit narrow reads Krzesimir Nowak
  2019-05-15 13:47 ` [PATCH bpf v1 1/3] selftests/bpf: Test correctness of narrow 32bit read on 64bit field Krzesimir Nowak
@ 2019-05-15 13:47 ` Krzesimir Nowak
  2019-05-15 21:45   ` Jakub Kicinski
  2019-05-15 13:47 ` [PATCH bpf v1 3/3] selftests/bpf: Avoid a clobbering of errno Krzesimir Nowak
  2 siblings, 1 reply; 12+ messages in thread
From: Krzesimir Nowak @ 2019-05-15 13:47 UTC (permalink / raw)
  To: bpf
  Cc: iago, alban, Krzesimir Nowak, Shuah Khan, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrey Ignatov, Jakub Kicinski, linux-kselftest, netdev,
	linux-kernel

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".

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 ccd896b98cac..bf0da03f593b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -825,11 +825,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("Unexpected bpf_prog_test_run error ");
+			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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH bpf v1 3/3] selftests/bpf: Avoid a clobbering of errno
  2019-05-15 13:47 [PATCH bpf v1 0/3] Test the 32bit narrow reads Krzesimir Nowak
  2019-05-15 13:47 ` [PATCH bpf v1 1/3] selftests/bpf: Test correctness of narrow 32bit read on 64bit field Krzesimir Nowak
  2019-05-15 13:47 ` [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program Krzesimir Nowak
@ 2019-05-15 13:47 ` Krzesimir Nowak
  2019-05-15 21:50   ` Jakub Kicinski
  2 siblings, 1 reply; 12+ messages in thread
From: Krzesimir Nowak @ 2019-05-15 13:47 UTC (permalink / raw)
  To: bpf
  Cc: iago, alban, Krzesimir Nowak, Jakub Kicinski, Shuah Khan,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrey Ignatov, Jiong Wang, linux-kselftest,
	netdev, linux-kernel

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.

Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
Fixes: 5a8d5209ac022 ("selftests: bpf: add trivial JSET tests")
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 bf0da03f593b..514e17246396 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -818,15 +818,17 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
 	__u32 size_tmp = sizeof(tmp);
 	uint32_t retval;
 	int err;
+	int 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) {
-		switch (errno) {
+		switch (saved_errno) {
 		case 524/*ENOTSUPP*/:
 			printf("Did not run the program (not supported) ");
 			return 0;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program
  2019-05-15 13:47 ` [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program Krzesimir Nowak
@ 2019-05-15 21:45   ` Jakub Kicinski
  2019-05-16  9:29     ` Krzesimir Nowak
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2019-05-15 21:45 UTC (permalink / raw)
  To: Krzesimir Nowak
  Cc: bpf, iago, alban, Shuah Khan, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrey Ignatov, linux-kselftest, netdev, linux-kernel

On Wed, 15 May 2019 15:47:27 +0200, Krzesimir Nowak 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".
> 
> 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 ccd896b98cac..bf0da03f593b 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -825,11 +825,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;

Perhaps use strerror(errno)?

> +		default:
> +			printf("Unexpected bpf_prog_test_run error ");
> +			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;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf v1 3/3] selftests/bpf: Avoid a clobbering of errno
  2019-05-15 13:47 ` [PATCH bpf v1 3/3] selftests/bpf: Avoid a clobbering of errno Krzesimir Nowak
@ 2019-05-15 21:50   ` Jakub Kicinski
  2019-05-16  9:31     ` Krzesimir Nowak
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2019-05-15 21:50 UTC (permalink / raw)
  To: Krzesimir Nowak
  Cc: bpf, iago, alban, Shuah Khan, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrey Ignatov, Jiong Wang, linux-kselftest, netdev,
	linux-kernel

On Wed, 15 May 2019 15:47:28 +0200, Krzesimir Nowak 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.
> 
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> Fixes: 5a8d5209ac022 ("selftests: bpf: add trivial JSET tests")

This commit (of mine) just moved this code into a helper, the bug is
older:

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 bf0da03f593b..514e17246396 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -818,15 +818,17 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
>  	__u32 size_tmp = sizeof(tmp);
>  	uint32_t retval;
>  	int err;
> +	int 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) {
> -		switch (errno) {
> +		switch (saved_errno) {
>  		case 524/*ENOTSUPP*/:
>  			printf("Did not run the program (not supported) ");
>  			return 0;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program
  2019-05-15 21:45   ` Jakub Kicinski
@ 2019-05-16  9:29     ` Krzesimir Nowak
  2019-05-16 15:50       ` Jakub Kicinski
  0 siblings, 1 reply; 12+ messages in thread
From: Krzesimir Nowak @ 2019-05-16  9:29 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: bpf, Iago López Galeiras, Alban Crequy (Kinvolk),
	Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrey Ignatov,
	linux-kselftest, netdev, linux-kernel

On Wed, May 15, 2019 at 11:46 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 15 May 2019 15:47:27 +0200, Krzesimir Nowak 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".
> >
> > 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 ccd896b98cac..bf0da03f593b 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -825,11 +825,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;
>
> Perhaps use strerror(errno)?

As I said in the commit message, I open-coded those messages because
strerror for ENOTSUPP returns "Unknown error 524".

>
> > +             default:
> > +                     printf("Unexpected bpf_prog_test_run error ");
> > +                     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;
>


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf v1 3/3] selftests/bpf: Avoid a clobbering of errno
  2019-05-15 21:50   ` Jakub Kicinski
@ 2019-05-16  9:31     ` Krzesimir Nowak
  0 siblings, 0 replies; 12+ messages in thread
From: Krzesimir Nowak @ 2019-05-16  9:31 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: bpf, Iago López Galeiras, Alban Crequy (Kinvolk),
	Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrey Ignatov,
	Jiong Wang, linux-kselftest, netdev, linux-kernel

On Wed, May 15, 2019 at 11:51 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 15 May 2019 15:47:28 +0200, Krzesimir Nowak 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.
> >
> > Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Fixes: 5a8d5209ac022 ("selftests: bpf: add trivial JSET tests")
>
> This commit (of mine) just moved this code into a helper, the bug is
> older:
>
> Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")

Oops, ok. Will fix it. Thanks.

>
> > 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 bf0da03f593b..514e17246396 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -818,15 +818,17 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> >       __u32 size_tmp = sizeof(tmp);
> >       uint32_t retval;
> >       int err;
> > +     int 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) {
> > -             switch (errno) {
> > +             switch (saved_errno) {
> >               case 524/*ENOTSUPP*/:
> >                       printf("Did not run the program (not supported) ");
> >                       return 0;
>


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program
  2019-05-16  9:29     ` Krzesimir Nowak
@ 2019-05-16 15:50       ` Jakub Kicinski
  2019-05-16 16:21         ` Krzesimir Nowak
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2019-05-16 15:50 UTC (permalink / raw)
  To: Krzesimir Nowak
  Cc: bpf, Iago López Galeiras, Alban Crequy (Kinvolk),
	Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrey Ignatov,
	linux-kselftest, netdev, linux-kernel

On Thu, 16 May 2019 11:29:39 +0200, Krzesimir Nowak wrote:
> > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > index ccd896b98cac..bf0da03f593b 100644
> > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > @@ -825,11 +825,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;  
> >
> > Perhaps use strerror(errno)?  
> 
> As I said in the commit message, I open-coded those messages because
> strerror for ENOTSUPP returns "Unknown error 524".

Ah, sorry, missed that.  I wonder if that's something worth addressing
in libc, since the BPF subsystem uses ENOTSUPP a lot.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program
  2019-05-16 15:50       ` Jakub Kicinski
@ 2019-05-16 16:21         ` Krzesimir Nowak
  2019-05-16 16:54           ` Jakub Kicinski
  0 siblings, 1 reply; 12+ messages in thread
From: Krzesimir Nowak @ 2019-05-16 16:21 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: bpf, Iago López Galeiras, Alban Crequy (Kinvolk),
	Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrey Ignatov,
	linux-kselftest, netdev, linux-kernel

On Thu, May 16, 2019 at 5:51 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 16 May 2019 11:29:39 +0200, Krzesimir Nowak wrote:
> > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > > index ccd896b98cac..bf0da03f593b 100644
> > > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > > @@ -825,11 +825,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;
> > >
> > > Perhaps use strerror(errno)?
> >
> > As I said in the commit message, I open-coded those messages because
> > strerror for ENOTSUPP returns "Unknown error 524".
>
> Ah, sorry, missed that.  I wonder if that's something worth addressing
> in libc, since the BPF subsystem uses ENOTSUPP a lot.

The "not supported" errno situation seems to be a mess. There is an
ENOTSUP define in libc. ENOTSUP is usually defined to be EOPNOTSUPP
(taken from kernel), which in turn seems to have a different value
(95) than kernel's ENOTSUPP (524). Adding ENOTSUPP (with two Ps) to
libc would only add to the confusion. So it's kind of meh and I guess
people just moved on with workarounds.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program
  2019-05-16 16:21         ` Krzesimir Nowak
@ 2019-05-16 16:54           ` Jakub Kicinski
  0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2019-05-16 16:54 UTC (permalink / raw)
  To: Krzesimir Nowak
  Cc: bpf, Iago López Galeiras, Alban Crequy (Kinvolk),
	Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrey Ignatov,
	linux-kselftest, netdev, linux-kernel, ogerlitz

On Thu, 16 May 2019 18:21:32 +0200, Krzesimir Nowak wrote:
> On Thu, May 16, 2019 at 5:51 PM Jakub Kicinski wrote:
> > On Thu, 16 May 2019 11:29:39 +0200, Krzesimir Nowak wrote:  
> > > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > > > index ccd896b98cac..bf0da03f593b 100644
> > > > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > > > @@ -825,11 +825,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;  
> > > >
> > > > Perhaps use strerror(errno)?  
> > >
> > > As I said in the commit message, I open-coded those messages because
> > > strerror for ENOTSUPP returns "Unknown error 524".  
> >
> > Ah, sorry, missed that.  I wonder if that's something worth addressing
> > in libc, since the BPF subsystem uses ENOTSUPP a lot.  
> 
> The "not supported" errno situation seems to be a mess. There is an
> ENOTSUP define in libc. ENOTSUP is usually defined to be EOPNOTSUPP
> (taken from kernel), which in turn seems to have a different value
> (95) than kernel's ENOTSUPP (524). Adding ENOTSUPP (with two Ps) to
> libc would only add to the confusion. So it's kind of meh and I guess
> people just moved on with workarounds.

Yes, ENOTSUP is never used in the kernel, but it's a mess.

This commit a while ago said ENOTSUPP is from NFS:

commit 423b3aecf29085a52530d4f9167c56a84b081042
Author: Or Gerlitz <ogerlitz@mellanox.com>
Date:   Thu Feb 23 12:02:41 2017 +0200

    net/mlx4: Change ENOTSUPP to EOPNOTSUPP
    
    As ENOTSUPP is specific to NFS, change the return error value to
    EOPNOTSUPP in various places in the mlx4 driver.
    
    Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
    Suggested-by: Yotam Gigi <yotamg@mellanox.com>
    Reviewed-by: Matan Barak <matanb@mellanox.com>
    Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

But it's spreading throughout the kernel like a wildfire, I counted 1364
in my tree :/  Some are in tools/, but still.  My understanding was that
system calls should never return values above 512, but I'm probably
wrong about that.

Given the popularity, and the fact its an ABI at this point, we
probably have no choice but to add it to libc, but to be clear IMO it's
not a blocker for your patches.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf v1 1/3] selftests/bpf: Test correctness of narrow 32bit read on 64bit field
  2019-05-15 13:47 ` [PATCH bpf v1 1/3] selftests/bpf: Test correctness of narrow 32bit read on 64bit field Krzesimir Nowak
@ 2019-05-23 16:02   ` Daniel Borkmann
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Borkmann @ 2019-05-23 16:02 UTC (permalink / raw)
  To: Krzesimir Nowak, bpf
  Cc: iago, alban, Shuah Khan, Alexei Starovoitov, Martin KaFai Lau,
	Song Liu, Yonghong Song, Andrey Ignatov, Jiong Wang,
	Jakub Kicinski, linux-kselftest, netdev, linux-kernel

On 05/15/2019 03:47 PM, Krzesimir Nowak wrote:
> 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.
> 
> This isn't really tested - the program is not being run, because
> BPF_PROG_TYPE_PERF_EVENT is not supported by bpf_test_run_prog.

One option could be to add actual support for BPF_PROG_TYPE_PERF_EVENT to
test_verifier where the program gets actually triggered, and the result
stored in a map value that the test case reads out for checking the result
against the expected one. Recently added something similar for LRU maps in
the test suite, that shouldn't be too complex.

Thanks,
Daniel

> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
>  tools/testing/selftests/bpf/verifier/var_off.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/verifier/var_off.c b/tools/testing/selftests/bpf/verifier/var_off.c
> index 8504ac937809..2668819dcc85 100644
> --- a/tools/testing/selftests/bpf/verifier/var_off.c
> +++ b/tools/testing/selftests/bpf/verifier/var_off.c
> @@ -246,3 +246,18 @@
>  	.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_4, 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,
> +},
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-05-23 16:27 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-15 13:47 [PATCH bpf v1 0/3] Test the 32bit narrow reads Krzesimir Nowak
2019-05-15 13:47 ` [PATCH bpf v1 1/3] selftests/bpf: Test correctness of narrow 32bit read on 64bit field Krzesimir Nowak
2019-05-23 16:02   ` Daniel Borkmann
2019-05-15 13:47 ` [PATCH bpf v1 2/3] selftests/bpf: Print a message when tester could not run a program Krzesimir Nowak
2019-05-15 21:45   ` Jakub Kicinski
2019-05-16  9:29     ` Krzesimir Nowak
2019-05-16 15:50       ` Jakub Kicinski
2019-05-16 16:21         ` Krzesimir Nowak
2019-05-16 16:54           ` Jakub Kicinski
2019-05-15 13:47 ` [PATCH bpf v1 3/3] selftests/bpf: Avoid a clobbering of errno Krzesimir Nowak
2019-05-15 21:50   ` Jakub Kicinski
2019-05-16  9:31     ` Krzesimir Nowak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).