netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic)
@ 2022-07-20 11:46 Artem Savkov
  2022-07-20 11:46 ` [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD Artem Savkov
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Artem Savkov @ 2022-07-20 11:46 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf, netdev
  Cc: linux-kernel, Andrea Arcangeli, Daniel Vacek, Jiri Olsa,
	Song Liu, Artem Savkov

eBPF is often used for kernel debugging, and one of the widely used and
powerful debugging techniques is post-mortem debugging with a full memory dump.
Triggering a panic at exactly the right moment allows the user to get such a
dump and thus a better view at the system's state. Right now the only way to
do this in BPF is to signal userspace to trigger kexec/panic. This is
suboptimal as going through userspace requires context changes and adds
significant delays taking system further away from "the right moment". On a
single-cpu system the situation is even worse because BPF program won't even be
able to block the thread of interest.

This patchset tries to solve this problem by allowing properly marked tracing
bpf programs to call crash_kexec() kernel function.

This is a continuation of bpf_panic patchset with initial feedback taken into
account.

Changes from RFC:
 - sysctl knob dropped
 - using crash_kexec() instead of panic()
 - using kfuncs instead of adding a new helper

Artem Savkov (4):
  bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
  bpf: add destructive kfunc set
  selftests/bpf: add destructive kfunc tests
  bpf: export crash_kexec() as destructive kfunc

 include/linux/bpf.h                           |  1 +
 include/linux/btf.h                           |  2 +
 include/uapi/linux/bpf.h                      |  6 +++
 kernel/bpf/syscall.c                          |  4 +-
 kernel/bpf/verifier.c                         | 12 ++++++
 kernel/kexec_core.c                           | 22 ++++++++++
 net/bpf/test_run.c                            | 12 +++++-
 tools/include/uapi/linux/bpf.h                |  6 +++
 .../selftests/bpf/prog_tests/kfunc_call.c     | 41 +++++++++++++++++++
 .../bpf/progs/kfunc_call_destructive.c        | 14 +++++++
 10 files changed, 118 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/kfunc_call_destructive.c

-- 
2.35.3


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
  2022-07-20 11:46 [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Artem Savkov
@ 2022-07-20 11:46 ` Artem Savkov
  2022-07-21 14:02   ` Alexei Starovoitov
  2022-07-20 11:46 ` [PATCH bpf-next 2/4] bpf: add destructive kfunc set Artem Savkov
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Artem Savkov @ 2022-07-20 11:46 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf, netdev
  Cc: linux-kernel, Andrea Arcangeli, Daniel Vacek, Jiri Olsa,
	Song Liu, Artem Savkov

Add a BPF_F_DESTRUCTIVE flag which will be required to be supplied
during BPF_PROG_LOAD for programs to be able to call destructive kfuncs.

Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
 include/linux/bpf.h            | 1 +
 include/uapi/linux/bpf.h       | 6 ++++++
 kernel/bpf/syscall.c           | 4 +++-
 tools/include/uapi/linux/bpf.h | 6 ++++++
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a5bf00649995e..7b404d0b80aef 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1044,6 +1044,7 @@ struct bpf_prog_aux {
 	bool sleepable;
 	bool tail_call_reachable;
 	bool xdp_has_frags;
+	bool destructive;
 	/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
 	const struct btf_type *attach_func_proto;
 	/* function name for valid attach_btf_id */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 379e68fb866fc..ae81ad2e658dd 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1122,6 +1122,12 @@ enum bpf_link_type {
  */
 #define BPF_F_XDP_HAS_FRAGS	(1U << 5)
 
+/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program
+ * will be able to perform destructive operations such as calling bpf_panic()
+ * helper.
+ */
+#define BPF_F_DESTRUCTIVE	(1U << 6)
+
 /* link_create.kprobe_multi.flags used in LINK_CREATE command for
  * BPF_TRACE_KPROBE_MULTI attach type to create return probe.
  */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 83c7136c5788d..86927521d0ea2 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2467,7 +2467,8 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 				 BPF_F_TEST_STATE_FREQ |
 				 BPF_F_SLEEPABLE |
 				 BPF_F_TEST_RND_HI32 |
-				 BPF_F_XDP_HAS_FRAGS))
+				 BPF_F_XDP_HAS_FRAGS |
+				 BPF_F_DESTRUCTIVE))
 		return -EINVAL;
 
 	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) &&
