[bpf-next PATCH v5 0/5] bpf: Add sk_msg and networking helpers

[bpf-next PATCH v5 0/5] bpf: Add sk_msg and networking helpers

[John Fastabend]

This series adds helpers for sk_msg program type and based on feedback
from v1 adds *_task_* helpers and probe_* helpers to all networking
programs with perfmon_capable() capabilities.

The list of helpers breaks down as follows,

Networking with perfmon_capable() guard (patch2):

 BPF_FUNC_get_current_task
 BPF_FUNC_probe_read_user
 BPF_FUNC_probe_read_kernel
 BPF_FUNC_probe_read_user_str
 BPF_FUNC_probe_read_kernel_str

Added to sk_msg program types (patch1,3):

 BPF_FUNC_perf_event_output
 BPF_FUNC_get_current_uid_gid
 BPF_FUNC_get_current_pid_tgid
 BPF_FUNC_get_current_cgroup_id
 BPF_FUNC_get_current_ancestor_cgroup_id
 BPF_FUNC_get_cgroup_classid

 BPF_FUNC_sk_storage_get
 BPF_FUNC_sk_storage_delete

For testing we create two tests. One specifically for the sk_msg
program types which encodes a common pattern we use to test verifier
logic now and as the verifier evolves.

Next we have skb classifier test. This uses the test run infra to
run a test which uses the get_current_task, current_task_under_cgroup,
probe_read_kernel, and probe_reak_kernel_str.

Note we dropped the old probe_read variants probe_read() and
probe_read_str() in v2.

v4->v5:
 Remove BPF_FUNC_current_task_under_cgroup because it requires a
 valid current and at least at the moment seems less usable in all
 contexts. It also probably doesn't need to be guarded by perfoman_cap.
 We can add it on a per type basis when its needed or decide later
 after some more experience that its universally useful.

v3->v4:
 patch4, remove macros and put code inline, add test cleanup, remove
 version in bpf program.
 patch5, use ctask returned from task_under_cgroup so that we avoid
 any potential compiler warnings, add test cleanup, use BTF style
 maps.

v2->v3:
 Pulled header update of tools sk_msg_md{} structure into patch3 for
 easier review. ACKs from Yonghong pushed into v3

v1->v2:
 Pulled generic helpers *current_task* and probe_* into the
 base func helper so they can be used more widely in networking scope.
 BPF capabilities patch is now in bpf-next so use perfmon_capable() check
 instead of CAP_SYS_ADMIN.

 Drop old probe helpers, probe_read() and probe_read_str()

 Added tests.

 Thanks to Daniel, Yonghong, and Andrii for review and feedback.

---

John Fastabend (5):
      bpf, sk_msg: add some generic helpers that may be useful from sk_msg
      bpf: extend bpf_base_func_proto helpers with probe_* and *current_task*
      bpf, sk_msg: add get socket storage helpers
      bpf, selftests: add sk_msg helpers load and attach test
      bpf, selftests: test probe_* helpers from SCHED_CLS


 include/uapi/linux/bpf.h                           |    2 +
 kernel/bpf/helpers.c                               |   24 ++++++++++
 kernel/trace/bpf_trace.c                           |   10 ++--
 net/core/filter.c                                  |   31 +++++++++++++
 tools/include/uapi/linux/bpf.h                     |    2 +
 .../testing/selftests/bpf/prog_tests/skb_helpers.c |   30 +++++++++++++
 .../selftests/bpf/prog_tests/sockmap_basic.c       |   35 +++++++++++++++
 .../testing/selftests/bpf/progs/test_skb_helpers.c |   28 ++++++++++++
 .../selftests/bpf/progs/test_skmsg_load_helpers.c  |   47 ++++++++++++++++++++
 9 files changed, 204 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/skb_helpers.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_skb_helpers.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c

--
Signature

[bpf-next PATCH v5 1/5] bpf, sk_msg: add some generic helpers that may be useful from sk_msg

[John Fastabend]

Add these generic helpers that may be useful to use from sk_msg programs.
The helpers do not depend on ctx so we can simply add them here,

 BPF_FUNC_perf_event_output
 BPF_FUNC_get_current_uid_gid
 BPF_FUNC_get_current_pid_tgid
 BPF_FUNC_get_current_comm
 BPF_FUNC_get_current_cgroup_id
 BPF_FUNC_get_current_ancestor_cgroup_id
 BPF_FUNC_get_cgroup_classid

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/core/filter.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 822d662..a56046a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6443,6 +6443,22 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_msg_push_data_proto;
 	case BPF_FUNC_msg_pop_data:
 		return &bpf_msg_pop_data_proto;
+	case BPF_FUNC_perf_event_output:
+		return &bpf_event_output_data_proto;
+	case BPF_FUNC_get_current_uid_gid:
+		return &bpf_get_current_uid_gid_proto;
+	case BPF_FUNC_get_current_pid_tgid:
+		return &bpf_get_current_pid_tgid_proto;
+#ifdef CONFIG_CGROUPS
+	case BPF_FUNC_get_current_cgroup_id:
+		return &bpf_get_current_cgroup_id_proto;
+	case BPF_FUNC_get_current_ancestor_cgroup_id:
+		return &bpf_get_current_ancestor_cgroup_id_proto;
+#endif
+#ifdef CONFIG_CGROUP_NET_CLASSID
+	case BPF_FUNC_get_cgroup_classid:
+		return &bpf_get_cgroup_classid_curr_proto;
+#endif
 	default:
 		return bpf_base_func_proto(func_id);
 	}

