Fix unsafe BPF_PROG_TEST_RUN interface

Use data_size_out as a size hint when copying test output to user space. A program using BPF_PERF_OUTPUT can compare its own buffer length with data_size_out after the syscall to detect whether truncation has taken place. Callers which so far did not set data_size_in are not affected.

Signed-off-by Lorenz Bauer <[email protected]>

---
 net/bpf/test_run.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

net/bpf/test_run.c +1 -1

index c89c22c49015..30c57b7f4ba4 100644
@@ -74,8 +74,15 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 {
        void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
        int err = -EFAULT;
+       u32 copy_size = size;
 
-       if (data_out && copy_to_user(data_out, data, size))

--
2.17.1

Require size_out to be non-NULL if data_out is given. This prevents accidental overwriting of process memory after the output buffer.

Adjust callers of bpf_prog_test_run to this behaviour.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/lib/bpf/bpf.c                      |  4 +++-
 tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

tools/lib/bpf/bpf.c +0 -0

index 03f9bcc4ef50..127a9aa6170e 100644

tools/lib/bpf/bpf.c +1 -1

        attr.test.repeat = repeat;
 
        ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
-       if (size_out)
+       if (data_out)
                *size_out = attr.test.data_size_out;
        if (retval)
                *retval = attr.test.retval;

tools/testing/selftests/bpf/test_progs.c +10 -1

index 2d3c04f45530..560d7527b86b 100644
@@ -150,6 +150,7 @@ static void test_xdp(void)
        bpf_map_update_elem(map_fd, &key4, &value4, 0);
        bpf_map_update_elem(map_fd, &key6, &value6, 0);
 
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
                                buf, &size, &retval, &duration);
 

@@ -158,6 +159,7 @@ static void test_xdp(void)
              "err %d errno %d retval %d size %d\n",
              err, errno, retval, size);
 
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
                                buf, &size, &retval, &duration);
        CHECK(err || retval != XDP_TX || size != 114 ||

@@ -182,6 +184,7 @@ static void test_xdp_adjust_tail(void)
                return;
        }
 
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
                                buf, &size, &retval, &duration);
 

@@ -189,6 +192,7 @@ static void test_xdp_adjust_tail(void)
              "ipv4", "err %d errno %d retval %d size %d\n",
              err, errno, retval, size);
 
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
                                buf, &size, &retval, &duration);
        CHECK(err || retval != XDP_TX || size != 54,

@@ -252,6 +256,7 @@ static void test_l4lb(const char *file)
                goto out;
        bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
 
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
                                buf, &size, &retval, &duration);
        CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 54 ||

@@ -259,6 +264,7 @@ static void test_l4lb(const char *file)
              "err %d errno %d retval %d size %d magic %x\n",
              err, errno, retval, size, *magic);
 
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
                                buf, &size, &retval, &duration);
        CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 74 ||

@@ -341,6 +347,7 @@ static void test_xdp_noinline(void)
                goto out;
        bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
 
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
                                buf, &size, &retval, &duration);
        CHECK(err || retval != 1 || size != 54 ||

@@ -348,6 +355,7 @@ static void test_xdp_noinline(void)
              "err %d errno %d retval %d size %d magic %x\n",
              err, errno, retval, size, *magic);
 
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
                                buf, &size, &retval, &duration);
        CHECK(err || retval != 1 || size != 74 ||

@@ -1795,6 +1803,7 @@ static void test_queue_stack_map(int type)
                        pkt_v4.iph.saddr = vals[MAP_SIZE - 1 - i] * 5;
                }
 
+               size = sizeof(buf);
                err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
                                        buf, &size, &retval, &duration);
                if (err || retval || size != sizeof(pkt_v4) ||

@@ -1808,6 +1817,7 @@ static void test_queue_stack_map(int type)
              err, errno, retval, size, iph->daddr);
 
        /* Queue is empty, program should return TC_ACT_SHOT */