@@ -2554,6 +2555,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 	prog->aux->offload_requested = !!attr->prog_ifindex;
 	prog->aux->sleepable = attr->prog_flags & BPF_F_SLEEPABLE;
 	prog->aux->xdp_has_frags = attr->prog_flags & BPF_F_XDP_HAS_FRAGS;
+	prog->aux->destructive = attr->prog_flags & BPF_F_DESTRUCTIVE;
 
 	err = security_bpf_prog_alloc(prog->aux);
 	if (err)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 379e68fb866fc..ae81ad2e658dd 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1122,6 +1122,12 @@ enum bpf_link_type {
  */
 #define BPF_F_XDP_HAS_FRAGS	(1U << 5)
 
+/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program
+ * will be able to perform destructive operations such as calling bpf_panic()
+ * helper.
+ */
+#define BPF_F_DESTRUCTIVE	(1U << 6)
+
 /* link_create.kprobe_multi.flags used in LINK_CREATE command for
  * BPF_TRACE_KPROBE_MULTI attach type to create return probe.
  */
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH bpf-next 2/4] bpf: add destructive kfunc set
  2022-07-20 11:46 [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Artem Savkov
  2022-07-20 11:46 ` [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD Artem Savkov
@ 2022-07-20 11:46 ` Artem Savkov
  2022-07-20 11:46 ` [PATCH bpf-next 3/4] selftests/bpf: add destructive kfunc tests Artem Savkov
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Artem Savkov @ 2022-07-20 11:46 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf, netdev
  Cc: linux-kernel, Andrea Arcangeli, Daniel Vacek, Jiri Olsa,
	Song Liu, Artem Savkov

Add BTF_KFUNC_TYPE_DESTRUCTIVE and a new destructive_set in struct
btf_kfunc_id_set. Functions in this set will require CAP_SYS_BOOT
capabilities and BPF_F_DESTRUCTIVE flag.

Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
 include/linux/btf.h   |  2 ++
 kernel/bpf/verifier.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 1bfed7fa04287..6c58aa70e8125 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -18,6 +18,7 @@ enum btf_kfunc_type {
 	BTF_KFUNC_TYPE_RELEASE,
 	BTF_KFUNC_TYPE_RET_NULL,
 	BTF_KFUNC_TYPE_KPTR_ACQUIRE,
+	BTF_KFUNC_TYPE_DESTRUCTIVE,
 	BTF_KFUNC_TYPE_MAX,
 };
 
@@ -37,6 +38,7 @@ struct btf_kfunc_id_set {
 			struct btf_id_set *release_set;
 			struct btf_id_set *ret_null_set;
 			struct btf_id_set *kptr_acquire_set;
+			struct btf_id_set *destructive_set;
 		};
 		struct btf_id_set *sets[BTF_KFUNC_TYPE_MAX];
 	};
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c59c3df0fea61..064035e70deac 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7582,6 +7582,18 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		return -EACCES;
 	}
 
+	if (btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog),
+				      BTF_KFUNC_TYPE_DESTRUCTIVE, func_id)) {
+		if (!env->prog->aux->destructive) {
+			verbose(env, "destructive kfunc calls require BPF_F_DESTRUCTIVE flag\n");
+			return -EACCES;
+		}
+		if (!capable(CAP_SYS_BOOT)) {
+			verbose(env, "destructive kfunc calls require CAP_SYS_BOOT capabilities\n");
+			return -EACCES;
+		}
+	}
+
 	acq = btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog),
 					BTF_KFUNC_TYPE_ACQUIRE, func_id);
 
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH bpf-next 3/4] selftests/bpf: add destructive kfunc tests
  2022-07-20 11:46 [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Artem Savkov
  2022-07-20 11:46 ` [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD Artem Savkov
  2022-07-20 11:46 ` [PATCH bpf-next 2/4] bpf: add destructive kfunc set Artem Savkov
@ 2022-07-20 11:46 ` Artem Savkov
  2022-07-20 11:46 ` [PATCH bpf-next 4/4] bpf: export crash_kexec() as destructive kfunc Artem Savkov
  2022-07-21 13:00 ` [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Daniel Borkmann
  4 siblings, 0 replies; 11+ messages in thread
From: Artem Savkov @ 2022-07-20 11:46 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf, netdev
  Cc: linux-kernel, Andrea Arcangeli, Daniel Vacek, Jiri Olsa,
	Song Liu, Artem Savkov

Add tests checking that programs calling destructive kfuncs can only do
so if they have BPF_F_DESTRUCTIVE flag set and CAP_SYS_BOOT
capabilities.

Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
 net/bpf/test_run.c                            | 12 +++++-
 .../selftests/bpf/prog_tests/kfunc_call.c     | 41 +++++++++++++++++++
 .../bpf/progs/kfunc_call_destructive.c        | 14 +++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/kfunc_call_destructive.c

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 2ca96acbc50a0..dc267fa308f92 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -691,6 +691,10 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
 {
 }
 
+noinline void bpf_kfunc_call_test_destructive(void)
+{
+}
+
 __diag_pop();
 
 ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO);
@@ -714,6 +718,7 @@ BTF_ID(func, bpf_kfunc_call_test_fail3)
 BTF_ID(func, bpf_kfunc_call_test_mem_len_pass1)
 BTF_ID(func, bpf_kfunc_call_test_mem_len_fail1)
 BTF_ID(func, bpf_kfunc_call_test_mem_len_fail2)
+BTF_ID(func, bpf_kfunc_call_test_destructive)
 BTF_SET_END(test_sk_check_kfunc_ids)
 
 BTF_SET_START(test_sk_acquire_kfunc_ids)
@@ -738,6 +743,10 @@ BTF_SET_START(test_sk_kptr_acquire_kfunc_ids)
 BTF_ID(func, bpf_kfunc_call_test_kptr_get)
 BTF_SET_END(test_sk_kptr_acquire_kfunc_ids)
 
+BTF_SET_START(test_sk_destructive_kfunc_ids)
+BTF_ID(func, bpf_kfunc_call_test_destructive)
+BTF_SET_END(test_sk_destructive_kfunc_ids)
+
 static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
 			   u32 size, u32 headroom, u32 tailroom)
 {
@@ -1622,7 +1631,8 @@ static const struct btf_kfunc_id_set bpf_prog_test_kfunc_set = {
 	.acquire_set      = &test_sk_acquire_kfunc_ids,
 	.release_set      = &test_sk_release_kfunc_ids,
 	.ret_null_set     = &test_sk_ret_null_kfunc_ids,
-	.kptr_acquire_set = &test_sk_kptr_acquire_kfunc_ids
+	.kptr_acquire_set = &test_sk_kptr_acquire_kfunc_ids,
+	.destructive_set  = &test_sk_destructive_kfunc_ids,
 };
 
 BTF_ID_LIST(bpf_prog_test_dtor_kfunc_ids)
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index c00eb974eb85e..1c2c4fdfba88b 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -5,6 +5,9 @@
 #include "kfunc_call_test.lskel.h"
 #include "kfunc_call_test_subprog.skel.h"
 #include "kfunc_call_test_subprog.lskel.h"
+#include "kfunc_call_destructive.skel.h"
+
+#include "cap_helpers.h"
 
 static void test_main(void)
 {
@@ -86,6 +89,41 @@ static void test_subprog_lskel(void)
 	kfunc_call_test_subprog_lskel__destroy(skel);
 }
 
+static int test_destructive_open_and_load(int set_flag)
+{
+	struct kfunc_call_destructive *skel;
+	int err;
+
+	skel = kfunc_call_destructive__open();
+	if (!ASSERT_OK_PTR(skel, "prog_open"))
+		return -1;
+
+	if (set_flag)
+		bpf_program__set_flags(skel->progs.kfunc_destructive_test,
+				bpf_program__flags(skel->progs.kfunc_destructive_test) | BPF_F_DESTRUCTIVE);
+
+	err = kfunc_call_destructive__load(skel);
+
+	kfunc_call_destructive__destroy(skel);
+
+	return err;
+}
+
+static void test_destructive(void)
+{
+	__u64 save_caps = 0;
+
+	ASSERT_EQ(test_destructive_open_and_load(0), -13, "no_flag_failure");
+	ASSERT_OK(test_destructive_open_and_load(1), "succesful_load");
+
+	if (!ASSERT_OK(cap_disable_effective(1ULL << CAP_SYS_BOOT, &save_caps), "drop_caps"))
+		return;
+
+	ASSERT_EQ(test_destructive_open_and_load(1), -13, "no_caps_failure");
+
+	cap_enable_effective(save_caps, NULL);
+}
+
 void test_kfunc_call(void)
 {
 	if (test__start_subtest("main"))
@@ -96,4 +134,7 @@ void test_kfunc_call(void)
 
 	if (test__start_subtest("subprog_lskel"))
 		test_subprog_lskel();
+
+	if (test__start_subtest("destructive"))
+		test_destructive();
 }
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
new file mode 100644
index 0000000000000..767472bc5a97b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+
+extern void bpf_kfunc_call_test_destructive(void) __ksym;
+
+SEC("tc")
+int kfunc_destructive_test(void)
+{
+	bpf_kfunc_call_test_destructive();
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH bpf-next 4/4] bpf: export crash_kexec() as destructive kfunc
  2022-07-20 11:46 [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Artem Savkov
                   ` (2 preceding siblings ...)
  2022-07-20 11:46 ` [PATCH bpf-next 3/4] selftests/bpf: add destructive kfunc tests Artem Savkov
@ 2022-07-20 11:46 ` Artem Savkov
  2022-07-21 13:00 ` [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Daniel Borkmann
  4 siblings, 0 replies; 11+ messages in thread
From: Artem Savkov @ 2022-07-20 11:46 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf, netdev
  Cc: linux-kernel, Andrea Arcangeli, Daniel Vacek, Jiri Olsa,
	Song Liu, Artem Savkov

Allow properly marked bpf programs to call crash_kexec().

Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
 kernel/kexec_core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 4d34c78334ce4..a21fe8d326a8e 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -39,6 +39,8 @@
 #include <linux/hugetlb.h>
 #include <linux/objtool.h>
 #include <linux/kmsg_dump.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
 
 #include <asm/page.h>
 #include <asm/sections.h>
@@ -1238,3 +1240,23 @@ void __weak arch_kexec_protect_crashkres(void)
 
 void __weak arch_kexec_unprotect_crashkres(void)
 {}
+
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+BTF_SET_START(kexec_btf_ids)
+BTF_ID(func, crash_kexec)
+BTF_SET_END(kexec_btf_ids)
+
+static const struct btf_kfunc_id_set kexec_kfunc_set = {
+	.owner			= THIS_MODULE,
+	.check_set		= &kexec_btf_ids,
+	.destructive_set	= &kexec_btf_ids,
+};
+
+static int __init crash_kfunc_init(void)
+{
+	register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &kexec_kfunc_set);
+	return 0;
+}
+
+subsys_initcall(crash_kfunc_init);
+#endif
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic)
  2022-07-20 11:46 [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Artem Savkov
                   ` (3 preceding siblings ...)
  2022-07-20 11:46 ` [PATCH bpf-next 4/4] bpf: export crash_kexec() as destructive kfunc Artem Savkov
@ 2022-07-21 13:00 ` Daniel Borkmann
  4 siblings, 0 replies; 11+ messages in thread
From: Daniel Borkmann @ 2022-07-21 13:00 UTC (permalink / raw)
  To: Artem Savkov, Alexei Starovoitov, Andrii Nakryiko, bpf, netdev
  Cc: linux-kernel, Andrea Arcangeli, Daniel Vacek, Jiri Olsa, Song Liu

On 7/20/22 1:46 PM, Artem Savkov wrote:
> eBPF is often used for kernel debugging, and one of the widely used and
> powerful debugging techniques is post-mortem debugging with a full memory dump.
> Triggering a panic at exactly the right moment allows the user to get such a
> dump and thus a better view at the system's state. Right now the only way to
> do this in BPF is to signal userspace to trigger kexec/panic. This is
> suboptimal as going through userspace requires context changes and adds
> significant delays taking system further away from "the right moment". On a
> single-cpu system the situation is even worse because BPF program won't even be
> able to block the thread of interest.
> 
> This patchset tries to solve this problem by allowing properly marked tracing
> bpf programs to call crash_kexec() kernel function.
> 
> This is a continuation of bpf_panic patchset with initial feedback taken into
> account.
> 
> Changes from RFC:
>   - sysctl knob dropped
>   - using crash_kexec() instead of panic()
>   - using kfuncs instead of adding a new helper
> 
> Artem Savkov (4):
>    bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
>    bpf: add destructive kfunc set
>    selftests/bpf: add destructive kfunc tests
>    bpf: export crash_kexec() as destructive kfunc

First and second patch ccould be folded together into one. The selftest
should be last in series so that if people bisect the test won't fail due
to missing functionality. First one also has a stale comment wrt bpf_panic()
helper.

>   include/linux/bpf.h                           |  1 +
>   include/linux/btf.h                           |  2 +
>   include/uapi/linux/bpf.h                      |  6 +++
>   kernel/bpf/syscall.c                          |  4 +-
>   kernel/bpf/verifier.c                         | 12 ++++++
>   kernel/kexec_core.c                           | 22 ++++++++++
>   net/bpf/test_run.c                            | 12 +++++-
>   tools/include/uapi/linux/bpf.h                |  6 +++
>   .../selftests/bpf/prog_tests/kfunc_call.c     | 41 +++++++++++++++++++
>   .../bpf/progs/kfunc_call_destructive.c        | 14 +++++++
>   10 files changed, 118 insertions(+), 2 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
  2022-07-20 11:46 ` [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD Artem Savkov
@ 2022-07-21 14:02   ` Alexei Starovoitov
  2022-07-22  4:18     ` Artem Savkov
  0 siblings, 1 reply; 11+ messages in thread
From: Alexei Starovoitov @ 2022-07-21 14:02 UTC (permalink / raw)
  To: Artem Savkov
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Network Development, LKML, Andrea Arcangeli, Daniel Vacek,
	Jiri Olsa, Song Liu

On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote:
>
> +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program
> + * will be able to perform destructive operations such as calling bpf_panic()
> + * helper.
> + */
> +#define BPF_F_DESTRUCTIVE      (1U << 6)

I don't understand what value this flag provides.

bpf prog won't be using kexec accidentally.
Requiring user space to also pass this flag seems pointless.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
  2022-07-21 14:02   ` Alexei Starovoitov
@ 2022-07-22  4:18     ` Artem Savkov
  2022-07-22  4:32       ` Alexei Starovoitov
  0 siblings, 1 reply; 11+ messages in thread
From: Artem Savkov @ 2022-07-22  4:18 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Network Development, LKML, Andrea Arcangeli, Daniel Vacek,
	Jiri Olsa, Song Liu

On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote:
> On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote:
> >
> > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program
> > + * will be able to perform destructive operations such as calling bpf_panic()
> > + * helper.
> > + */
> > +#define BPF_F_DESTRUCTIVE      (1U << 6)
> 
> I don't understand what value this flag provides.
> 
> bpf prog won't be using kexec accidentally.
> Requiring user space to also pass this flag seems pointless.

bpf program likely won't. But I think it is not uncommon for people to
run bpftrace scripts they fetched off the internet to run them without
fully reading the code. So the idea was to provide intermediate tools
like that with a common way to confirm user's intent without
implementing their own guards around dangerous calls.
If that is not a good enough of a reason to add the flag I can drop it.

-- 
 Artem


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
  2022-07-22  4:18     ` Artem Savkov
@ 2022-07-22  4:32       ` Alexei Starovoitov
  2022-07-25  9:27         ` Artem Savkov
  2022-07-25 19:23         ` Daniel Xu
  0 siblings, 2 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2022-07-22  4:32 UTC (permalink / raw)
  To: Artem Savkov
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Network Development, LKML, Andrea Arcangeli, Daniel Vacek,
	Jiri Olsa, Song Liu

On Thu, Jul 21, 2022 at 9:18 PM Artem Savkov <asavkov@redhat.com> wrote:
>
> On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote:
> > On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote:
> > >
> > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program
> > > + * will be able to perform destructive operations such as calling bpf_panic()
> > > + * helper.
> > > + */
> > > +#define BPF_F_DESTRUCTIVE      (1U << 6)
> >
> > I don't understand what value this flag provides.
> >
> > bpf prog won't be using kexec accidentally.
> > Requiring user space to also pass this flag seems pointless.
>
> bpf program likely won't. But I think it is not uncommon for people to
> run bpftrace scripts they fetched off the internet to run them without
> fully reading the code. So the idea was to provide intermediate tools
> like that with a common way to confirm user's intent without
> implementing their own guards around dangerous calls.
> If that is not a good enough of a reason to add the flag I can drop it.

The intent makes sense, but bpftrace will set the flag silently.
Since bpftrace compiles the prog it knows what helpers are being
called, so it will have to pass that extra flag automatically anyway.
You can argue that bpftrace needs to require a mandatory cmdline flag
from users to run such scripts, but even if you convince the bpftrace
community to do that everybody else might just ignore that request.
Any tool (even libbpf) can scan the insns and provide flags.

Long ago we added the 'kern_version' field to the prog_load command.
The intent was to tie bpf prog with kernel version.
Soon enough people started querying the kernel and put that
version in there ignoring SEC("version") that bpf prog had.
It took years to clean that up.
BPF_F_DESTRUCTIVE flag looks similar to me.
Good intent, but unlikely to achieve the goal.

Do you have other ideas to achieve the goal:
'cannot run destructive prog by accident' ?

If we had an UI it would be a question 'are you sure? please type: yes'.

I hate to propose the following, since it will delay your patch
for a long time, but maybe we should only allow signed bpf programs
to be destructive?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
  2022-07-22  4:32       ` Alexei Starovoitov
@ 2022-07-25  9:27         ` Artem Savkov
  2022-07-25 19:23         ` Daniel Xu
  1 sibling, 0 replies; 11+ messages in thread
From: Artem Savkov @ 2022-07-25  9:27 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Network Development, LKML, Andrea Arcangeli, Daniel Vacek,
	Jiri Olsa, Song Liu

On Thu, Jul 21, 2022 at 09:32:51PM -0700, Alexei Starovoitov wrote:
> On Thu, Jul 21, 2022 at 9:18 PM Artem Savkov <asavkov@redhat.com> wrote:
> >
> > On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote:
> > > On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote:
> > > >
> > > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program
> > > > + * will be able to perform destructive operations such as calling bpf_panic()
> > > > + * helper.
> > > > + */
> > > > +#define BPF_F_DESTRUCTIVE      (1U << 6)
> > >
> > > I don't understand what value this flag provides.
> > >
> > > bpf prog won't be using kexec accidentally.
> > > Requiring user space to also pass this flag seems pointless.
> >
> > bpf program likely won't. But I think it is not uncommon for people to
> > run bpftrace scripts they fetched off the internet to run them without
> > fully reading the code. So the idea was to provide intermediate tools
> > like that with a common way to confirm user's intent without
> > implementing their own guards around dangerous calls.
> > If that is not a good enough of a reason to add the flag I can drop it.
> 
> The intent makes sense, but bpftrace will set the flag silently.
> Since bpftrace compiles the prog it knows what helpers are being
> called, so it will have to pass that extra flag automatically anyway.
> You can argue that bpftrace needs to require a mandatory cmdline flag
> from users to run such scripts, but even if you convince the bpftrace
> community to do that everybody else might just ignore that request.
> Any tool (even libbpf) can scan the insns and provide flags.
> 
> Long ago we added the 'kern_version' field to the prog_load command.
> The intent was to tie bpf prog with kernel version.
> Soon enough people started querying the kernel and put that
> version in there ignoring SEC("version") that bpf prog had.
> It took years to clean that up.
> BPF_F_DESTRUCTIVE flag looks similar to me.
> Good intent, but unlikely to achieve the goal.

Good point, I only thought of those who would like to use this, not the
ones who would try to work around it.

> Do you have other ideas to achieve the goal:
> 'cannot run destructive prog by accident' ?
> 
> If we had an UI it would be a question 'are you sure? please type: yes'.
> 
> I hate to propose the following, since it will delay your patch
> for a long time, but maybe we should only allow signed bpf programs
> to be destructive?

Anything I can think of is likely to be as easily defeated as the flag,
requirement for destructive programs to be signed is not. So I like the
idea. However I think that if bpf program signature checking is disabled
on the system then destructive programs should be able to run with just
CAP_SYS_BOOT. So maybe we can treat everything as this case until we
have the ability to sign bpf programs?

-- 
 Artem


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
  2022-07-22  4:32       ` Alexei Starovoitov
  2022-07-25  9:27         ` Artem Savkov
@ 2022-07-25 19:23         ` Daniel Xu
  1 sibling, 0 replies; 11+ messages in thread
From: Daniel Xu @ 2022-07-25 19:23 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Artem Savkov, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, bpf, Network Development, LKML,
	Andrea Arcangeli, Daniel Vacek, Jiri Olsa, Song Liu

On Thu, Jul 21, 2022 at 09:32:51PM -0700, Alexei Starovoitov wrote:
> On Thu, Jul 21, 2022 at 9:18 PM Artem Savkov <asavkov@redhat.com> wrote:
> >
> > On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote:
> > > On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote:
> > > >
> > > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program
> > > > + * will be able to perform destructive operations such as calling bpf_panic()
> > > > + * helper.
> > > > + */
> > > > +#define BPF_F_DESTRUCTIVE      (1U << 6)
> > >
> > > I don't understand what value this flag provides.
> > >
> > > bpf prog won't be using kexec accidentally.
> > > Requiring user space to also pass this flag seems pointless.
> >
> > bpf program likely won't. But I think it is not uncommon for people to
> > run bpftrace scripts they fetched off the internet to run them without
> > fully reading the code. So the idea was to provide intermediate tools
> > like that with a common way to confirm user's intent without
> > implementing their own guards around dangerous calls.
> > If that is not a good enough of a reason to add the flag I can drop it.
>
> The intent makes sense, but bpftrace will set the flag silently.
> Since bpftrace compiles the prog it knows what helpers are being
> called, so it will have to pass that extra flag automatically anyway.
> You can argue that bpftrace needs to require a mandatory cmdline flag
> from users to run such scripts, but even if you convince the bpftrace
> community to do that everybody else might just ignore that request.
> Any tool (even libbpf) can scan the insns and provide flags.

FWIW I added --unsafe flag to bpftrace a while ago for
situations/helpers such as these. So this load flag would work OK for
bpftrace.

[...]
> Do you have other ideas to achieve the goal:
> 'cannot run destructive prog by accident' ?
>
> If we had an UI it would be a question 'are you sure? please type: yes'.
>
> I hate to propose the following, since it will delay your patch
> for a long time, but maybe we should only allow signed bpf programs
> to be destructive?

I don't have any opinion on the signing part but I do think it'd be nice
if there was some sort of opt-in mechanism. It wouldn't be very nice if
some arbitrary tracing tool panicked my machine. But I suppose tracing
programs could already do some significant damage by bpf_send_signal()ing
random processes.

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-07-25 19:23 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-20 11:46 [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Artem Savkov
2022-07-20 11:46 ` [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD Artem Savkov
2022-07-21 14:02   ` Alexei Starovoitov
2022-07-22  4:18     ` Artem Savkov
2022-07-22  4:32       ` Alexei Starovoitov
2022-07-25  9:27         ` Artem Savkov
2022-07-25 19:23         ` Daniel Xu
2022-07-20 11:46 ` [PATCH bpf-next 2/4] bpf: add destructive kfunc set Artem Savkov
2022-07-20 11:46 ` [PATCH bpf-next 3/4] selftests/bpf: add destructive kfunc tests Artem Savkov
2022-07-20 11:46 ` [PATCH bpf-next 4/4] bpf: export crash_kexec() as destructive kfunc Artem Savkov
2022-07-21 13:00 ` [PATCH bpf-next 0/4] destructive bpf kfuncs (was: bpf_panic) Daniel Borkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).