[PATCH bpf-next 1/3] bpf: support input __sk_buff context in BPF_PROG_TEST_RUN

[PATCH bpf-next 1/3] bpf: support input __sk_buff context in BPF_PROG_TEST_RUN

[Stanislav Fomichev]

Add new set of arguments to bpf_attr for BPF_PROG_TEST_RUN:
* ctx_in/ctx_size_in - input context
* ctx_out/ctx_size_out - output context

The intended use case is to pass some meta data to the test runs that
operate on skb (this has being brought up on recent LPC).

For programs that use bpf_prog_test_run_skb, support __sk_buff input and
output. Initially, from input __sk_buff, copy _only_ cb and priority into
skb, all other non-zero fields are prohibited (with EINVAL).
If the user has set ctx_out/ctx_size_out, copy the potentially modified
__sk_buff back to the userspace.

We require all fields of input __sk_buff except the ones we explicitly
support to be set to zero. The expectation is that in the future we might
add support for more fields and we want to fail explicitly if the user
runs the program on the kernel where we don't yet support them.

The API is intentionally vague (i.e. we don't explicitly add __sk_buff
to bpf_attr, but ctx_in) to potentially let other test_run types use
this interface in the future (this can be xdp_md for xdp types for
example).

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 include/uapi/linux/bpf.h |   7 +++
 kernel/bpf/syscall.c     |   2 +-
 net/bpf/test_run.c       | 133 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 136 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 837024512baf..8e96f99cebf8 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -396,6 +396,13 @@ union bpf_attr {
 		__aligned_u64	data_out;
 		__u32		repeat;
 		__u32		duration;
+		__u32		ctx_size_in;	/* input: len of ctx_in */
+		__u32		ctx_size_out;	/* input/output: len of ctx_out
+						 *   returns ENOSPC if ctx_out
+						 *   is too small.
+						 */
+		__aligned_u64	ctx_in;
+		__aligned_u64	ctx_out;
 	} test;
 
 	struct { /* anonymous struct used by BPF_*_GET_*_ID */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 1d65e56594db..4ad754e2943a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1949,7 +1949,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
 	return cgroup_bpf_prog_query(attr, uattr);
 }
 