+       size = sizeof(buf);
        err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
                                buf, &size, &retval, &duration);
        CHECK(err || retval != 2 /* TC_ACT_SHOT */|| size != sizeof(pkt_v4),
--

2.17.1

Make sure that bpf_prog_test_run returns the correct length in the size_out argument and that the kernel respects the output size hint.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

tools/testing/selftests/bpf/test_progs.c +3 -0

index 560d7527b86b..6ab98e10e86f 100644
@@ -124,6 +124,39 @@ static void test_pkt_access(void)
        bpf_object__close(obj);
 }
 
+static void test_output_size_hint(void)
+{
+       const char *file = "./test_pkt_access.o";

tools/testing/selftests/bpf/test_progs.c +21 -1

+
+       memset(buf, 0, sizeof(buf));
+
+       size = 5;
+       err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+                               buf, &size, &retval, &duration);
+       CHECK(err || retval, "run",
+             "err %d errno %d retval %d\n",
+             err, errno, retval);
+
+       CHECK(size != sizeof(pkt_v4), "out_size",
+             "incorrect output size, want %lu have %u\n",
+             sizeof(pkt_v4), size);
+
+       CHECK(buf[5] != 0, "overflow",
+             "prog_test_run ignored size hint\n");
+
+       bpf_object__close(obj);
+}
+
 static void test_xdp(void)
 {
        struct vip key4 = {.protocol = 6, .family = AF_INET};

@@ -1847,6 +1880,7 @@ int main(void)
        jit_enabled = is_jit_enabled();
 
        test_pkt_access();
+       test_output_size_hint();
        test_xdp();
        test_xdp_adjust_tail();
        test_l4lb_all();
--

2.17.1

Quoted 21 lines · Lorenz Bauer

Make sure that bpf_prog_test_run returns the correct length in the size_out argument and that the kernel respects the output size hint.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

tools/testing/selftests/bpf/test_progs.c +3 -0

index 560d7527b86b..6ab98e10e86f 100644
@@ -124,6 +124,39 @@ static void test_pkt_access(void)
        bpf_object__close(obj);
 }
 
+static void test_output_size_hint(void)
+{
+       const char *file = "./test_pkt_access.o";

tools/testing/selftests/bpf/test_progs.c static void test_pkt_access(void) · line 139

Quoted 10 lines

tools/testing/selftests/bpf/test_progs.c +10 -0

+       struct bpf_object *obj;
+       __u32 retval, size, duration;
+       int err, prog_fd;
+       char buf[10];
+
+       err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+       if (err) {
+               error_cnt++;
+               return;
+       }

CHECK can also be used here.

if (CHECK(...)) {
   goto done;
}

where label "done" is right before bpf_object__close.

Quoted 34 lines · Y Song

Quoted 21 lines · Lorenz Bauer

Make sure that bpf_prog_test_run returns the correct length in the size_out argument and that the kernel respects the output size hint.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

tools/testing/selftests/bpf/test_progs.c +3 -0

index 560d7527b86b..6ab98e10e86f 100644
@@ -124,6 +124,39 @@ static void test_pkt_access(void)
        bpf_object__close(obj);
 }
 