[bpf-next PATCH v5 2/5] bpf: extend bpf_base_func_proto helpers with probe_* and *current_task*

[John Fastabend]

Often it is useful when applying policy to know something about the
task. If the administrator has CAP_SYS_ADMIN rights then they can
use kprobe + networking hook and link the two programs together to
accomplish this. However, this is a bit clunky and also means we have
to call both the network program and kprobe program when we could just
use a single program and avoid passing metadata through sk_msg/skb->cb,
socket, maps, etc.

To accomplish this add probe_* helpers to bpf_base_func_proto programs
guarded by a perfmon_capable() check. New supported helpers are the
following,

 BPF_FUNC_get_current_task
 BPF_FUNC_current_task_under_cgroup
 BPF_FUNC_probe_read_user
 BPF_FUNC_probe_read_kernel
 BPF_FUNC_probe_read_user_str
 BPF_FUNC_probe_read_kernel_str

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/helpers.c     |   24 ++++++++++++++++++++++++
 kernel/trace/bpf_trace.c |   10 +++++-----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 886949f..bb4fb63 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -601,6 +601,12 @@ const struct bpf_func_proto bpf_event_output_data_proto =  {
 	.arg5_type      = ARG_CONST_SIZE_OR_ZERO,
 };
 
+const struct bpf_func_proto bpf_get_current_task_proto __weak;
+const struct bpf_func_proto bpf_probe_read_user_proto __weak;
+const struct bpf_func_proto bpf_probe_read_user_str_proto __weak;
+const struct bpf_func_proto bpf_probe_read_kernel_proto __weak;
+const struct bpf_func_proto bpf_probe_read_kernel_str_proto __weak;
+
 const struct bpf_func_proto *
 bpf_base_func_proto(enum bpf_func_id func_id)
 {
@@ -648,6 +654,24 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 	case BPF_FUNC_jiffies64:
 		return &bpf_jiffies64_proto;
 	default:
+		break;
+	}
+
+	if (!perfmon_capable())
+		return NULL;
+
+	switch (func_id) {
+	case BPF_FUNC_get_current_task:
+		return &bpf_get_current_task_proto;
+	case BPF_FUNC_probe_read_user:
+		return &bpf_probe_read_user_proto;
+	case BPF_FUNC_probe_read_kernel:
+		return &bpf_probe_read_kernel_proto;
+	case BPF_FUNC_probe_read_user_str:
+		return &bpf_probe_read_user_str_proto;
+	case BPF_FUNC_probe_read_kernel_str:
+		return &bpf_probe_read_kernel_str_proto;
+	default:
 		return NULL;
 	}
 }
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 9531f54..187cd69 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -147,7 +147,7 @@ BPF_CALL_3(bpf_probe_read_user, void *, dst, u32, size,
 	return ret;
 }
 