-#define BPF_PROG_TEST_RUN_LAST_FIELD test.duration
+#define BPF_PROG_TEST_RUN_LAST_FIELD test.ctx_out
 
 static int bpf_prog_test_run(const union bpf_attr *attr,
 			     union bpf_attr __user *uattr)
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index fab142b796ef..593912a66d64 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -123,12 +123,121 @@ 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);
+	u32 size = kattr->test.ctx_size_in;
+	void *data;
+
+	if ((kattr->test.ctx_size_in && !kattr->test.ctx_in) ||
+	    (!kattr->test.ctx_size_in && kattr->test.ctx_in))
+		return ERR_PTR(-EINVAL);
+
+	if ((kattr->test.ctx_size_out && !kattr->test.ctx_out) ||
+	    (!kattr->test.ctx_size_out && kattr->test.ctx_out))
+		return ERR_PTR(-EINVAL);
+
+	if (!size)
+		return NULL;
+
+	if (size > max_size)
+		return ERR_PTR(-EINVAL);
+
+	data = kzalloc(max_size, GFP_USER);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	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);
+	u32 copy_size = size;
+
+	if (!kattr->test.ctx_size_out)
+		return 0;
+
+	if (copy_size > kattr->test.ctx_size_out)
+		return -ENOSPC;
+
+	if (data_out && copy_to_user(data_out, data, copy_size))
+		return -EFAULT;
+	if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size)))
+		return -EFAULT;
+
+	return 0;
+}
+
+/**
+ * range_is_zero - test whether buffer is initialized
+ * @buf: buffer to check
+ * @from: check from this position
+ * @to: check up until (excluding) this position
+ *
+ * This function returns true if the there is a non-zero byte
+ * in the buf in the range [from,to).
+ */
+static inline bool range_is_zero(void *buf, size_t from, size_t to)
+{
+	return !memchr_inv((u8 *)buf + from, 0, to - from);
+}
+
+static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
+{
+	struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb;
+
+	if (!__skb)
+		return 0;
+
+	/* make sure the fields we don't use are zeroed */
+	if (!range_is_zero(__skb, 0, offsetof(struct __sk_buff, priority)))
+		return -EINVAL;
+
+	/* priority is allowed */
+
+	if (!range_is_zero(__skb, offsetof(struct __sk_buff, priority) +
+			   FIELD_SIZEOF(struct __sk_buff, priority),
+			   offsetof(struct __sk_buff, cb)))
+		return -EINVAL;
+
+	/* cb is allowed */
+
+	if (!range_is_zero(__skb, offsetof(struct __sk_buff, cb) +
+			   FIELD_SIZEOF(struct __sk_buff, cb),
+			   sizeof(struct __sk_buff)))
+		return -EINVAL;
+
+	skb->priority = __skb->priority;
+	memcpy(&cb->data, __skb->cb, QDISC_CB_PRIV_LEN);
+
+	return 0;
+}
+
+static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
+{
+	struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb;
+
+	if (!__skb)
+		return;
+
+	__skb->priority = skb->priority;
+	memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
+}
+
 int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 			  union bpf_attr __user *uattr)
 {
 	bool is_l2 = false, is_direct_pkt_access = false;
 	u32 size = kattr->test.data_size_in;
 	u32 repeat = kattr->test.repeat;
+	struct __sk_buff *ctx = NULL;
 	u32 retval, duration;
 	int hh_len = ETH_HLEN;
 	struct sk_buff *skb;
@@ -141,6 +250,12 @@ 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));
+	if (IS_ERR(ctx)) {
+		kfree(data);
+		return PTR_ERR(ctx);
+	}
+
 	switch (prog->type) {
 	case BPF_PROG_TYPE_SCHED_CLS:
 	case BPF_PROG_TYPE_SCHED_ACT:
@@ -158,6 +273,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	sk = kzalloc(sizeof(struct sock), GFP_USER);
 	if (!sk) {
 		kfree(data);
+		kfree(ctx);
 		return -ENOMEM;
 	}
 	sock_net_set(sk, current->nsproxy->net_ns);
@@ -166,6 +282,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	skb = build_skb(data, 0);
 	if (!skb) {
 		kfree(data);
+		kfree(ctx);
 		kfree(sk);
 		return -ENOMEM;
 	}
@@ -180,12 +297,12 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 		__skb_push(skb, hh_len);
 	if (is_direct_pkt_access)
 		bpf_compute_data_pointers(skb);
+	ret = convert___skb_to_skb(skb, ctx);
+	if (ret)
+		goto out;
 	ret = bpf_test_run(prog, skb, repeat, &retval, &duration);
-	if (ret) {
-		kfree_skb(skb);
-		kfree(sk);
-		return ret;
-	}
+	if (ret)
+		goto out;
 	if (!is_l2) {
 		if (skb_headroom(skb) < hh_len) {
 			int nhead = HH_DATA_ALIGN(hh_len - skb_headroom(skb));
@@ -198,14 +315,20 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 		}
 		memset(__skb_push(skb, hh_len), 0, hh_len);
 	}
+	convert_skb_to___skb(skb, ctx);
 
 	size = skb->len;
 	/* 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));
+out:
 	kfree_skb(skb);
 	kfree(sk);
+	kfree(ctx);
 	return ret;
 }
 
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH bpf-next 2/3] libbpf: add support for ctx_{size,}_{in,out} in BPF_PROG_TEST_RUN

[Stanislav Fomichev]

Support recently introduced input/output context for test runs.
We extend only bpf_prog_test_run_xattr. bpf_prog_test_run is
unextendable and left as is.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/include/uapi/linux/bpf.h | 7 +++++++
 tools/lib/bpf/bpf.c            | 5 +++++
 tools/lib/bpf/bpf.h            | 5 +++++
 3 files changed, 17 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 837024512baf..8e96f99cebf8 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -396,6 +396,13 @@ union bpf_attr {
 		__aligned_u64	data_out;
 		__u32		repeat;
 		__u32		duration;
+		__u32		ctx_size_in;	/* input: len of ctx_in */
+		__u32		ctx_size_out;	/* input/output: len of ctx_out
+						 *   returns ENOSPC if ctx_out
+						 *   is too small.
+						 */
+		__aligned_u64	ctx_in;
+		__aligned_u64	ctx_out;
 	} test;
 
 	struct { /* anonymous struct used by BPF_*_GET_*_ID */
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index a1db869a6fda..d740da7ae1d3 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -545,10 +545,15 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
 	attr.test.data_out = ptr_to_u64(test_attr->data_out);
 	attr.test.data_size_in = test_attr->data_size_in;
 	attr.test.data_size_out = test_attr->data_size_out;
+	attr.test.ctx_in = ptr_to_u64(test_attr->ctx_in);
+	attr.test.ctx_out = ptr_to_u64(test_attr->ctx_out);
+	attr.test.ctx_size_in = test_attr->ctx_size_in;
+	attr.test.ctx_size_out = test_attr->ctx_size_out;
 	attr.test.repeat = test_attr->repeat;
 
 	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
 	test_attr->data_size_out = attr.test.data_size_out;
+	test_attr->ctx_size_out = attr.test.ctx_size_out;
 	test_attr->retval = attr.test.retval;
 	test_attr->duration = attr.test.duration;
 	return ret;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index e2c0df7b831f..385418a6d54b 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -135,6 +135,11 @@ struct bpf_prog_test_run_attr {
 			      * out: length of data_out */
 	__u32 retval;        /* out: return code of the BPF program */
 	__u32 duration;      /* out: average per repetition in ns */
+	const void *ctx_in; /* optional */
+	__u32 ctx_size_in;
+	void *ctx_out;      /* optional */
+	__u32 ctx_size_out; /* in: max length of ctx_out
+			     * out: length of cxt_out */
 };
 
 LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr);
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH bpf-next 3/3] selftests: bpf: add selftest for __sk_buff context in BPF_PROG_TEST_RUN