+static void test_output_size_hint(void)
+{
+       const char *file = "./test_pkt_access.o";

tools/testing/selftests/bpf/test_progs.c static void test_pkt_access(void) · line 139

Quoted 10 lines

tools/testing/selftests/bpf/test_progs.c +10 -0

+       struct bpf_object *obj;
+       __u32 retval, size, duration;
+       int err, prog_fd;
+       char buf[10];
+
+       err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+       if (err) {
+               error_cnt++;
+               return;
+       }

CHECK can also be used here.

Quoted 4 lines

if (CHECK(...)) {
   goto done;
}

where label "done" is right before bpf_object__close.

I just copied this part from test_pkt_access, happy to change it though. However, I think "goto done" would lead to freeing an unallocated object in this case?

--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out. This is because bpf_test_finish copies the output buffer to user space without checking its size. This can lead to the kernel overwriting data in user space after the buffer if xdp_adjust_head and friends are in play.

Fix this by using bpf_attr.test.data_size_out as a size hint. The old behaviour is retained if size_hint is zero.

Interestingly, do_test_single() in test_verifier.c already assumes that this is the intended use of data_size_out, and sets it to the output buffer size.

Lorenz Bauer (3):
bpf: respect size hint to BPF_PROG_TEST_RUN if present
libbpf: require size hint in bpf_prog_test_run
selftests: add a test for bpf_prog_test_run output size

 net/bpf/test_run.c                       |  9 ++++-
 tools/lib/bpf/bpf.c                      |  4 ++-
 tools/testing/selftests/bpf/test_progs.c | 44 ++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 2 deletions(-)

--
2.17.1

There is a slight change of user space behavior for this patch. Without this patch, the value bpf_attr.test.data_size_out is output only. For example,
output buffer : out_buf (user allocated size 10) data_size_out is a random value (e.g., 1),

The actual data to copy is 5.

In today's implementation, the kernel will copy 5 and set data_size_out is 5.

With this patch, the kernel will copy 1 and set data_size_out is 5.

I am not 100% sure at this time whether we CAN overload data_size_out since it MAY break existing applications.

Quoted 11 lines · Y Song

There is a slight change of user space behavior for this patch. Without this patch, the value bpf_attr.test.data_size_out is output only. For example,
output buffer : out_buf (user allocated size 10) data_size_out is a random value (e.g., 1),

The actual data to copy is 5.

In today's implementation, the kernel will copy 5 and set data_size_out is 5.

Quoted 4 lines

With this patch, the kernel will copy 1 and set data_size_out is 5.

I am not 100% sure at this time whether we CAN overload data_size_out since it MAY break existing applications.

Yes, that's correct. I think that the likelihood of this is low. It would affect code that uses bpf_attr without zeroing it first. I had a look around, and I could only find code that would keep working:

Require size_out to be non-NULL if data_out is given. This prevents accidental overwriting of process memory after the output buffer.

Adjust callers of bpf_prog_test_run to this behaviour.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/lib/bpf/bpf.c                      |  7 ++++++-
 tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

tools/lib/bpf/bpf.c +6 -1

index 961e1b9fc592..1a835ff27486 100644
@@ -407,15 +407,20 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
 	union bpf_attr attr;
 	int ret;
 
+	if (data_out && !size_out)
+		return -EINVAL;
+
 	bzero(&attr, sizeof(attr));
 	attr.test.prog_fd = prog_fd;
 	attr.test.data_in = ptr_to_u64(data);
 	attr.test.data_out = ptr_to_u64(data_out);
 	attr.test.data_size_in = size;
+	if (data_out)
+		attr.test.data_size_out = *size_out;
 	attr.test.repeat = repeat;
 
 	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
-	if (size_out)
+	if (data_out)
 		*size_out = attr.test.data_size_out;
 	if (retval)
 		*retval = attr.test.retval;

tools/testing/selftests/bpf/test_progs.c +1 -0

index c1e688f61061..299938603cb6 100644
@@ -150,6 +150,7 @@ static void test_xdp(void)
 	bpf_map_update_elem(map_fd, &key4, &value4, 0);
 	bpf_map_update_elem(map_fd, &key6, &value6, 0);
 
+	size = sizeof(buf);
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 				buf, &size, &retval, &duration);
 

Quoted 50 lines · Lorenz Bauer

Require size_out to be non-NULL if data_out is given. This prevents accidental overwriting of process memory after the output buffer.

Adjust callers of bpf_prog_test_run to this behaviour.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/lib/bpf/bpf.c                      |  7 ++++++-
 tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

tools/lib/bpf/bpf.c +6 -1

index 961e1b9fc592..1a835ff27486 100644
@@ -407,15 +407,20 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
      union bpf_attr attr;
      int ret;
 
+     if (data_out && !size_out)
+             return -EINVAL;
+
      bzero(&attr, sizeof(attr));
      attr.test.prog_fd = prog_fd;
      attr.test.data_in = ptr_to_u64(data);
      attr.test.data_out = ptr_to_u64(data_out);
      attr.test.data_size_in = size;
+     if (data_out)
+             attr.test.data_size_out = *size_out;
      attr.test.repeat = repeat;
 
      ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
-     if (size_out)
+     if (data_out)
              *size_out = attr.test.data_size_out;
      if (retval)
              *retval = attr.test.retval;

