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,

+},

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;

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.

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.

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;