[Stanislav Fomichev]

Simple test that sets cb to {1,2,3,4,5} and priority to 6, runs bpf
program that fails if cb is not what we expect and increments cb[i] and
priority. When the test finishes, we check that cb is now {2,3,4,5,6}
and priority is 7.

We also test the sanity checks:
* ctx_in is provided, but ctx_size_in is zero (same for
  ctx_out/ctx_size_out)
* unexpected non-zero fields in __sk_buff return EINVAL

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 .../testing/selftests/bpf/prog_tests/skb_cb.c | 89 +++++++++++++++++++
 .../testing/selftests/bpf/progs/test_skb_cb.c | 21 +++++
 2 files changed, 110 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/skb_cb.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_skb_cb.c

diff --git a/tools/testing/selftests/bpf/prog_tests/skb_cb.c b/tools/testing/selftests/bpf/prog_tests/skb_cb.c
new file mode 100644
index 000000000000..83f0b95ae0e9
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/skb_cb.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_skb_cb(void)
+{
+	struct __sk_buff skb = {
+		.cb[0] = 1,
+		.cb[1] = 2,
+		.cb[2] = 3,
+		.cb[3] = 4,
+		.cb[4] = 5,
+		.priority = 6,
+	};
+	struct bpf_prog_test_run_attr tattr = {
+		.data_in = &pkt_v4,
+		.data_size_in = sizeof(pkt_v4),
+		.ctx_in = &skb,
+		.ctx_size_in = sizeof(skb),
+		.ctx_out = &skb,
+		.ctx_size_out = sizeof(skb),
+	};
+	struct bpf_object *obj;
+	int err;
+	int i;
+
+	err = bpf_prog_load("./test_skb_cb.o", BPF_PROG_TYPE_SCHED_CLS, &obj,
+			    &tattr.prog_fd);
+	if (CHECK_ATTR(err, "load", "err %d errno %d\n", err, errno))
+		return;
+
+	/* ctx_in != NULL, ctx_size_in == 0 */
+
+	tattr.ctx_size_in = 0;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err == 0, "ctx_size_in", "err %d errno %d\n", err, errno);
+	tattr.ctx_size_in = sizeof(skb);
+
+	/* ctx_out != NULL, ctx_size_out == 0 */
+
+	tattr.ctx_size_out = 0;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err == 0, "ctx_size_out", "err %d errno %d\n", err, errno);
+	tattr.ctx_size_out = sizeof(skb);
+
+	/* non-zero [len, tc_index] fields should be rejected*/
+
+	skb.len = 1;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err == 0, "len", "err %d errno %d\n", err, errno);
+	skb.len = 0;
+
+	skb.tc_index = 1;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err == 0, "tc_index", "err %d errno %d\n", err, errno);
+	skb.tc_index = 0;
+
+	/* non-zero [hash, sk] fields should be rejected */
+
+	skb.hash = 1;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err == 0, "hash", "err %d errno %d\n", err, errno);
+	skb.hash = 0;
+
+	skb.sk = (struct bpf_sock *)1;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err == 0, "sk", "err %d errno %d\n", err, errno);
+	skb.sk = 0;
+
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err != 0 || tattr.retval,
+		   "run",
+		   "err %d errno %d retval %d\n",
+		   err, errno, tattr.retval);
+
+	CHECK_ATTR(tattr.ctx_size_out != sizeof(skb),
+		   "ctx_size_out",
+		   "incorrect output size, want %lu have %u\n",
+		   sizeof(skb), tattr.ctx_size_out);
+
+	for (i = 0; i < 5; i++)
+		CHECK_ATTR(skb.cb[i] != i + 2,
+			   "ctx_out_cb",
+			   "skb->cb[i] == %d, expected %d\n",
+			   skb.cb[i], i + 2);
+	CHECK_ATTR(skb.priority != 7,
+		   "ctx_out_priority",
+		   "skb->priority == %d, expected %d\n",
+		   skb.priority, 7);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_skb_cb.c b/tools/testing/selftests/bpf/progs/test_skb_cb.c
new file mode 100644
index 000000000000..50c9df29668f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_skb_cb.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
+
+SEC("skb_cb")
+int process(struct __sk_buff *skb)
+{
+	#pragma clang loop unroll(full)
+	for (int i = 0; i < 5; i++) {
+		if (skb->cb[i] != i + 1)
+			return 1;
+		skb->cb[i]++;
+	}
+	skb->priority++;
+
+	return 0;
+}
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH bpf-next 1/3] bpf: support input __sk_buff context in BPF_PROG_TEST_RUN