tools/testing/selftests/bpf/test_progs.c +1 -0

index c1e688f61061..299938603cb6 100644
@@ -150,6 +150,7 @@ static void test_xdp(void)
      bpf_map_update_elem(map_fd, &key4, &value4, 0);
      bpf_map_update_elem(map_fd, &key6, &value6, 0);
 
+     size = sizeof(buf);
      err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
                              buf, &size, &retval, &duration);
 

tools/testing/selftests/bpf/test_progs.c static void test_xdp(void) · line 164

Quoted 6 lines

tools/testing/selftests/bpf/test_progs.c +1 -0

@@ -158,6 +159,7 @@ static void test_xdp(void)
            "err %d errno %d retval %d size %d\n",
            err, errno, retval, size);
 
+     size = sizeof(buf);
      err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
                              buf, &size, &retval, &duration);

This will surely break existing bpf_prog_test_run users.
Like it will break our testing framework.
we can fix out stuff and libbpf is a user space library, but I don't think that this is the case to invoke such pain.
libbpf's bpf_prog_test_run() should be a simple wrapper on top of syscall. I don't think it should be making such restrictions on api.

btw patch 1 looks good to me.

Quoted 66 lines · Alexei Starovoitov

Quoted 50 lines · Lorenz Bauer

Require size_out to be non-NULL if data_out is given. This prevents accidental overwriting of process memory after the output buffer.

Adjust callers of bpf_prog_test_run to this behaviour.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/lib/bpf/bpf.c                      |  7 ++++++-
 tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

tools/lib/bpf/bpf.c +6 -1

index 961e1b9fc592..1a835ff27486 100644
@@ -407,15 +407,20 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
      union bpf_attr attr;
      int ret;
 
+     if (data_out && !size_out)
+             return -EINVAL;
+
      bzero(&attr, sizeof(attr));
      attr.test.prog_fd = prog_fd;
      attr.test.data_in = ptr_to_u64(data);
      attr.test.data_out = ptr_to_u64(data_out);
      attr.test.data_size_in = size;
+     if (data_out)
+             attr.test.data_size_out = *size_out;
      attr.test.repeat = repeat;
 
      ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
-     if (size_out)
+     if (data_out)
              *size_out = attr.test.data_size_out;
      if (retval)
              *retval = attr.test.retval;

tools/testing/selftests/bpf/test_progs.c +1 -0

index c1e688f61061..299938603cb6 100644
@@ -150,6 +150,7 @@ static void test_xdp(void)
      bpf_map_update_elem(map_fd, &key4, &value4, 0);
      bpf_map_update_elem(map_fd, &key6, &value6, 0);
 
+     size = sizeof(buf);
      err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
                              buf, &size, &retval, &duration);
 

tools/testing/selftests/bpf/test_progs.c static void test_xdp(void) · line 164

Quoted 6 lines

tools/testing/selftests/bpf/test_progs.c +1 -0

@@ -158,6 +159,7 @@ static void test_xdp(void)
            "err %d errno %d retval %d size %d\n",
            err, errno, retval, size);
 
+     size = sizeof(buf);
      err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
                              buf, &size, &retval, &duration);

This will surely break existing bpf_prog_test_run users.
Like it will break our testing framework.
we can fix out stuff and libbpf is a user space library, but I don't think that this is the case to invoke such pain.
libbpf's bpf_prog_test_run() should be a simple wrapper on top of syscall. I don't think it should be making such restrictions on api.

Quoted 2 lines

btw patch 1 looks good to me.

What if I add bpf_prog_test_run_safe or similar, with the behaviour proposed in the patch?
Makes sense that you don't want to break existing users of libbpf outside the kernel, OTOH
user space really should specify the output buffer length (or be given the choice).

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out. This is because bpf_test_finish copies the output buffer to user space without checking its size. This can lead to the kernel overwriting data in user space after the buffer if xdp_adjust_head and friends are in play.

Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out

