* [bpf-next PATCH v3 1/5] bpf: sk_msg add some generic helpers that may be useful from sk_msg
2020-05-21 14:34 [bpf-next PATCH v3 0/5] bpf: Add sk_msg and networking helpers John Fastabend
@ 2020-05-21 14:34 ` John Fastabend
2020-05-21 14:35 ` [bpf-next PATCH v3 2/5] bpf: extend bpf_base_func_proto helpers with probe_* and *current_task* John Fastabend
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: John Fastabend @ 2020-05-21 14:34 UTC (permalink / raw)
To: yhs, ast, daniel; +Cc: lmb, bpf, john.fastabend, jakub, netdev
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);
}
^ permalink raw reply related [flat|nested] 15+ messages in thread* [bpf-next PATCH v3 2/5] bpf: extend bpf_base_func_proto helpers with probe_* and *current_task*
2020-05-21 14:34 [bpf-next PATCH v3 0/5] bpf: Add sk_msg and networking helpers John Fastabend
2020-05-21 14:34 ` [bpf-next PATCH v3 1/5] bpf: sk_msg add some generic helpers that may be useful from sk_msg John Fastabend
@ 2020-05-21 14:35 ` John Fastabend
2020-05-21 14:35 ` [bpf-next PATCH v3 3/5] bpf: sk_msg add get socket storage helpers John Fastabend
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: John Fastabend @ 2020-05-21 14:35 UTC (permalink / raw)
To: yhs, ast, daniel; +Cc: lmb, bpf, john.fastabend, jakub, netdev
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 | 27 +++++++++++++++++++++++++++
kernel/trace/bpf_trace.c | 16 ++++++++--------
2 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 886949f..ee992dd 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -601,6 +601,13 @@ const struct bpf_func_proto bpf_event_output_data_proto = {
.arg5_type = ARG_CONST_SIZE_OR_ZERO,
};
+const struct bpf_func_proto bpf_current_task_under_cgroup_proto __weak;
+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 +655,26 @@ 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_current_task_under_cgroup:
+ return &bpf_current_task_under_cgroup_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..6fabbc4 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,
@@ -213,7 +213,7 @@ BPF_CALL_3(bpf_probe_read_compat, void *, dst, u32, size,
return bpf_probe_read_kernel_common(dst, size, unsafe_ptr, true);
}
-static const struct bpf_func_proto bpf_probe_read_compat_proto = {
+const struct bpf_func_proto bpf_probe_read_compat_proto = {
.func = bpf_probe_read_compat,
.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,
@@ -268,7 +268,7 @@ BPF_CALL_3(bpf_probe_read_compat_str, void *, dst, u32, size,
return bpf_probe_read_kernel_str_common(dst, size, unsafe_ptr, true);
}
-static const struct bpf_func_proto bpf_probe_read_compat_str_proto = {
+const struct bpf_func_proto bpf_probe_read_compat_str_proto = {
.func = bpf_probe_read_compat_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,
@@ -928,7 +928,7 @@ BPF_CALL_2(bpf_current_task_under_cgroup, struct bpf_map *, map, u32, idx)
return task_under_cgroup_hierarchy(current, cgrp);
}
-static const struct bpf_func_proto bpf_current_task_under_cgroup_proto = {
+const struct bpf_func_proto bpf_current_task_under_cgroup_proto = {
.func = bpf_current_task_under_cgroup,
.gpl_only = false,
.ret_type = RET_INTEGER,
^ permalink raw reply related [flat|nested] 15+ messages in thread* [bpf-next PATCH v3 3/5] bpf: sk_msg add get socket storage helpers
2020-05-21 14:34 [bpf-next PATCH v3 0/5] bpf: Add sk_msg and networking helpers John Fastabend
2020-05-21 14:34 ` [bpf-next PATCH v3 1/5] bpf: sk_msg add some generic helpers that may be useful from sk_msg John Fastabend
2020-05-21 14:35 ` [bpf-next PATCH v3 2/5] bpf: extend bpf_base_func_proto helpers with probe_* and *current_task* John Fastabend
@ 2020-05-21 14:35 ` John Fastabend
2020-05-21 14:35 ` [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test John Fastabend
2020-05-21 14:36 ` [bpf-next PATCH v3 5/5] bpf: selftests, test probe_* helpers from SCHED_CLS John Fastabend
4 siblings, 0 replies; 15+ messages in thread
From: John Fastabend @ 2020-05-21 14:35 UTC (permalink / raw)
To: yhs, ast, daniel; +Cc: lmb, bpf, john.fastabend, jakub, netdev
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 {
^ permalink raw reply related [flat|nested] 15+ messages in thread* [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test
2020-05-21 14:34 [bpf-next PATCH v3 0/5] bpf: Add sk_msg and networking helpers John Fastabend
` (2 preceding siblings ...)
2020-05-21 14:35 ` [bpf-next PATCH v3 3/5] bpf: sk_msg add get socket storage helpers John Fastabend
@ 2020-05-21 14:35 ` John Fastabend
2020-05-21 18:23 ` Yonghong Song
2020-05-21 18:51 ` Andrii Nakryiko
2020-05-21 14:36 ` [bpf-next PATCH v3 5/5] bpf: selftests, test probe_* helpers from SCHED_CLS John Fastabend
4 siblings, 2 replies; 15+ messages in thread
From: John Fastabend @ 2020-05-21 14:35 UTC (permalink / raw)
To: yhs, ast, daniel; +Cc: lmb, bpf, john.fastabend, jakub, netdev
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 | 57 ++++++++++++++++++++
.../selftests/bpf/progs/test_skmsg_load_helpers.c | 48 +++++++++++++++++
2 files changed, 105 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..cacb4ad 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -1,13 +1,46 @@
// 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 */
#define TCP_REPAIR_ON 1
#define TCP_REPAIR_OFF_NO_WP -1 /* Turn off without window probes */
+#define _FAIL(errnum, fmt...) \
+ ({ \
+ error_at_line(0, (errnum), __func__, __LINE__, fmt); \
+ CHECK_FAIL(true); \
+ })
+#define FAIL(fmt...) _FAIL(0, fmt)
+#define FAIL_ERRNO(fmt...) _FAIL(errno, fmt)
+#define FAIL_LIBBPF(err, msg) \
+ ({ \
+ char __buf[MAX_STRERR_LEN]; \
+ libbpf_strerror((err), __buf, sizeof(__buf)); \
+ FAIL("%s: %s", (msg), __buf); \
+ })
+
+#define xbpf_prog_attach(prog, target, type, flags) \
+ ({ \
+ int __ret = \
+ bpf_prog_attach((prog), (target), (type), (flags)); \
+ if (__ret == -1) \
+ FAIL_ERRNO("prog_attach(" #type ")"); \
+ __ret; \
+ })
+
+#define xbpf_prog_detach2(prog, target, type) \
+ ({ \
+ int __ret = bpf_prog_detach2((prog), (target), (type)); \
+ if (__ret == -1) \
+ FAIL_ERRNO("prog_detach2(" #type ")"); \
+ __ret; \
+ })
+
static int connected_socket_v4(void)
{
struct sockaddr_in addr = {
@@ -70,10 +103,34 @@ 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 (!skel) {
+ FAIL("skeleton open/load failed");
+ return;
+ }
+
+ verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
+ map = bpf_map__fd(skel->maps.sock_map);
+
+ err = xbpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
+ if (err)
+ return;
+ xbpf_prog_detach2(verdict, map, BPF_SK_MSG_VERDICT);
+}
+
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..b68eb6c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
@@ -0,0 +1,48 @@
+// 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;
+}
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test
2020-05-21 14:35 ` [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test John Fastabend
@ 2020-05-21 18:23 ` Yonghong Song
2020-05-21 18:51 ` John Fastabend
2020-05-21 18:51 ` Andrii Nakryiko
1 sibling, 1 reply; 15+ messages in thread
From: Yonghong Song @ 2020-05-21 18:23 UTC (permalink / raw)
To: John Fastabend, ast, daniel; +Cc: lmb, bpf, jakub, netdev
On 5/21/20 7:35 AM, John Fastabend 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>
> ---
> .../selftests/bpf/prog_tests/sockmap_basic.c | 57 ++++++++++++++++++++
> .../selftests/bpf/progs/test_skmsg_load_helpers.c | 48 +++++++++++++++++
> 2 files changed, 105 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..cacb4ad 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -1,13 +1,46 @@
> // 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 */
>
> #define TCP_REPAIR_ON 1
> #define TCP_REPAIR_OFF_NO_WP -1 /* Turn off without window probes */
>
> +#define _FAIL(errnum, fmt...) \
> + ({ \
> + error_at_line(0, (errnum), __func__, __LINE__, fmt); \
> + CHECK_FAIL(true); \
> + })
> +#define FAIL(fmt...) _FAIL(0, fmt)
> +#define FAIL_ERRNO(fmt...) _FAIL(errno, fmt)
> +#define FAIL_LIBBPF(err, msg) \
> + ({ \
> + char __buf[MAX_STRERR_LEN]; \
> + libbpf_strerror((err), __buf, sizeof(__buf)); \
> + FAIL("%s: %s", (msg), __buf); \
> + })
Can we use existing macros in test_progs.h?
This will be consistent with other test_progs selftests.
> +
> +#define xbpf_prog_attach(prog, target, type, flags) \
> + ({ \
> + int __ret = \
> + bpf_prog_attach((prog), (target), (type), (flags)); \
> + if (__ret == -1) \
> + FAIL_ERRNO("prog_attach(" #type ")"); \
> + __ret; \
> + })
> +
> +#define xbpf_prog_detach2(prog, target, type) \
> + ({ \
> + int __ret = bpf_prog_detach2((prog), (target), (type)); \
> + if (__ret == -1) \
> + FAIL_ERRNO("prog_detach2(" #type ")"); \
> + __ret; \
> + })
The above xbpf_prog_attach() and xbpf_prog_detach2()
are only called once, maybe fold into the calling function itself?
> +
> static int connected_socket_v4(void)
> {
> struct sockaddr_in addr = {
> @@ -70,10 +103,34 @@ 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 (!skel) {
> + FAIL("skeleton open/load failed");
> + return;
> + }
> +
> + verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
> + map = bpf_map__fd(skel->maps.sock_map);
> +
> + err = xbpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
> + if (err)
> + return;
> + xbpf_prog_detach2(verdict, map, BPF_SK_MSG_VERDICT);
> +}
> +
> 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..b68eb6c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
> @@ -0,0 +1,48 @@
> +// 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;
> +}
> +
> +int _version SEC("version") = 1;
> +char _license[] SEC("license") = "GPL";
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test
2020-05-21 18:23 ` Yonghong Song
@ 2020-05-21 18:51 ` John Fastabend
0 siblings, 0 replies; 15+ messages in thread
From: John Fastabend @ 2020-05-21 18:51 UTC (permalink / raw)
To: Yonghong Song, John Fastabend, ast, daniel; +Cc: lmb, bpf, jakub, netdev
Yonghong Song wrote:
>
>
> On 5/21/20 7:35 AM, John Fastabend 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>
> > ---
> > .../selftests/bpf/prog_tests/sockmap_basic.c | 57 ++++++++++++++++++++
> > .../selftests/bpf/progs/test_skmsg_load_helpers.c | 48 +++++++++++++++++
> > 2 files changed, 105 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..cacb4ad 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > @@ -1,13 +1,46 @@
> > // 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 */
> >
> > #define TCP_REPAIR_ON 1
> > #define TCP_REPAIR_OFF_NO_WP -1 /* Turn off without window probes */
> >
> > +#define _FAIL(errnum, fmt...) \
> > + ({ \
> > + error_at_line(0, (errnum), __func__, __LINE__, fmt); \
> > + CHECK_FAIL(true); \
> > + })
> > +#define FAIL(fmt...) _FAIL(0, fmt)
> > +#define FAIL_ERRNO(fmt...) _FAIL(errno, fmt)
> > +#define FAIL_LIBBPF(err, msg) \
> > + ({ \
> > + char __buf[MAX_STRERR_LEN]; \
> > + libbpf_strerror((err), __buf, sizeof(__buf)); \
> > + FAIL("%s: %s", (msg), __buf); \
> > + })
>
> Can we use existing macros in test_progs.h?
> This will be consistent with other test_progs selftests.
That will work. I was planning to come back and cleanup tests that are
not using the test_progs.h variants but good point no point in adding
one more.
>
> > +
> > +#define xbpf_prog_attach(prog, target, type, flags) \
> > + ({ \
> > + int __ret = \
> > + bpf_prog_attach((prog), (target), (type), (flags)); \
> > + if (__ret == -1) \
> > + FAIL_ERRNO("prog_attach(" #type ")"); \
> > + __ret; \
> > + })
> > +
> > +#define xbpf_prog_detach2(prog, target, type) \
> > + ({ \
> > + int __ret = bpf_prog_detach2((prog), (target), (type)); \
> > + if (__ret == -1) \
> > + FAIL_ERRNO("prog_detach2(" #type ")"); \
> > + __ret; \
> > + })
>
> The above xbpf_prog_attach() and xbpf_prog_detach2()
> are only called once, maybe fold into the calling function itself?
>
Sure.
Thanks,
John
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test
2020-05-21 14:35 ` [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test John Fastabend
2020-05-21 18:23 ` Yonghong Song
@ 2020-05-21 18:51 ` Andrii Nakryiko
2020-05-21 19:03 ` John Fastabend
1 sibling, 1 reply; 15+ messages in thread
From: Andrii Nakryiko @ 2020-05-21 18:51 UTC (permalink / raw)
To: John Fastabend
Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Lorenz Bauer,
bpf, Jakub Sitnicki, Networking
On Thu, May 21, 2020 at 7:36 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>
> ---
> .../selftests/bpf/prog_tests/sockmap_basic.c | 57 ++++++++++++++++++++
> .../selftests/bpf/progs/test_skmsg_load_helpers.c | 48 +++++++++++++++++
> 2 files changed, 105 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..cacb4ad 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -1,13 +1,46 @@
> // 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 */
>
> #define TCP_REPAIR_ON 1
> #define TCP_REPAIR_OFF_NO_WP -1 /* Turn off without window probes */
>
> +#define _FAIL(errnum, fmt...) \
> + ({ \
> + error_at_line(0, (errnum), __func__, __LINE__, fmt); \
> + CHECK_FAIL(true); \
> + })
> +#define FAIL(fmt...) _FAIL(0, fmt)
> +#define FAIL_ERRNO(fmt...) _FAIL(errno, fmt)
> +#define FAIL_LIBBPF(err, msg) \
> + ({ \
> + char __buf[MAX_STRERR_LEN]; \
> + libbpf_strerror((err), __buf, sizeof(__buf)); \
> + FAIL("%s: %s", (msg), __buf); \
> + })
> +
> +#define xbpf_prog_attach(prog, target, type, flags) \
> + ({ \
> + int __ret = \
> + bpf_prog_attach((prog), (target), (type), (flags)); \
> + if (__ret == -1) \
> + FAIL_ERRNO("prog_attach(" #type ")"); \
> + __ret; \
> + })
> +
> +#define xbpf_prog_detach2(prog, target, type) \
> + ({ \
> + int __ret = bpf_prog_detach2((prog), (target), (type)); \
> + if (__ret == -1) \
> + FAIL_ERRNO("prog_detach2(" #type ")"); \
> + __ret; \
> + })
I'm not convinced we need these macro, can you please just use CHECKs?
I'd rather not learn each specific test's custom macros.
> +
> static int connected_socket_v4(void)
> {
> struct sockaddr_in addr = {
> @@ -70,10 +103,34 @@ 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 (!skel) {
> + FAIL("skeleton open/load failed");
> + return;
> + }
> +
> + verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
> + map = bpf_map__fd(skel->maps.sock_map);
> +
> + err = xbpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
> + if (err)
> + return;
> + xbpf_prog_detach2(verdict, map, BPF_SK_MSG_VERDICT);
no cleanup in this test, at all
> +}
> +
[...]
> +
> +int _version SEC("version") = 1;
version not needed
> +char _license[] SEC("license") = "GPL";
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test
2020-05-21 18:51 ` Andrii Nakryiko
@ 2020-05-21 19:03 ` John Fastabend
2020-05-21 19:09 ` John Fastabend
0 siblings, 1 reply; 15+ messages in thread
From: John Fastabend @ 2020-05-21 19:03 UTC (permalink / raw)
To: Andrii Nakryiko, John Fastabend
Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Lorenz Bauer,
bpf, Jakub Sitnicki, Networking
Andrii Nakryiko wrote:
> On Thu, May 21, 2020 at 7:36 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>
> > ---
> > .../selftests/bpf/prog_tests/sockmap_basic.c | 57 ++++++++++++++++++++
> > .../selftests/bpf/progs/test_skmsg_load_helpers.c | 48 +++++++++++++++++
> > 2 files changed, 105 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..cacb4ad 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > @@ -1,13 +1,46 @@
> > // 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 */
> >
> > #define TCP_REPAIR_ON 1
> > #define TCP_REPAIR_OFF_NO_WP -1 /* Turn off without window probes */
> >
> > +#define _FAIL(errnum, fmt...) \
> > + ({ \
> > + error_at_line(0, (errnum), __func__, __LINE__, fmt); \
> > + CHECK_FAIL(true); \
> > + })
> > +#define FAIL(fmt...) _FAIL(0, fmt)
> > +#define FAIL_ERRNO(fmt...) _FAIL(errno, fmt)
> > +#define FAIL_LIBBPF(err, msg) \
> > + ({ \
> > + char __buf[MAX_STRERR_LEN]; \
> > + libbpf_strerror((err), __buf, sizeof(__buf)); \
> > + FAIL("%s: %s", (msg), __buf); \
> > + })
> > +
> > +#define xbpf_prog_attach(prog, target, type, flags) \
> > + ({ \
> > + int __ret = \
> > + bpf_prog_attach((prog), (target), (type), (flags)); \
> > + if (__ret == -1) \
> > + FAIL_ERRNO("prog_attach(" #type ")"); \
> > + __ret; \
> > + })
> > +
> > +#define xbpf_prog_detach2(prog, target, type) \
> > + ({ \
> > + int __ret = bpf_prog_detach2((prog), (target), (type)); \
> > + if (__ret == -1) \
> > + FAIL_ERRNO("prog_detach2(" #type ")"); \
> > + __ret; \
> > + })
>
> I'm not convinced we need these macro, can you please just use CHECKs?
> I'd rather not learn each specific test's custom macros.
Will just remove the entire block above.
>
> > +
> > static int connected_socket_v4(void)
> > {
> > struct sockaddr_in addr = {
> > @@ -70,10 +103,34 @@ 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 (!skel) {
> > + FAIL("skeleton open/load failed");
> > + return;
> > + }
> > +
> > + verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
> > + map = bpf_map__fd(skel->maps.sock_map);
> > +
> > + err = xbpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
> > + if (err)
> > + return;
> > + xbpf_prog_detach2(verdict, map, BPF_SK_MSG_VERDICT);
>
> no cleanup in this test, at all
Guess we need __destroy(skel) here.
As an aside how come if the program closes and refcnt drops the entire
thing isn't destroyed. I didn't think there was any pinning happening
in the __open_and_load piece.
>
> > +}
> > +
>
> [...]
>
> > +
> > +int _version SEC("version") = 1;
>
> version not needed
>
> > +char _license[] SEC("license") = "GPL";
> >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test
2020-05-21 19:03 ` John Fastabend
@ 2020-05-21 19:09 ` John Fastabend
2020-05-21 19:12 ` Andrii Nakryiko
0 siblings, 1 reply; 15+ messages in thread
From: John Fastabend @ 2020-05-21 19:09 UTC (permalink / raw)
To: John Fastabend, Andrii Nakryiko, John Fastabend
Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Lorenz Bauer,
bpf, Jakub Sitnicki, Networking
John Fastabend wrote:
> Andrii Nakryiko wrote:
> > On Thu, May 21, 2020 at 7:36 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>
> > > ---
[...]
> > > +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 (!skel) {
> > > + FAIL("skeleton open/load failed");
> > > + return;
> > > + }
> > > +
> > > + verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
> > > + map = bpf_map__fd(skel->maps.sock_map);
> > > +
> > > + err = xbpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
> > > + if (err)
> > > + return;
> > > + xbpf_prog_detach2(verdict, map, BPF_SK_MSG_VERDICT);
> >
> > no cleanup in this test, at all
>
> Guess we need __destroy(skel) here.
>
> As an aside how come if the program closes and refcnt drops the entire
> thing isn't destroyed. I didn't think there was any pinning happening
> in the __open_and_load piece.
I guess these are in progs_test so we can't leave these around for
any following tests to trip over. OK. Same thing for patch 3 fwiw.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test
2020-05-21 19:09 ` John Fastabend
@ 2020-05-21 19:12 ` Andrii Nakryiko
0 siblings, 0 replies; 15+ messages in thread
From: Andrii Nakryiko @ 2020-05-21 19:12 UTC (permalink / raw)
To: John Fastabend
Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Lorenz Bauer,
bpf, Jakub Sitnicki, Networking
On Thu, May 21, 2020 at 12:09 PM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> John Fastabend wrote:
> > Andrii Nakryiko wrote:
> > > On Thu, May 21, 2020 at 7:36 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>
> > > > ---
>
> [...]
>
> > > > +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 (!skel) {
> > > > + FAIL("skeleton open/load failed");
> > > > + return;
> > > > + }
> > > > +
> > > > + verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
> > > > + map = bpf_map__fd(skel->maps.sock_map);
> > > > +
> > > > + err = xbpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
> > > > + if (err)
> > > > + return;
> > > > + xbpf_prog_detach2(verdict, map, BPF_SK_MSG_VERDICT);
> > >
> > > no cleanup in this test, at all
> >
> > Guess we need __destroy(skel) here.
> >
> > As an aside how come if the program closes and refcnt drops the entire
> > thing isn't destroyed. I didn't think there was any pinning happening
> > in the __open_and_load piece.
>
> I guess these are in progs_test so we can't leave these around for
> any following tests to trip over. OK. Same thing for patch 3 fwiw.
Yep, exactly. It's a cooperative environment at the moment. We've
talked about running tests in forked processes and in parallel, but
until then, cleaning up is very important.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [bpf-next PATCH v3 5/5] bpf: selftests, test probe_* helpers from SCHED_CLS
2020-05-21 14:34 [bpf-next PATCH v3 0/5] bpf: Add sk_msg and networking helpers John Fastabend
` (3 preceding siblings ...)
2020-05-21 14:35 ` [bpf-next PATCH v3 4/5] bpf: selftests, add sk_msg helpers load and attach test John Fastabend
@ 2020-05-21 14:36 ` John Fastabend
2020-05-21 18:32 ` Yonghong Song
2020-05-21 18:47 ` Andrii Nakryiko
4 siblings, 2 replies; 15+ messages in thread
From: John Fastabend @ 2020-05-21 14:36 UTC (permalink / raw)
To: yhs, ast, daniel; +Cc: lmb, bpf, john.fastabend, jakub, netdev
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.
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 | 33 ++++++++++++++++++++
2 files changed, 63 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..5a865c4
--- /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 != 0, "len", "err %d errno %d\n", err, errno);
+}
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..05a1260
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+int _version SEC("version") = 1;
+
+#define TEST_COMM_LEN 10
+
+struct bpf_map_def SEC("maps") cgroup_map = {
+ .type = BPF_MAP_TYPE_CGROUP_ARRAY,
+ .key_size = sizeof(u32),
+ .value_size = sizeof(u32),
+ .max_entries = 1,
+};
+
+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;
+ int ctask;
+
+ ctask = bpf_current_task_under_cgroup(&cgroup_map, 0);
+ 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;
+}
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [bpf-next PATCH v3 5/5] bpf: selftests, test probe_* helpers from SCHED_CLS
2020-05-21 14:36 ` [bpf-next PATCH v3 5/5] bpf: selftests, test probe_* helpers from SCHED_CLS John Fastabend
@ 2020-05-21 18:32 ` Yonghong Song
2020-05-21 18:47 ` Andrii Nakryiko
1 sibling, 0 replies; 15+ messages in thread
From: Yonghong Song @ 2020-05-21 18:32 UTC (permalink / raw)
To: John Fastabend, ast, daniel; +Cc: lmb, bpf, jakub, netdev
On 5/21/20 7:36 AM, John Fastabend 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.
>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Ack with a minor nit below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> .../testing/selftests/bpf/prog_tests/skb_helpers.c | 30 ++++++++++++++++++
> .../testing/selftests/bpf/progs/test_skb_helpers.c | 33 ++++++++++++++++++++
> 2 files changed, 63 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..5a865c4
> --- /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 != 0, "len", "err %d errno %d\n", err, errno);
> +}
> 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..05a1260
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +
> +int _version SEC("version") = 1;
> +
> +#define TEST_COMM_LEN 10
> +
> +struct bpf_map_def SEC("maps") cgroup_map = {
> + .type = BPF_MAP_TYPE_CGROUP_ARRAY,
> + .key_size = sizeof(u32),
> + .value_size = sizeof(u32),
> + .max_entries = 1,
> +};
> +
> +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;
> + int ctask;
> +
> + ctask = bpf_current_task_under_cgroup(&cgroup_map, 0);
ctask is not used. Could you test ctask against expected value?
> + 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;
> +}
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [bpf-next PATCH v3 5/5] bpf: selftests, test probe_* helpers from SCHED_CLS
2020-05-21 14:36 ` [bpf-next PATCH v3 5/5] bpf: selftests, test probe_* helpers from SCHED_CLS John Fastabend
2020-05-21 18:32 ` Yonghong Song
@ 2020-05-21 18:47 ` Andrii Nakryiko
2020-05-21 19:11 ` John Fastabend
1 sibling, 1 reply; 15+ messages in thread
From: Andrii Nakryiko @ 2020-05-21 18:47 UTC (permalink / raw)
To: John Fastabend
Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Lorenz Bauer,
bpf, Jakub Sitnicki, Networking
On Thu, May 21, 2020 at 7:36 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.
>
> 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 | 33 ++++++++++++++++++++
> 2 files changed, 63 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..5a865c4
> --- /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);
hm... who's destroying bpf_object?
> + if (CHECK_ATTR(err, "load", "err %d errno %d\n", err, errno))
> + return;
> +
> + err = bpf_prog_test_run_xattr(&tattr);
> + CHECK_ATTR(err != 0, "len", "err %d errno %d\n", err, errno);
> +}
> 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..05a1260
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +
> +int _version SEC("version") = 1;
version is not needed
> +
> +#define TEST_COMM_LEN 10
doesn't matter for this test, but it's 16 everywhere, let's stay consistent
> +
> +struct bpf_map_def SEC("maps") cgroup_map = {
> + .type = BPF_MAP_TYPE_CGROUP_ARRAY,
> + .key_size = sizeof(u32),
> + .value_size = sizeof(u32),
> + .max_entries = 1,
> +};
> +
Please use new BTF syntax for 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];
this is array of pointer, not array of chars
> + __u32 tpid;
> + int ctask;
> +
> + ctask = bpf_current_task_under_cgroup(&cgroup_map, 0);
compiler might complain that ctask is written, but not read. Let's
assign it to some global variable?
> + 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;
> +}
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [bpf-next PATCH v3 5/5] bpf: selftests, test probe_* helpers from SCHED_CLS
2020-05-21 18:47 ` Andrii Nakryiko
@ 2020-05-21 19:11 ` John Fastabend
0 siblings, 0 replies; 15+ messages in thread
From: John Fastabend @ 2020-05-21 19:11 UTC (permalink / raw)
To: Andrii Nakryiko, John Fastabend
Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Lorenz Bauer,
bpf, Jakub Sitnicki, Networking
Andrii Nakryiko wrote:
> On Thu, May 21, 2020 at 7:36 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.
> >
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
[...]
> > +++ b/tools/testing/selftests/bpf/progs/test_skb_helpers.c
> > @@ -0,0 +1,33 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_endian.h>
> > +
> > +int _version SEC("version") = 1;
>
> version is not needed
>
> > +
> > +#define TEST_COMM_LEN 10
>
> doesn't matter for this test, but it's 16 everywhere, let's stay consistent
>
> > +
> > +struct bpf_map_def SEC("maps") cgroup_map = {
> > + .type = BPF_MAP_TYPE_CGROUP_ARRAY,
> > + .key_size = sizeof(u32),
> > + .value_size = sizeof(u32),
> > + .max_entries = 1,
> > +};
> > +
>
> Please use new BTF syntax for 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];
>
> this is array of pointer, not array of chars
>
> > + __u32 tpid;
> > + int ctask;
> > +
> > + ctask = bpf_current_task_under_cgroup(&cgroup_map, 0);
>
> compiler might complain that ctask is written, but not read. Let's
> assign it to some global variable?
I'll do a read here and check the value then it should be fine.
Will fold in the above comments as well.
> > + 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;
> > +}
> >
^ permalink raw reply [flat|nested] 15+ messages in thread