[Martin Lau]

On Thu, Apr 04, 2019 at 11:51:31AM -0700, Stanislav Fomichev wrote:
> Add new set of arguments to bpf_attr for BPF_PROG_TEST_RUN:
> * ctx_in/ctx_size_in - input context
> * ctx_out/ctx_size_out - output context
> 
> The intended use case is to pass some meta data to the test runs that
> operate on skb (this has being brought up on recent LPC).
> 
> For programs that use bpf_prog_test_run_skb, support __sk_buff input and
> output. Initially, from input __sk_buff, copy _only_ cb and priority into
> skb, all other non-zero fields are prohibited (with EINVAL).
> If the user has set ctx_out/ctx_size_out, copy the potentially modified
> __sk_buff back to the userspace.
> 
> We require all fields of input __sk_buff except the ones we explicitly
> support to be set to zero. The expectation is that in the future we might
> add support for more fields and we want to fail explicitly if the user
> runs the program on the kernel where we don't yet support them.
> 
> The API is intentionally vague (i.e. we don't explicitly add __sk_buff
> to bpf_attr, but ctx_in) to potentially let other test_run types use
> this interface in the future (this can be xdp_md for xdp types for
> example).
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  include/uapi/linux/bpf.h |   7 +++
>  kernel/bpf/syscall.c     |   2 +-
>  net/bpf/test_run.c       | 133 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 136 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 837024512baf..8e96f99cebf8 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -396,6 +396,13 @@ union bpf_attr {
>  		__aligned_u64	data_out;
>  		__u32		repeat;
>  		__u32		duration;
> +		__u32		ctx_size_in;	/* input: len of ctx_in */
> +		__u32		ctx_size_out;	/* input/output: len of ctx_out
> +						 *   returns ENOSPC if ctx_out
> +						 *   is too small.
> +						 */
> +		__aligned_u64	ctx_in;
> +		__aligned_u64	ctx_out;
>  	} test;
>  
>  	struct { /* anonymous struct used by BPF_*_GET_*_ID */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 1d65e56594db..4ad754e2943a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1949,7 +1949,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
>  	return cgroup_bpf_prog_query(attr, uattr);
>  }
>  
> -#define BPF_PROG_TEST_RUN_LAST_FIELD test.duration
> +#define BPF_PROG_TEST_RUN_LAST_FIELD test.ctx_out
>  
>  static int bpf_prog_test_run(const union bpf_attr *attr,
>  			     union bpf_attr __user *uattr)
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index fab142b796ef..593912a66d64 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -123,12 +123,121 @@ 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);
> +	u32 size = kattr->test.ctx_size_in;
> +	void *data;
> +
> +	if ((kattr->test.ctx_size_in && !kattr->test.ctx_in) ||
> +	    (!kattr->test.ctx_size_in && kattr->test.ctx_in))
> +		return ERR_PTR(-EINVAL);
> +
> +	if ((kattr->test.ctx_size_out && !kattr->test.ctx_out) ||
> +	    (!kattr->test.ctx_size_out && kattr->test.ctx_out))
> +		return ERR_PTR(-EINVAL);
These attr checks belong to bpf_prog_test_run() in syscall.c.