-static const struct bpf_func_proto bpf_probe_read_user_proto = {
+const struct bpf_func_proto bpf_probe_read_user_proto = {
 	.func		= bpf_probe_read_user,
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
@@ -167,7 +167,7 @@ BPF_CALL_3(bpf_probe_read_user_str, void *, dst, u32, size,
 	return ret;
 }
 
-static const struct bpf_func_proto bpf_probe_read_user_str_proto = {
+const struct bpf_func_proto bpf_probe_read_user_str_proto = {
 	.func		= bpf_probe_read_user_str,
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
@@ -198,7 +198,7 @@ BPF_CALL_3(bpf_probe_read_kernel, void *, dst, u32, size,
 	return bpf_probe_read_kernel_common(dst, size, unsafe_ptr, false);
 }
 
-static const struct bpf_func_proto bpf_probe_read_kernel_proto = {
+const struct bpf_func_proto bpf_probe_read_kernel_proto = {
 	.func		= bpf_probe_read_kernel,
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
@@ -253,7 +253,7 @@ BPF_CALL_3(bpf_probe_read_kernel_str, void *, dst, u32, size,
 	return bpf_probe_read_kernel_str_common(dst, size, unsafe_ptr, false);
 }
 
-static const struct bpf_func_proto bpf_probe_read_kernel_str_proto = {
+const struct bpf_func_proto bpf_probe_read_kernel_str_proto = {
 	.func		= bpf_probe_read_kernel_str,
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
@@ -907,7 +907,7 @@ BPF_CALL_0(bpf_get_current_task)
 	return (long) current;
 }
 
-static const struct bpf_func_proto bpf_get_current_task_proto = {
+const struct bpf_func_proto bpf_get_current_task_proto = {
 	.func		= bpf_get_current_task,
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,

[bpf-next PATCH v5 3/5] bpf, sk_msg: add get socket storage helpers

[John Fastabend]

Add helpers to use local socket storage.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
 include/uapi/linux/bpf.h       |    2 ++
 net/core/filter.c              |   15 +++++++++++++++
 tools/include/uapi/linux/bpf.h |    2 ++
 3 files changed, 19 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b9b8a0f..d394b09 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3641,6 +3641,8 @@ struct sk_msg_md {
 	__u32 remote_port;	/* Stored in network byte order */
 	__u32 local_port;	/* stored in host byte order */
 	__u32 size;		/* Total size of sk_msg */
+
+	__bpf_md_ptr(struct bpf_sock *, sk); /* current socket */
 };
 
 struct sk_reuseport_md {
diff --git a/net/core/filter.c b/net/core/filter.c
index a56046a..48b499f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6449,6 +6449,10 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_get_current_uid_gid_proto;
 	case BPF_FUNC_get_current_pid_tgid:
 		return &bpf_get_current_pid_tgid_proto;
+	case BPF_FUNC_sk_storage_get:
+		return &bpf_sk_storage_get_proto;
+	case BPF_FUNC_sk_storage_delete:
+		return &bpf_sk_storage_delete_proto;
 #ifdef CONFIG_CGROUPS
 	case BPF_FUNC_get_current_cgroup_id:
 		return &bpf_get_current_cgroup_id_proto;
@@ -7269,6 +7273,11 @@ static bool sk_msg_is_valid_access(int off, int size,
 		if (size != sizeof(__u64))
 			return false;
 		break;
+	case offsetof(struct sk_msg_md, sk):
+		if (size != sizeof(__u64))
+			return false;
+		info->reg_type = PTR_TO_SOCKET;
+		break;
 	case bpf_ctx_range(struct sk_msg_md, family):
 	case bpf_ctx_range(struct sk_msg_md, remote_ip4):
 	case bpf_ctx_range(struct sk_msg_md, local_ip4):
@@ -8605,6 +8614,12 @@ static u32 sk_msg_convert_ctx_access(enum bpf_access_type type,
 				      si->dst_reg, si->src_reg,
 				      offsetof(struct sk_msg_sg, size));
 		break;
+
+	case offsetof(struct sk_msg_md, sk):
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_msg, sk),
+				      si->dst_reg, si->src_reg,
+				      offsetof(struct sk_msg, sk));
+		break;
 	}
 
 	return insn - insn_buf;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 146c742..b95bb16 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3641,6 +3641,8 @@ struct sk_msg_md {
 	__u32 remote_port;	/* Stored in network byte order */
 	__u32 local_port;	/* stored in host byte order */
 	__u32 size;		/* Total size of sk_msg */
+
+	__bpf_md_ptr(struct bpf_sock *, sk); /* current socket */
 };
 
 struct sk_reuseport_md {

[bpf-next PATCH v5 4/5] bpf, selftests: add sk_msg helpers load and attach test

[John Fastabend]

The test itself is not particularly useful but it encodes a common
pattern we have.

Namely do a sk storage lookup then depending on data here decide if
we need to do more work or alternatively allow packet to PASS. Then
if we need to do more work consult task_struct for more information
about the running task. Finally based on this additional information
drop or pass the data. In this case the suspicious check is not so
realisitic but it encodes the general pattern and uses the helpers
so we test the workflow.

This is a load test to ensure verifier correctly handles this case.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c       |   35 +++++++++++++++
 .../selftests/bpf/progs/test_skmsg_load_helpers.c  |   47 ++++++++++++++++++++
 2 files changed, 82 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index aa43e0b..96e7b7f 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -1,7 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2020 Cloudflare
+#include <error.h>
 
 #include "test_progs.h"
+#include "test_skmsg_load_helpers.skel.h"
 
 #define TCP_REPAIR		19	/* TCP sock is under repair right now */
 
@@ -70,10 +72,43 @@ static void test_sockmap_create_update_free(enum bpf_map_type map_type)
 	close(s);
 }
 
+static void test_skmsg_helpers(enum bpf_map_type map_type)
+{
+	struct test_skmsg_load_helpers *skel;
+	int err, map, verdict;
+
+	skel = test_skmsg_load_helpers__open_and_load();
+	if (CHECK_FAIL(!skel)) {
+		perror("test_skmsg_load_helpers__open_and_load");
+		return;
+	}
+
+	verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
+	map = bpf_map__fd(skel->maps.sock_map);
+
+	err = bpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
+	if (CHECK_FAIL(err)) {
+		perror("bpf_prog_attach");
+		goto out;
+	}
+
+	err = bpf_prog_detach2(verdict, map, BPF_SK_MSG_VERDICT);
+	if (CHECK_FAIL(err)) {
+		perror("bpf_prog_detach2");
+		goto out;
+	}
+out:
+	test_skmsg_load_helpers__destroy(skel);
+}
+
 void test_sockmap_basic(void)
 {
 	if (test__start_subtest("sockmap create_update_free"))
 		test_sockmap_create_update_free(BPF_MAP_TYPE_SOCKMAP);
 	if (test__start_subtest("sockhash create_update_free"))
 		test_sockmap_create_update_free(BPF_MAP_TYPE_SOCKHASH);
+	if (test__start_subtest("sockmap sk_msg load helpers"))
+		test_skmsg_helpers(BPF_MAP_TYPE_SOCKMAP);
+	if (test__start_subtest("sockhash sk_msg load helpers"))
+		test_skmsg_helpers(BPF_MAP_TYPE_SOCKHASH);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c b/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
new file mode 100644
index 0000000..45e8fc7
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Isovalent, Inc.
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKMAP);
+	__uint(max_entries, 2);
+	__type(key, __u32);
+	__type(value, __u64);
+} sock_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKHASH);
+	__uint(max_entries, 2);
+	__type(key, __u32);
+	__type(value, __u64);
+} sock_hash SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SK_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, __u32);
+	__type(value, __u64);
+} socket_storage SEC(".maps");
+
+SEC("sk_msg")
+int prog_msg_verdict(struct sk_msg_md *msg)
+{
+	struct task_struct *task = (struct task_struct *)bpf_get_current_task();
+	int verdict = SK_PASS;
+	__u32 pid, tpid;
+	__u64 *sk_stg;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	sk_stg = bpf_sk_storage_get(&socket_storage, msg->sk, 0, BPF_SK_STORAGE_GET_F_CREATE);
+	if (!sk_stg)
+		return SK_DROP;
+	*sk_stg = pid;
+	bpf_probe_read_kernel(&tpid , sizeof(tpid), &task->tgid);
+	if (pid != tpid)
+		verdict = SK_DROP;
+	bpf_sk_storage_delete(&socket_storage, (void *)msg->sk);
+	return verdict;
+}
+
+char _license[] SEC("license") = "GPL";

[bpf-next PATCH v5 5/5] bpf, selftests: test probe_* helpers from SCHED_CLS

[John Fastabend]

Lets test using probe* in SCHED_CLS network programs as well just
to be sure these keep working. Its cheap to add the extra test
and provides a second context to test outside of sk_msg after
we generalized probe* helpers to all networking types.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 .../testing/selftests/bpf/prog_tests/skb_helpers.c |   30 ++++++++++++++++++++
 .../testing/selftests/bpf/progs/test_skb_helpers.c |   28 +++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/skb_helpers.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_skb_helpers.c

diff --git a/tools/testing/selftests/bpf/prog_tests/skb_helpers.c b/tools/testing/selftests/bpf/prog_tests/skb_helpers.c
new file mode 100644
index 0000000..f302ad8
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/skb_helpers.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+
+void test_skb_helpers(void)
+{
+	struct __sk_buff skb = {
+		.wire_len = 100,
+		.gso_segs = 8,
+		.gso_size = 10,
+	};
+	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;
+
+	err = bpf_prog_load("./test_skb_helpers.o", BPF_PROG_TYPE_SCHED_CLS, &obj,
+			    &tattr.prog_fd);
+	if (CHECK_ATTR(err, "load", "err %d errno %d\n", err, errno))
+		return;
+	err = bpf_prog_test_run_xattr(&tattr);
+	CHECK_ATTR(err, "len", "err %d errno %d\n", err, errno);
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_skb_helpers.c b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
new file mode 100644
index 0000000..bb3fbf1
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+#define TEST_COMM_LEN 16
+
+struct {
+	__uint(type, BPF_MAP_TYPE_CGROUP_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, u32);
+	__type(value, u32);
+} cgroup_map SEC(".maps");
+
+char _license[] SEC("license") = "GPL";
+
+SEC("classifier/test_skb_helpers")
+int test_skb_helpers(struct __sk_buff *skb)
+{
+	struct task_struct *task;
+	char comm[TEST_COMM_LEN];
+	__u32 tpid;
+
+	task = (struct task_struct *)bpf_get_current_task();
+	bpf_probe_read_kernel(&tpid , sizeof(tpid), &task->tgid);
+	bpf_probe_read_kernel_str(&comm, sizeof(comm), &task->comm);
+	return 0;
+}

Re: [bpf-next PATCH v5 1/5] bpf, sk_msg: add some generic helpers that may be useful from sk_msg

[Daniel Borkmann]

On 5/24/20 6:50 PM, John Fastabend wrote:
> Add these generic helpers that may be useful to use from sk_msg programs.
> The helpers do not depend on ctx so we can simply add them here,
> 
>   BPF_FUNC_perf_event_output
>   BPF_FUNC_get_current_uid_gid
>   BPF_FUNC_get_current_pid_tgid
>   BPF_FUNC_get_current_comm

Hmm, added helpers below are what you list here except get_current_comm.
Was this forgotten to be added here?

>   BPF_FUNC_get_current_cgroup_id
>   BPF_FUNC_get_current_ancestor_cgroup_id
>   BPF_FUNC_get_cgroup_classid
> 
> Acked-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
>   net/core/filter.c |   16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 822d662..a56046a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6443,6 +6443,22 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>   		return &bpf_msg_push_data_proto;
>   	case BPF_FUNC_msg_pop_data:
>   		return &bpf_msg_pop_data_proto;
> +	case BPF_FUNC_perf_event_output:
> +		return &bpf_event_output_data_proto;
> +	case BPF_FUNC_get_current_uid_gid:
> +		return &bpf_get_current_uid_gid_proto;
> +	case BPF_FUNC_get_current_pid_tgid:
> +		return &bpf_get_current_pid_tgid_proto;
> +#ifdef CONFIG_CGROUPS
> +	case BPF_FUNC_get_current_cgroup_id:
> +		return &bpf_get_current_cgroup_id_proto;
> +	case BPF_FUNC_get_current_ancestor_cgroup_id:
> +		return &bpf_get_current_ancestor_cgroup_id_proto;
> +#endif
> +#ifdef CONFIG_CGROUP_NET_CLASSID
> +	case BPF_FUNC_get_cgroup_classid:
> +		return &bpf_get_cgroup_classid_curr_proto;
> +#endif
>   	default:
>   		return bpf_base_func_proto(func_id);
>   	}
> 

Re: [bpf-next PATCH v5 2/5] bpf: extend bpf_base_func_proto helpers with probe_* and *current_task*

[Daniel Borkmann]

On 5/24/20 6:50 PM, John Fastabend wrote:
> Often it is useful when applying policy to know something about the
> task. If the administrator has CAP_SYS_ADMIN rights then they can
> use kprobe + networking hook and link the two programs together to
> accomplish this. However, this is a bit clunky and also means we have
> to call both the network program and kprobe program when we could just
> use a single program and avoid passing metadata through sk_msg/skb->cb,
> socket, maps, etc.
> 
> To accomplish this add probe_* helpers to bpf_base_func_proto programs
> guarded by a perfmon_capable() check. New supported helpers are the
> following,
> 
>   BPF_FUNC_get_current_task
>   BPF_FUNC_current_task_under_cgroup

Nit: Stale commit message?

>   BPF_FUNC_probe_read_user
>   BPF_FUNC_probe_read_kernel
>   BPF_FUNC_probe_read_user_str
>   BPF_FUNC_probe_read_kernel_str
> 
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> Acked-by: Yonghong Song <yhs@fb.com>
> ---
>   kernel/bpf/helpers.c     |   24 ++++++++++++++++++++++++
>   kernel/trace/bpf_trace.c |   10 +++++-----
>   2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 886949f..bb4fb63 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -601,6 +601,12 @@ const struct bpf_func_proto bpf_event_output_data_proto =  {
>   	.arg5_type      = ARG_CONST_SIZE_OR_ZERO,
>   };
>   
> +const struct bpf_func_proto bpf_get_current_task_proto __weak;
> +const struct bpf_func_proto bpf_probe_read_user_proto __weak;
> +const struct bpf_func_proto bpf_probe_read_user_str_proto __weak;
> +const struct bpf_func_proto bpf_probe_read_kernel_proto __weak;
> +const struct bpf_func_proto bpf_probe_read_kernel_str_proto __weak;
> +
>   const struct bpf_func_proto *
>   bpf_base_func_proto(enum bpf_func_id func_id)
>   {
> @@ -648,6 +654,24 @@ bpf_base_func_proto(enum bpf_func_id func_id)
>   	case BPF_FUNC_jiffies64:
>   		return &bpf_jiffies64_proto;
>   	default:
> +		break;
> +	}
> +
> +	if (!perfmon_capable())
> +		return NULL;
> +
> +	switch (func_id) {
> +	case BPF_FUNC_get_current_task:
> +		return &bpf_get_current_task_proto;
> +	case BPF_FUNC_probe_read_user:
> +		return &bpf_probe_read_user_proto;
> +	case BPF_FUNC_probe_read_kernel:
> +		return &bpf_probe_read_kernel_proto;
> +	case BPF_FUNC_probe_read_user_str:
> +		return &bpf_probe_read_user_str_proto;
> +	case BPF_FUNC_probe_read_kernel_str:
> +		return &bpf_probe_read_kernel_str_proto;
> +	default:
>   		return NULL;
>   	}
>   }
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9531f54..187cd69 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -147,7 +147,7 @@ BPF_CALL_3(bpf_probe_read_user, void *, dst, u32, size,
>   	return ret;
>   }
>   
> -static const struct bpf_func_proto bpf_probe_read_user_proto = {
> +const struct bpf_func_proto bpf_probe_read_user_proto = {
>   	.func		= bpf_probe_read_user,
>   	.gpl_only	= true,
>   	.ret_type	= RET_INTEGER,
> @@ -167,7 +167,7 @@ BPF_CALL_3(bpf_probe_read_user_str, void *, dst, u32, size,
>   	return ret;
>   }
>   
> -static const struct bpf_func_proto bpf_probe_read_user_str_proto = {
> +const struct bpf_func_proto bpf_probe_read_user_str_proto = {
>   	.func		= bpf_probe_read_user_str,
>   	.gpl_only	= true,
>   	.ret_type	= RET_INTEGER,
> @@ -198,7 +198,7 @@ BPF_CALL_3(bpf_probe_read_kernel, void *, dst, u32, size,
>   	return bpf_probe_read_kernel_common(dst, size, unsafe_ptr, false);
>   }
>   
> -static const struct bpf_func_proto bpf_probe_read_kernel_proto = {
> +const struct bpf_func_proto bpf_probe_read_kernel_proto = {
>   	.func		= bpf_probe_read_kernel,
>   	.gpl_only	= true,
>   	.ret_type	= RET_INTEGER,
> @@ -253,7 +253,7 @@ BPF_CALL_3(bpf_probe_read_kernel_str, void *, dst, u32, size,
>   	return bpf_probe_read_kernel_str_common(dst, size, unsafe_ptr, false);
>   }
>   
> -static const struct bpf_func_proto bpf_probe_read_kernel_str_proto = {
> +const struct bpf_func_proto bpf_probe_read_kernel_str_proto = {
>   	.func		= bpf_probe_read_kernel_str,
>   	.gpl_only	= true,
>   	.ret_type	= RET_INTEGER,
> @@ -907,7 +907,7 @@ BPF_CALL_0(bpf_get_current_task)
>   	return (long) current;
>   }
>   
> -static const struct bpf_func_proto bpf_get_current_task_proto = {
> +const struct bpf_func_proto bpf_get_current_task_proto = {
>   	.func		= bpf_get_current_task,
>   	.gpl_only	= true,
>   	.ret_type	= RET_INTEGER,
> 

Re: [bpf-next PATCH v5 1/5] bpf, sk_msg: add some generic helpers that may be useful from sk_msg

[John Fastabend]

Daniel Borkmann wrote:
> On 5/24/20 6:50 PM, John Fastabend wrote:
> > Add these generic helpers that may be useful to use from sk_msg programs.
> > The helpers do not depend on ctx so we can simply add them here,
> > 
> >   BPF_FUNC_perf_event_output
> >   BPF_FUNC_get_current_uid_gid
> >   BPF_FUNC_get_current_pid_tgid
> >   BPF_FUNC_get_current_comm
> 
> Hmm, added helpers below are what you list here except get_current_comm.
> Was this forgotten to be added here?

Forgot to update commit messages. I dropped it because it wasn't clear to
me it was very useful or how I would use it from this context. I figure we
can add it later if its needed.

> 
> >   BPF_FUNC_get_current_cgroup_id
> >   BPF_FUNC_get_current_ancestor_cgroup_id
> >   BPF_FUNC_get_cgroup_classid
> > 
> > Acked-by: Yonghong Song <yhs@fb.com>
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
> >   net/core/filter.c |   16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> > 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 822d662..a56046a 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -6443,6 +6443,22 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >   		return &bpf_msg_push_data_proto;
> >   	case BPF_FUNC_msg_pop_data:
> >   		return &bpf_msg_pop_data_proto;
> > +	case BPF_FUNC_perf_event_output:
> > +		return &bpf_event_output_data_proto;
> > +	case BPF_FUNC_get_current_uid_gid:
> > +		return &bpf_get_current_uid_gid_proto;
> > +	case BPF_FUNC_get_current_pid_tgid:
> > +		return &bpf_get_current_pid_tgid_proto;
> > +#ifdef CONFIG_CGROUPS
> > +	case BPF_FUNC_get_current_cgroup_id:
> > +		return &bpf_get_current_cgroup_id_proto;
> > +	case BPF_FUNC_get_current_ancestor_cgroup_id:
> > +		return &bpf_get_current_ancestor_cgroup_id_proto;
> > +#endif
> > +#ifdef CONFIG_CGROUP_NET_CLASSID
> > +	case BPF_FUNC_get_cgroup_classid:
> > +		return &bpf_get_cgroup_classid_curr_proto;
> > +#endif
> >   	default:
> >   		return bpf_base_func_proto(func_id);
> >   	}
> > 
> 

Re: [bpf-next PATCH v5 2/5] bpf: extend bpf_base_func_proto helpers with probe_* and *current_task*

[John Fastabend]

Daniel Borkmann wrote:
> On 5/24/20 6:50 PM, John Fastabend wrote:
> > Often it is useful when applying policy to know something about the
> > task. If the administrator has CAP_SYS_ADMIN rights then they can
> > use kprobe + networking hook and link the two programs together to
> > accomplish this. However, this is a bit clunky and also means we have
> > to call both the network program and kprobe program when we could just
> > use a single program and avoid passing metadata through sk_msg/skb->cb,
> > socket, maps, etc.
> > 
> > To accomplish this add probe_* helpers to bpf_base_func_proto programs
> > guarded by a perfmon_capable() check. New supported helpers are the
> > following,
> > 
> >   BPF_FUNC_get_current_task
> >   BPF_FUNC_current_task_under_cgroup
> 
> Nit: Stale commit message?
> 

Correct, stale commit.

> >   BPF_FUNC_probe_read_user
> >   BPF_FUNC_probe_read_kernel
> >   BPF_FUNC_probe_read_user_str
> >   BPF_FUNC_probe_read_kernel_str
> > 
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > Acked-by: Yonghong Song <yhs@fb.com>
> > ---

[...]

Re: [bpf-next PATCH v5 4/5] bpf, selftests: add sk_msg helpers load and attach test

[Andrii Nakryiko]

On Sun, May 24, 2020 at 9:51 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> The test itself is not particularly useful but it encodes a common
> pattern we have.
>
> Namely do a sk storage lookup then depending on data here decide if
> we need to do more work or alternatively allow packet to PASS. Then
> if we need to do more work consult task_struct for more information
> about the running task. Finally based on this additional information
> drop or pass the data. In this case the suspicious check is not so
> realisitic but it encodes the general pattern and uses the helpers
> so we test the workflow.
>
> This is a load test to ensure verifier correctly handles this case.
>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---

Other than perror and CHECK_FAIL nag below, looks good:

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  .../selftests/bpf/prog_tests/sockmap_basic.c       |   35 +++++++++++++++
>  .../selftests/bpf/progs/test_skmsg_load_helpers.c  |   47 ++++++++++++++++++++
>  2 files changed, 82 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index aa43e0b..96e7b7f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (c) 2020 Cloudflare
> +#include <error.h>
>
>  #include "test_progs.h"
> +#include "test_skmsg_load_helpers.skel.h"
>
>  #define TCP_REPAIR             19      /* TCP sock is under repair right now */
>
> @@ -70,10 +72,43 @@ static void test_sockmap_create_update_free(enum bpf_map_type map_type)
>         close(s);
>  }
>
> +static void test_skmsg_helpers(enum bpf_map_type map_type)
> +{
> +       struct test_skmsg_load_helpers *skel;
> +       int err, map, verdict;
> +
> +       skel = test_skmsg_load_helpers__open_and_load();
> +       if (CHECK_FAIL(!skel)) {
> +               perror("test_skmsg_load_helpers__open_and_load");

All test_progs tests use CHECK() macro to test and emit error message
on error, so no need to do silent CHECK_FAIL() and then perror(). Same
below in few places. I don't think you need to send v6 just for this,
but please follow up with a clean up.

> +               return;
> +       }
> +
> +       verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
> +       map = bpf_map__fd(skel->maps.sock_map);
> +
> +       err = bpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
> +       if (CHECK_FAIL(err)) {
> +               perror("bpf_prog_attach");
> +               goto out;
> +       }
> +
> +       err = bpf_prog_detach2(verdict, map, BPF_SK_MSG_VERDICT);
> +       if (CHECK_FAIL(err)) {
> +               perror("bpf_prog_detach2");
> +               goto out;
> +       }
> +out:
> +       test_skmsg_load_helpers__destroy(skel);
> +}
> +

[...]

Re: [bpf-next PATCH v5 5/5] bpf, selftests: test probe_* helpers from SCHED_CLS

[Andrii Nakryiko]

On Sun, May 24, 2020 at 9:52 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Lets test using probe* in SCHED_CLS network programs as well just
> to be sure these keep working. Its cheap to add the extra test
> and provides a second context to test outside of sk_msg after
> we generalized probe* helpers to all networking types.
>
> Acked-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---

One day we need to just replace all bpf_prog_load() calls with
bpf_object__open() or skeleton open/load. But not today :)

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  .../testing/selftests/bpf/prog_tests/skb_helpers.c |   30 ++++++++++++++++++++
>  .../testing/selftests/bpf/progs/test_skb_helpers.c |   28 +++++++++++++++++++
>  2 files changed, 58 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/skb_helpers.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_skb_helpers.c
>

[...]

Re: [bpf-next PATCH v5 1/5] bpf, sk_msg: add some generic helpers that may be useful from sk_msg

[Andrii Nakryiko]

On Mon, May 25, 2020 at 3:57 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Daniel Borkmann wrote:
> > On 5/24/20 6:50 PM, John Fastabend wrote:
> > > Add these generic helpers that may be useful to use from sk_msg programs.
> > > The helpers do not depend on ctx so we can simply add them here,
> > >
> > >   BPF_FUNC_perf_event_output
> > >   BPF_FUNC_get_current_uid_gid
> > >   BPF_FUNC_get_current_pid_tgid
> > >   BPF_FUNC_get_current_comm
> >
> > Hmm, added helpers below are what you list here except get_current_comm.
> > Was this forgotten to be added here?
>
> Forgot to update commit messages. I dropped it because it wasn't clear to
> me it was very useful or how I would use it from this context. I figure we
> can add it later if its needed.

But it's also not harmful in any way and is in a similar group as
get_current_pid_tgid. So let's add it sooner rather than later. There
is no cost in allowing this, right?

>
> >
> > >   BPF_FUNC_get_current_cgroup_id
> > >   BPF_FUNC_get_current_ancestor_cgroup_id
> > >   BPF_FUNC_get_cgroup_classid
> > >
> > > Acked-by: Yonghong Song <yhs@fb.com>
> > > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > > ---
> > >   net/core/filter.c |   16 ++++++++++++++++
> > >   1 file changed, 16 insertions(+)
> > >
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index 822d662..a56046a 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -6443,6 +6443,22 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > >             return &bpf_msg_push_data_proto;
> > >     case BPF_FUNC_msg_pop_data:
> > >             return &bpf_msg_pop_data_proto;
> > > +   case BPF_FUNC_perf_event_output:
> > > +           return &bpf_event_output_data_proto;
> > > +   case BPF_FUNC_get_current_uid_gid:
> > > +           return &bpf_get_current_uid_gid_proto;
> > > +   case BPF_FUNC_get_current_pid_tgid:
> > > +           return &bpf_get_current_pid_tgid_proto;
> > > +#ifdef CONFIG_CGROUPS
> > > +   case BPF_FUNC_get_current_cgroup_id:
> > > +           return &bpf_get_current_cgroup_id_proto;
> > > +   case BPF_FUNC_get_current_ancestor_cgroup_id:
> > > +           return &bpf_get_current_ancestor_cgroup_id_proto;
> > > +#endif
> > > +#ifdef CONFIG_CGROUP_NET_CLASSID
> > > +   case BPF_FUNC_get_cgroup_classid:
> > > +           return &bpf_get_cgroup_classid_curr_proto;
> > > +#endif
> > >     default:
> > >             return bpf_base_func_proto(func_id);
> > >     }
> > >
> >
>
>

Re: [bpf-next PATCH v5 1/5] bpf, sk_msg: add some generic helpers that may be useful from sk_msg

[John Fastabend]

Andrii Nakryiko wrote:
> On Mon, May 25, 2020 at 3:57 PM John Fastabend <john.fastabend@gmail.com> wrote:
> >
> > Daniel Borkmann wrote:
> > > On 5/24/20 6:50 PM, John Fastabend wrote:
> > > > Add these generic helpers that may be useful to use from sk_msg programs.
> > > > The helpers do not depend on ctx so we can simply add them here,
> > > >
> > > >   BPF_FUNC_perf_event_output
> > > >   BPF_FUNC_get_current_uid_gid
> > > >   BPF_FUNC_get_current_pid_tgid
> > > >   BPF_FUNC_get_current_comm
> > >
> > > Hmm, added helpers below are what you list here except get_current_comm.
> > > Was this forgotten to be added here?
> >
> > Forgot to update commit messages. I dropped it because it wasn't clear to
> > me it was very useful or how I would use it from this context. I figure we
> > can add it later if its needed.
> 
> But it's also not harmful in any way and is in a similar group as
> get_current_pid_tgid. So let's add it sooner rather than later. There
> is no cost in allowing this, right?
> 

It shouldn't cost anything only thing is I have code that runs the other
three that has been deployed, at least into a dev environment, so I know
its useful and works.

How about we push it as a follow up? I can add it and do some cleanups
on the CHECK_FAILs tonight.

Thanks,
John

Re: [bpf-next PATCH v5 1/5] bpf, sk_msg: add some generic helpers that may be useful from sk_msg

[Andrii Nakryiko]

On Tue, May 26, 2020 at 1:51 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Andrii Nakryiko wrote:
> > On Mon, May 25, 2020 at 3:57 PM John Fastabend <john.fastabend@gmail.com> wrote:
> > >
> > > Daniel Borkmann wrote:
> > > > On 5/24/20 6:50 PM, John Fastabend wrote:
> > > > > Add these generic helpers that may be useful to use from sk_msg programs.
> > > > > The helpers do not depend on ctx so we can simply add them here,
> > > > >
> > > > >   BPF_FUNC_perf_event_output
> > > > >   BPF_FUNC_get_current_uid_gid
> > > > >   BPF_FUNC_get_current_pid_tgid
> > > > >   BPF_FUNC_get_current_comm
> > > >
> > > > Hmm, added helpers below are what you list here except get_current_comm.
> > > > Was this forgotten to be added here?
> > >
> > > Forgot to update commit messages. I dropped it because it wasn't clear to
> > > me it was very useful or how I would use it from this context. I figure we
> > > can add it later if its needed.
> >
> > But it's also not harmful in any way and is in a similar group as
> > get_current_pid_tgid. So let's add it sooner rather than later. There
> > is no cost in allowing this, right?
> >
>
> It shouldn't cost anything only thing is I have code that runs the other
> three that has been deployed, at least into a dev environment, so I know
> its useful and works.
>
> How about we push it as a follow up? I can add it and do some cleanups
> on the CHECK_FAILs tonight.

Sure, no worries, works for me.

>
> Thanks,
> John

Re: [bpf-next PATCH v5 1/5] bpf, sk_msg: add some generic helpers that may be useful from sk_msg

[Daniel Borkmann]

On 5/26/20 11:29 PM, Andrii Nakryiko wrote:
> On Tue, May 26, 2020 at 1:51 PM John Fastabend <john.fastabend@gmail.com> wrote:
>>
>> Andrii Nakryiko wrote:
>>> On Mon, May 25, 2020 at 3:57 PM John Fastabend <john.fastabend@gmail.com> wrote:
>>>>
>>>> Daniel Borkmann wrote:
>>>>> On 5/24/20 6:50 PM, John Fastabend wrote:
>>>>>> Add these generic helpers that may be useful to use from sk_msg programs.
>>>>>> The helpers do not depend on ctx so we can simply add them here,
>>>>>>
>>>>>>    BPF_FUNC_perf_event_output
>>>>>>    BPF_FUNC_get_current_uid_gid
>>>>>>    BPF_FUNC_get_current_pid_tgid
>>>>>>    BPF_FUNC_get_current_comm
>>>>>
>>>>> Hmm, added helpers below are what you list here except get_current_comm.
>>>>> Was this forgotten to be added here?
>>>>
>>>> Forgot to update commit messages. I dropped it because it wasn't clear to
>>>> me it was very useful or how I would use it from this context. I figure we
>>>> can add it later if its needed.
>>>
>>> But it's also not harmful in any way and is in a similar group as
>>> get_current_pid_tgid. So let's add it sooner rather than later. There
>>> is no cost in allowing this, right?
>>>
>>
>> It shouldn't cost anything only thing is I have code that runs the other
>> three that has been deployed, at least into a dev environment, so I know
>> its useful and works.
>>
>> How about we push it as a follow up? I can add it and do some cleanups
>> on the CHECK_FAILs tonight.
> 
> Sure, no worries, works for me.

Ok, applied then, thanks!