Lorenz Bauer (4):
bpf: respect size hint to BPF_PROG_TEST_RUN if present
tools: sync uapi/linux/bpf.h
libbpf: require size hint in bpf_prog_test_run
selftests: add a test for bpf_prog_test_run output size

Add a new function, which encourages safe usage of the test interface. bpf_prog_test_run continues to work as before, but should be considered unsafe.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/lib/bpf/bpf.c | 27 +++++++++++++++++++++++++++
 tools/lib/bpf/bpf.h | 13 +++++++++++++
 2 files changed, 40 insertions(+)

tools/lib/bpf/bpf.c +27 -0

index 961e1b9fc592..f8518bef6886 100644
@@ -424,6 +424,33 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
 	return ret;
 }
 
+int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+			    __u32 *size_out, __u32 *retval, __u32 *duration)
+{
+	union bpf_attr attr;
+	int ret;
+
+	if (!test_attr->data_out && test_attr->size_out > 0)
+		return -EINVAL;
+
+	bzero(&attr, sizeof(attr));
+	attr.test.prog_fd = test_attr->prog_fd;
+	attr.test.data_in = ptr_to_u64(test_attr->data);
+	attr.test.data_out = ptr_to_u64(test_attr->data_out);
+	attr.test.data_size_in = test_attr->size;
+	attr.test.data_size_out = test_attr->size_out;
+	attr.test.repeat = test_attr->repeat;
+
+	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
+	if (size_out)
+		*size_out = attr.test.data_size_out;
+	if (retval)
+		*retval = attr.test.retval;
+	if (duration)
+		*duration = attr.test.duration;
+	return ret;
+}
+
 int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
 {
 	union bpf_attr attr;

tools/lib/bpf/bpf.h +0 -0

index 26a51538213c..570f19f77f42 100644
@@ -110,6 +110,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
 LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);

Quoted 5 lines · Lorenz Bauer

Add a new function, which encourages safe usage of the test interface. bpf_prog_test_run continues to work as before, but should be considered unsafe.

Signed-off-by Lorenz Bauer <[email protected]>

Set looks good to me, thanks! Three small things below:

Quoted 49 lines

---
 tools/lib/bpf/bpf.c | 27 +++++++++++++++++++++++++++
 tools/lib/bpf/bpf.h | 13 +++++++++++++
 2 files changed, 40 insertions(+)

tools/lib/bpf/bpf.c +27 -0

index 961e1b9fc592..f8518bef6886 100644
@@ -424,6 +424,33 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
 	return ret;
 }
 
+int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+			    __u32 *size_out, __u32 *retval, __u32 *duration)
+{
+	union bpf_attr attr;
+	int ret;
+
+	if (!test_attr->data_out && test_attr->size_out > 0)
+		return -EINVAL;
+
+	bzero(&attr, sizeof(attr));
+	attr.test.prog_fd = test_attr->prog_fd;
+	attr.test.data_in = ptr_to_u64(test_attr->data);
+	attr.test.data_out = ptr_to_u64(test_attr->data_out);
+	attr.test.data_size_in = test_attr->size;
+	attr.test.data_size_out = test_attr->size_out;
+	attr.test.repeat = test_attr->repeat;
+
+	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
+	if (size_out)
+		*size_out = attr.test.data_size_out;
+	if (retval)
+		*retval = attr.test.retval;
+	if (duration)
+		*duration = attr.test.duration;
+	return ret;
+}
+
 int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
 {
 	union bpf_attr attr;

tools/lib/bpf/bpf.h +0 -0

index 26a51538213c..570f19f77f42 100644
@@ -110,6 +110,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
 LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);

tools/lib/bpf/bpf.h LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd, · line 120

Quoted 10 lines

tools/lib/bpf/bpf.h +8 -0

 LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
 				enum bpf_attach_type type);
+
+struct bpf_prog_test_run_attr {
+	int prog_fd;
+	int repeat;
+	const void *data;
+	__u32 size;
+	void *data_out; /* optional */
+	__u32 size_out;

Small nit: could we name these data_{in,out} and data_size_{in,out} as well, so it's analog to the ones from the bpf_attr?

tools/lib/bpf/bpf.h LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd, · line 125

Quoted 5 lines

tools/lib/bpf/bpf.h +5 -0

+};
+
+LIBBPF_API int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+				       __u32 *size_out, __u32 *retval,
+				       __u32 *duration);