> +
> +	if (!size)
> +		return NULL;
> +
> +	if (size > max_size)
Not allowed even the tailing (size - max_size) bytes are zero?

> +		return ERR_PTR(-EINVAL);
The convention should be -E2BIG if the userspace passed in something bigger
with tailing non-zero that the kernel cannot understand.

> +
> +	data = kzalloc(max_size, GFP_USER);
> +	if (!data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	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);
> +	u32 copy_size = size;
Is the "u32 copy_size = size;" needed?
I don't see "size" is changed.

> +
> +	if (!kattr->test.ctx_size_out)
> +		return 0;
> +
> +	if (copy_size > kattr->test.ctx_size_out)
If I read both bpf_ctx_init() and btf_ctx_finish() correctly,
the kattr->test.ctx_size_in (input) can be smaller than
sizeof(struct __sk_buff) but the kattr->test.ctx_size_out (output)
cannot be smaller than sizeof(struct __sk_buff)?

> +		return -ENOSPC;
> +
> +	if (data_out && copy_to_user(data_out, data, copy_size))
"data_out" check is not needed.

> +		return -EFAULT;
> +	if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +/**
> + * range_is_zero - test whether buffer is initialized
> + * @buf: buffer to check
> + * @from: check from this position
> + * @to: check up until (excluding) this position
> + *
> + * This function returns true if the there is a non-zero byte
> + * in the buf in the range [from,to).
> + */
> +static inline bool range_is_zero(void *buf, size_t from, size_t to)
> +{
> +	return !memchr_inv((u8 *)buf + from, 0, to - from);
> +}
> +
> +static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
> +{
> +	struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb;
> +
> +	if (!__skb)
> +		return 0;
> +
> +	/* make sure the fields we don't use are zeroed */
> +	if (!range_is_zero(__skb, 0, offsetof(struct __sk_buff, priority)))
> +		return -EINVAL;
> +
> +	/* priority is allowed */
> +
> +	if (!range_is_zero(__skb, offsetof(struct __sk_buff, priority) +
> +			   FIELD_SIZEOF(struct __sk_buff, priority),
> +			   offsetof(struct __sk_buff, cb)))
> +		return -EINVAL;
> +
> +	/* cb is allowed */
> +
> +	if (!range_is_zero(__skb, offsetof(struct __sk_buff, cb) +
> +			   FIELD_SIZEOF(struct __sk_buff, cb),
> +			   sizeof(struct __sk_buff)))
> +		return -EINVAL;
> +
> +	skb->priority = __skb->priority;
> +	memcpy(&cb->data, __skb->cb, QDISC_CB_PRIV_LEN);
> +
> +	return 0;
> +}
> +
> +static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
> +{
> +	struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb;
> +
> +	if (!__skb)
> +		return;
> +
> +	__skb->priority = skb->priority;
> +	memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
> +}
> +
>  int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  			  union bpf_attr __user *uattr)
>  {
>  	bool is_l2 = false, is_direct_pkt_access = false;
>  	u32 size = kattr->test.data_size_in;
>  	u32 repeat = kattr->test.repeat;
> +	struct __sk_buff *ctx = NULL;
>  	u32 retval, duration;
>  	int hh_len = ETH_HLEN;
>  	struct sk_buff *skb;
> @@ -141,6 +250,12 @@ 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));
> +	if (IS_ERR(ctx)) {
> +		kfree(data);
> +		return PTR_ERR(ctx);
> +	}
> +
>  	switch (prog->type) {
>  	case BPF_PROG_TYPE_SCHED_CLS:
>  	case BPF_PROG_TYPE_SCHED_ACT:
> @@ -158,6 +273,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  	sk = kzalloc(sizeof(struct sock), GFP_USER);
>  	if (!sk) {
>  		kfree(data);
> +		kfree(ctx);
>  		return -ENOMEM;
>  	}
>  	sock_net_set(sk, current->nsproxy->net_ns);
> @@ -166,6 +282,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  	skb = build_skb(data, 0);
>  	if (!skb) {
>  		kfree(data);
> +		kfree(ctx);
>  		kfree(sk);
>  		return -ENOMEM;
>  	}
> @@ -180,12 +297,12 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  		__skb_push(skb, hh_len);
>  	if (is_direct_pkt_access)
>  		bpf_compute_data_pointers(skb);
> +	ret = convert___skb_to_skb(skb, ctx);
> +	if (ret)
> +		goto out;
>  	ret = bpf_test_run(prog, skb, repeat, &retval, &duration);
> -	if (ret) {
> -		kfree_skb(skb);
> -		kfree(sk);
> -		return ret;
> -	}
> +	if (ret)
> +		goto out;
>  	if (!is_l2) {
>  		if (skb_headroom(skb) < hh_len) {
>  			int nhead = HH_DATA_ALIGN(hh_len - skb_headroom(skb));
I think the following needs kfree(ctx) also (or goto out).
Copying the context here:

			if (pskb_expand_head(skb, nhead, 0, GFP_USER)) {
				kfree_skb(skb);
				kfree(sk);
				return -ENOMEM;
			}

> @@ -198,14 +315,20 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>  		}
>  		memset(__skb_push(skb, hh_len), 0, hh_len);
>  	}
> +	convert_skb_to___skb(skb, ctx);
>  
>  	size = skb->len;
>  	/* 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));
> +out:
>  	kfree_skb(skb);
>  	kfree(sk);
> +	kfree(ctx);
>  	return ret;
>  }
>  
> -- 
> 2.21.0.392.gf8f6787159e-goog
> 

Re: [PATCH bpf-next 1/3] bpf: support input __sk_buff context in BPF_PROG_TEST_RUN

[Stanislav Fomichev]

On 04/05, Martin Lau wrote:
> On Thu, Apr 04, 2019 at 11:51:31AM -0700, Stanislav Fomichev wrote:
> > Add new set of arguments to bpf_attr for BPF_PROG_TEST_RUN:
> > * ctx_in/ctx_size_in - input context
> > * ctx_out/ctx_size_out - output context
> > 
> > The intended use case is to pass some meta data to the test runs that
> > operate on skb (this has being brought up on recent LPC).
> > 
> > For programs that use bpf_prog_test_run_skb, support __sk_buff input and
> > output. Initially, from input __sk_buff, copy _only_ cb and priority into
> > skb, all other non-zero fields are prohibited (with EINVAL).
> > If the user has set ctx_out/ctx_size_out, copy the potentially modified
> > __sk_buff back to the userspace.
> > 
> > We require all fields of input __sk_buff except the ones we explicitly
> > support to be set to zero. The expectation is that in the future we might
> > add support for more fields and we want to fail explicitly if the user
> > runs the program on the kernel where we don't yet support them.
> > 
> > The API is intentionally vague (i.e. we don't explicitly add __sk_buff
> > to bpf_attr, but ctx_in) to potentially let other test_run types use
> > this interface in the future (this can be xdp_md for xdp types for
> > example).
> > 
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  include/uapi/linux/bpf.h |   7 +++
> >  kernel/bpf/syscall.c     |   2 +-
> >  net/bpf/test_run.c       | 133 +++++++++++++++++++++++++++++++++++++--
> >  3 files changed, 136 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 837024512baf..8e96f99cebf8 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -396,6 +396,13 @@ union bpf_attr {
> >  		__aligned_u64	data_out;
> >  		__u32		repeat;
> >  		__u32		duration;
> > +		__u32		ctx_size_in;	/* input: len of ctx_in */
> > +		__u32		ctx_size_out;	/* input/output: len of ctx_out
> > +						 *   returns ENOSPC if ctx_out
> > +						 *   is too small.
> > +						 */
> > +		__aligned_u64	ctx_in;
> > +		__aligned_u64	ctx_out;
> >  	} test;
> >  
> >  	struct { /* anonymous struct used by BPF_*_GET_*_ID */
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 1d65e56594db..4ad754e2943a 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -1949,7 +1949,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
> >  	return cgroup_bpf_prog_query(attr, uattr);
> >  }
> >  
> > -#define BPF_PROG_TEST_RUN_LAST_FIELD test.duration
> > +#define BPF_PROG_TEST_RUN_LAST_FIELD test.ctx_out
> >  
> >  static int bpf_prog_test_run(const union bpf_attr *attr,
> >  			     union bpf_attr __user *uattr)
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index fab142b796ef..593912a66d64 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -123,12 +123,121 @@ 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);
> > +	u32 size = kattr->test.ctx_size_in;
> > +	void *data;
> > +
> > +	if ((kattr->test.ctx_size_in && !kattr->test.ctx_in) ||
> > +	    (!kattr->test.ctx_size_in && kattr->test.ctx_in))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if ((kattr->test.ctx_size_out && !kattr->test.ctx_out) ||
> > +	    (!kattr->test.ctx_size_out && kattr->test.ctx_out))
> > +		return ERR_PTR(-EINVAL);
> These attr checks belong to bpf_prog_test_run() in syscall.c.
Ack, will move.

> > +
> > +	if (!size)
> > +		return NULL;
> > +
> > +	if (size > max_size)
> Not allowed even the tailing (size - max_size) bytes are zero?
We can do that if you think it makes sense. My intention was
not to unnecessarily complicate it because it's for testing only.

> > +		return ERR_PTR(-EINVAL);
> The convention should be -E2BIG if the userspace passed in something bigger
> with tailing non-zero that the kernel cannot understand.
SGTM.

> > +
> > +	data = kzalloc(max_size, GFP_USER);
> > +	if (!data)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	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);
> > +	u32 copy_size = size;
> Is the "u32 copy_size = size;" needed?
> I don't see "size" is changed.
> 
> > +
> > +	if (!kattr->test.ctx_size_out)
> > +		return 0;
> > +
> > +	if (copy_size > kattr->test.ctx_size_out)
> If I read both bpf_ctx_init() and btf_ctx_finish() correctly,
> the kattr->test.ctx_size_in (input) can be smaller than
> sizeof(struct __sk_buff) but the kattr->test.ctx_size_out (output)
> cannot be smaller than sizeof(struct __sk_buff)?
Yeah, you read that correct. Let me actually do the same as we do in
bpf_test_finish - clamp to the user's requested size (and return
-ENOSPC). That will address you comment about copy_size as well.