tools/lib/bpf/bpf.h LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd, · line 128

Quoted 3 lines

tools/lib/bpf/bpf.h +0 -0

 LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
 				 __u32 size, void *data_out, __u32 *size_out,
 				 __u32 *retval, __u32 *duration);

Could we add a comment into the header here stating that we discourage bpf_prog_test_run()'s use?

It would probably also make sense since we go that route that we would convert the 10 bpf_prog_test_run() instances under test_progs.c at the same time so that people extending or looking at BPF kselftests don't copy discouraged bpf_prog_test_run() api as examples from this point onwards anymore.

Quoted 72 lines · Daniel Borkmann

Quoted 5 lines · Lorenz Bauer

Add a new function, which encourages safe usage of the test interface. bpf_prog_test_run continues to work as before, but should be considered unsafe.

Signed-off-by Lorenz Bauer <[email protected]>

Set looks good to me, thanks! Three small things below:

Quoted 49 lines

---
 tools/lib/bpf/bpf.c | 27 +++++++++++++++++++++++++++
 tools/lib/bpf/bpf.h | 13 +++++++++++++
 2 files changed, 40 insertions(+)

tools/lib/bpf/bpf.c +27 -0

index 961e1b9fc592..f8518bef6886 100644
@@ -424,6 +424,33 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
    return ret;
 }
 
+int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+                       __u32 *size_out, __u32 *retval, __u32 *duration)
+{
+   union bpf_attr attr;
+   int ret;
+
+   if (!test_attr->data_out && test_attr->size_out > 0)
+           return -EINVAL;
+
+   bzero(&attr, sizeof(attr));
+   attr.test.prog_fd = test_attr->prog_fd;
+   attr.test.data_in = ptr_to_u64(test_attr->data);
+   attr.test.data_out = ptr_to_u64(test_attr->data_out);
+   attr.test.data_size_in = test_attr->size;
+   attr.test.data_size_out = test_attr->size_out;
+   attr.test.repeat = test_attr->repeat;
+
+   ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
+   if (size_out)
+           *size_out = attr.test.data_size_out;
+   if (retval)
+           *retval = attr.test.retval;
+   if (duration)
+           *duration = attr.test.duration;
+   return ret;
+}
+
 int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
 {
    union bpf_attr attr;

tools/lib/bpf/bpf.h +0 -0

index 26a51538213c..570f19f77f42 100644
@@ -110,6 +110,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
 LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);

tools/lib/bpf/bpf.h LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd, · line 120

Quoted 10 lines

tools/lib/bpf/bpf.h +8 -0

 LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
                            enum bpf_attach_type type);
+
+struct bpf_prog_test_run_attr {
+   int prog_fd;
+   int repeat;
+   const void *data;
+   __u32 size;
+   void *data_out; /* optional */
+   __u32 size_out;

Small nit: could we name these data_{in,out} and data_size_{in,out} as well, so it's analog to the ones from the bpf_attr?

Quoted 5 lines

tools/lib/bpf/bpf.h LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd, · line 125

Quoted 5 lines

tools/lib/bpf/bpf.h +5 -0

+};
+
+LIBBPF_API int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+                                  __u32 *size_out, __u32 *retval,
+                                  __u32 *duration);

can we keep return values in struct bpf_prog_test_run_attr ? I think the interface will be easier to use and faster. Less pointers to pass around.

Quoted 5 lines

tools/lib/bpf/bpf.h LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd, · line 128

Quoted 4 lines

tools/lib/bpf/bpf.h +0 -0

 LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
                             __u32 size, void *data_out, __u32 *size_out,
                             __u32 *retval, __u32 *duration);

Quoted 8 lines

Could we add a comment into the header here stating that we discourage bpf_prog_test_run()'s use?

It would probably also make sense since we go that route that we would convert the 10 bpf_prog_test_run() instances under test_progs.c at the same time so that people extending or looking at BPF kselftests don't copy discouraged bpf_prog_test_run() api as examples from this point onwards anymore.