> > +		return -ENOSPC;
> > +
> > +	if (data_out && copy_to_user(data_out, data, copy_size))
> "data_out" check is not needed.
Agreed.
> 
> > +		return -EFAULT;
> > +	if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size)))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * range_is_zero - test whether buffer is initialized
> > + * @buf: buffer to check
> > + * @from: check from this position
> > + * @to: check up until (excluding) this position
> > + *
> > + * This function returns true if the there is a non-zero byte
> > + * in the buf in the range [from,to).
> > + */
> > +static inline bool range_is_zero(void *buf, size_t from, size_t to)
> > +{
> > +	return !memchr_inv((u8 *)buf + from, 0, to - from);
> > +}
> > +
> > +static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
> > +{
> > +	struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb;
> > +
> > +	if (!__skb)
> > +		return 0;
> > +
> > +	/* make sure the fields we don't use are zeroed */
> > +	if (!range_is_zero(__skb, 0, offsetof(struct __sk_buff, priority)))
> > +		return -EINVAL;
> > +
> > +	/* priority is allowed */
> > +
> > +	if (!range_is_zero(__skb, offsetof(struct __sk_buff, priority) +
> > +			   FIELD_SIZEOF(struct __sk_buff, priority),
> > +			   offsetof(struct __sk_buff, cb)))
> > +		return -EINVAL;
> > +
> > +	/* cb is allowed */
> > +
> > +	if (!range_is_zero(__skb, offsetof(struct __sk_buff, cb) +
> > +			   FIELD_SIZEOF(struct __sk_buff, cb),
> > +			   sizeof(struct __sk_buff)))
> > +		return -EINVAL;
> > +
> > +	skb->priority = __skb->priority;
> > +	memcpy(&cb->data, __skb->cb, QDISC_CB_PRIV_LEN);
> > +
> > +	return 0;
> > +}
> > +
> > +static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
> > +{
> > +	struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb;
> > +
> > +	if (!__skb)
> > +		return;
> > +
> > +	__skb->priority = skb->priority;
> > +	memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
> > +}
> > +
> >  int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >  			  union bpf_attr __user *uattr)
> >  {
> >  	bool is_l2 = false, is_direct_pkt_access = false;
> >  	u32 size = kattr->test.data_size_in;
> >  	u32 repeat = kattr->test.repeat;
> > +	struct __sk_buff *ctx = NULL;
> >  	u32 retval, duration;
> >  	int hh_len = ETH_HLEN;
> >  	struct sk_buff *skb;
> > @@ -141,6 +250,12 @@ 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));
> > +	if (IS_ERR(ctx)) {
> > +		kfree(data);
> > +		return PTR_ERR(ctx);
> > +	}
> > +
> >  	switch (prog->type) {
> >  	case BPF_PROG_TYPE_SCHED_CLS:
> >  	case BPF_PROG_TYPE_SCHED_ACT:
> > @@ -158,6 +273,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >  	sk = kzalloc(sizeof(struct sock), GFP_USER);
> >  	if (!sk) {
> >  		kfree(data);
> > +		kfree(ctx);
> >  		return -ENOMEM;
> >  	}
> >  	sock_net_set(sk, current->nsproxy->net_ns);
> > @@ -166,6 +282,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >  	skb = build_skb(data, 0);
> >  	if (!skb) {
> >  		kfree(data);
> > +		kfree(ctx);
> >  		kfree(sk);
> >  		return -ENOMEM;
> >  	}
> > @@ -180,12 +297,12 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >  		__skb_push(skb, hh_len);
> >  	if (is_direct_pkt_access)
> >  		bpf_compute_data_pointers(skb);
> > +	ret = convert___skb_to_skb(skb, ctx);
> > +	if (ret)
> > +		goto out;
> >  	ret = bpf_test_run(prog, skb, repeat, &retval, &duration);
> > -	if (ret) {
> > -		kfree_skb(skb);
> > -		kfree(sk);
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		goto out;
> >  	if (!is_l2) {
> >  		if (skb_headroom(skb) < hh_len) {
> >  			int nhead = HH_DATA_ALIGN(hh_len - skb_headroom(skb));
> I think the following needs kfree(ctx) also (or goto out).
> Copying the context here:
> 
> 			if (pskb_expand_head(skb, nhead, 0, GFP_USER)) {
> 				kfree_skb(skb);
> 				kfree(sk);
> 				return -ENOMEM;
> 			}
> 
Ah, good catch, not sure how I missed that :-(

Thank you for a review, will follow up with a v2!
> > @@ -198,14 +315,20 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >  		}
> >  		memset(__skb_push(skb, hh_len), 0, hh_len);
> >  	}
> > +	convert_skb_to___skb(skb, ctx);
> >  
> >  	size = skb->len;
> >  	/* 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));
> > +out:
> >  	kfree_skb(skb);
> >  	kfree(sk);
> > +	kfree(ctx);
> >  	return ret;
> >  }
> >  
> > -- 
> > 2.21.0.392.gf8f6787159e-goog
> >