I would keep bpf_prog_test_run() and test_progs.c as-is. I think the prog_test_run in the current form is actually fine. I don't find it 'unsafe'.
When it's called on a given bpf prog the user knows what prog suppose to do. If prog is increasing the packet size the test writer knows that and should size the output buffer accordingly.

Also there are plenty of tests where progs don't change the packet size and any test with 'repeat > 1' should have the packet size return to initial value. Like if the test is doing ipip encap at the end of the run the bpf prog should remove that encap. Otherwise 'repeat > 1' will produce wrong results.

Quoted 2 lines · Alexei Starovoitov

Quoted 1 line · Daniel Borkmann

On 11/22/2018 03:09 PM, Lorenz Bauer wrote:

Quoted 13 lines

Quoted 4 lines

Quoted 3 lines

LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,

                             __u32 size, void *data_out, __u32 *size_out,
                             __u32 *retval, __u32 *duration);

Quoted 8 lines

Could we add a comment into the header here stating that we discourage bpf_prog_test_run()'s use?

It would probably also make sense since we go that route that we would convert the 10 bpf_prog_test_run() instances under test_progs.c at the same time so that people extending or looking at BPF kselftests don't copy discouraged bpf_prog_test_run() api as examples from this point onwards anymore.

Quoted 6 lines

I would keep bpf_prog_test_run() and test_progs.c as-is. I think the prog_test_run in the current form is actually fine. I don't find it 'unsafe'.
When it's called on a given bpf prog the user knows what prog suppose to do. If prog is increasing the packet size the test writer knows that and should size the output buffer accordingly.

I guess this is a matter of perspective. I prefer an interface that gives me back
an error message, rather than corrupt my stack / heap, when the assumptions change. It's also nicer to use from "managed" languages like Go where users aren't as accustomed to issues like these.

Do you object to me adding the disclaimer to the header as Daniel suggested?

Quoted 5 lines

Also there are plenty of tests where progs don't change the packet size and any test with 'repeat > 1' should have the packet size return to initial value. Like if the test is doing ipip encap at the end of the run the bpf prog should remove that encap. Otherwise 'repeat > 1' will produce wrong results.

Yup. Another thorny part of the test interface, which I'd like to improve! :) Don't know how to do it yet though.

That's what I had initially, but that makes re-using test_attr really awkward. Either
you need to reset data_out_size before every call because it is used to return the
buffer size,

It also means
we can't take a const struct attr, which is contrary to the other xattr functions which
already exist.

I think actually inspecting the required size of the output buffer will be a rare
occurrence, so making the user jump through the hoop of a pointer doesn't seem too onerous.

Quoted 6 lines · Lorenz Bauer

That's what I had initially, but that makes re-using test_attr really awkward. Either
you need to reset data_out_size before every call because it is used to return the
buffer size,

I think that is exactly what the user of the interface would want to do. Why would anyone keep reusing the same test_attr on multiple calls into the kernel without changing the fields?

Quoted 5 lines

It also means
we can't take a const struct attr, which is contrary to the other xattr functions which
already exist.

I don't see an issue with that.

Quoted 4 lines

I think actually inspecting the required size of the output buffer will be a rare
occurrence, so making the user jump through the hoop of a pointer doesn't seem too onerous.

I think the opposite is the case.
If the output buffer is provided the test will be comparing it to expected value.

Add a new function, which encourages safe usage of the test interface. bpf_prog_test_run continues to work as before, but should be considered unsafe.

Signed-off-by Lorenz Bauer <[email protected]>

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out. This is because bpf_test_finish copies the output buffer to user space without checking its size. This can lead to the kernel overwriting data in user space after the buffer if xdp_adjust_head and friends are in play.

Thanks to everyone for their advice and patience with this patch set!

Changes in v5:
* Fix up libbpf.map

Changes in v4:
* Document bpf_prog_test_run and bpf_prog_test_run_xattr
* Use struct bpf_prog_test_run_attr for return values

Changes in v3:
* Introduce bpf_prog_test_run_xattr instead of modifying the existing function

Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out