* [PATCH bpf 0/2] Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs
@ 2021-11-08 16:46 Dmitrii Banshchikov
2021-11-08 16:46 ` [PATCH bpf 1/2] bpf: " Dmitrii Banshchikov
2021-11-08 16:46 ` [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers Dmitrii Banshchikov
0 siblings, 2 replies; 9+ messages in thread
From: Dmitrii Banshchikov @ 2021-11-08 16:46 UTC (permalink / raw)
To: bpf
Cc: Dmitrii Banshchikov, ast, daniel, andrii, kafai, songliubraving,
yhs, john.fastabend, kpsingh, netdev, rdna, john.stultz, sboyd,
peterz, mark.rutland, rosted
syzbot found a locking issue with bpf_ktime_get_coarse_ns() helper
executed in BPF_PROG_TYPE_PERF_EVENT prog type - [1]. The issue is
possible because the helper uses non fast version of time accessors
which isn't safe for any context. The helper was added because it
provides performance benefits in comparison to bpf_ktime_get_ns().
Forbid use of bpf_ktime_get_coarse_ns() helper in tracing progs.
The same issue is possible with bpf_timer_* set of helpers - forbid
their usage in tracing progs too.
In the discussion it was stated that bpf_spin_lock releated helpers
shall also be excluded for tracing progs. This is already done in a
different way - by compatibility check between a map and a program. The
verifier fails if a tracing program tries to use a map which value has
struct bpf_spin_lock. This prevents using bpf_spin_lock in tracing
progs.
Patch 1 adds allowance checks for helpers
Patch 2 adds tests
1. https://lore.kernel.org/all/00000000000013aebd05cff8e064@google.com/
Dmitrii Banshchikov (2):
bpf: Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs
selftests/bpf: Add tests for allowed helpers
kernel/bpf/helpers.c | 30 +++
tools/testing/selftests/bpf/test_verifier.c | 36 +++-
.../selftests/bpf/verifier/helper_allowed.c | 196 ++++++++++++++++++
3 files changed, 261 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/verifier/helper_allowed.c
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf 1/2] bpf: Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs
2021-11-08 16:46 [PATCH bpf 0/2] Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs Dmitrii Banshchikov
@ 2021-11-08 16:46 ` Dmitrii Banshchikov
2021-11-08 16:55 ` Denis Kirjanov
2021-11-09 21:52 ` Alexei Starovoitov
2021-11-08 16:46 ` [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers Dmitrii Banshchikov
1 sibling, 2 replies; 9+ messages in thread
From: Dmitrii Banshchikov @ 2021-11-08 16:46 UTC (permalink / raw)
To: bpf
Cc: Dmitrii Banshchikov, ast, daniel, andrii, kafai, songliubraving,
yhs, john.fastabend, kpsingh, netdev, rdna, john.stultz, sboyd,
peterz, mark.rutland, rosted, syzbot+43fd005b5a1b4d10781e
bpf_ktime_get_coarse_ns() helper uses ktime_get_coarse_ns() time
accessor that isn't safe for any context.
This results in locking issues:
======================================================
WARNING: possible circular locking dependency detected
5.15.0-syzkaller #0 Not tainted
------------------------------------------------------
syz-executor.4/14877 is trying to acquire lock:
ffffffff8cb30008 (tk_core.seq.seqcount){----}-{0:0}, at: ktime_get_coarse_ts64+0x25/0x110 kernel/time/timekeeping.c:2255
but task is already holding lock:
ffffffff90dbf200 (&obj_hash[i].lock){-.-.}-{2:2}, at: debug_object_deactivate+0x61/0x400 lib/debugobjects.c:735
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&obj_hash[i].lock){-.-.}-{2:2}:
lock_acquire+0x19f/0x4d0 kernel/locking/lockdep.c:5625
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0xd1/0x120 kernel/locking/spinlock.c:162
__debug_object_init+0xd9/0x1860 lib/debugobjects.c:569
debug_hrtimer_init kernel/time/hrtimer.c:414 [inline]
debug_init kernel/time/hrtimer.c:468 [inline]
hrtimer_init+0x20/0x40 kernel/time/hrtimer.c:1592
ntp_init_cmos_sync kernel/time/ntp.c:676 [inline]
ntp_init+0xa1/0xad kernel/time/ntp.c:1095
timekeeping_init+0x512/0x6bf kernel/time/timekeeping.c:1639
start_kernel+0x267/0x56e init/main.c:1030
secondary_startup_64_no_verify+0xb1/0xbb
-> #0 (tk_core.seq.seqcount){----}-{0:0}:
check_prev_add kernel/locking/lockdep.c:3051 [inline]
check_prevs_add kernel/locking/lockdep.c:3174 [inline]
validate_chain+0x1dfb/0x8240 kernel/locking/lockdep.c:3789
__lock_acquire+0x1382/0x2b00 kernel/locking/lockdep.c:5015
lock_acquire+0x19f/0x4d0 kernel/locking/lockdep.c:5625
seqcount_lockdep_reader_access+0xfe/0x230 include/linux/seqlock.h:103
ktime_get_coarse_ts64+0x25/0x110 kernel/time/timekeeping.c:2255
ktime_get_coarse include/linux/timekeeping.h:120 [inline]
ktime_get_coarse_ns include/linux/timekeeping.h:126 [inline]
____bpf_ktime_get_coarse_ns kernel/bpf/helpers.c:173 [inline]
bpf_ktime_get_coarse_ns+0x7e/0x130 kernel/bpf/helpers.c:171
bpf_prog_a99735ebafdda2f1+0x10/0xb50
bpf_dispatcher_nop_func include/linux/bpf.h:721 [inline]
__bpf_prog_run include/linux/filter.h:626 [inline]
bpf_prog_run include/linux/filter.h:633 [inline]
BPF_PROG_RUN_ARRAY include/linux/bpf.h:1294 [inline]
trace_call_bpf+0x2cf/0x5d0 kernel/trace/bpf_trace.c:127
perf_trace_run_bpf_submit+0x7b/0x1d0 kernel/events/core.c:9708
perf_trace_lock+0x37c/0x440 include/trace/events/lock.h:39
trace_lock_release+0x128/0x150 include/trace/events/lock.h:58
lock_release+0x82/0x810 kernel/locking/lockdep.c:5636
__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:149 [inline]
_raw_spin_unlock_irqrestore+0x75/0x130 kernel/locking/spinlock.c:194
debug_hrtimer_deactivate kernel/time/hrtimer.c:425 [inline]
debug_deactivate kernel/time/hrtimer.c:481 [inline]
__run_hrtimer kernel/time/hrtimer.c:1653 [inline]
__hrtimer_run_queues+0x2f9/0xa60 kernel/time/hrtimer.c:1749
hrtimer_interrupt+0x3b3/0x1040 kernel/time/hrtimer.c:1811
local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1086 [inline]
__sysvec_apic_timer_interrupt+0xf9/0x270 arch/x86/kernel/apic/apic.c:1103
sysvec_apic_timer_interrupt+0x8c/0xb0 arch/x86/kernel/apic/apic.c:1097
asm_sysvec_apic_timer_interrupt+0x12/0x20
__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:152 [inline]
_raw_spin_unlock_irqrestore+0xd4/0x130 kernel/locking/spinlock.c:194
try_to_wake_up+0x702/0xd20 kernel/sched/core.c:4118
wake_up_process kernel/sched/core.c:4200 [inline]
wake_up_q+0x9a/0xf0 kernel/sched/core.c:953
futex_wake+0x50f/0x5b0 kernel/futex/waitwake.c:184
do_futex+0x367/0x560 kernel/futex/syscalls.c:127
__do_sys_futex kernel/futex/syscalls.c:199 [inline]
__se_sys_futex+0x401/0x4b0 kernel/futex/syscalls.c:180
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
Locking issues are also possible with bpf_timer_* set of helpers:
hrtimer_start()
lock_base();
trace_hrtimer...()
perf_event()
bpf_run()
bpf_timer_start()
hrtimer_start()
lock_base() <- DEADLOCK
Forbid use of bpf_ktime_get_coarse_ns() and bpf_timer_* helpers in
BPF_PROG_TYPE_KPROBE, BPF_PROG_TYPE_TRACEPOINT, BPF_PROG_TYPE_PERF_EVENT
and BPF_PROG_TYPE_RAW_TRACEPOINT prog types as it may result locking
issues.
Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
Reported-by: syzbot+43fd005b5a1b4d10781e@syzkaller.appspotmail.com
---
kernel/bpf/helpers.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 1ffd469c217f..3de24928166e 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -18,6 +18,19 @@
#include "../../lib/kstrtox.h"
+static bool is_tracing_prog_type(enum bpf_prog_type type)
+{
+ switch (type) {
+ case BPF_PROG_TYPE_KPROBE:
+ case BPF_PROG_TYPE_TRACEPOINT:
+ case BPF_PROG_TYPE_PERF_EVENT:
+ case BPF_PROG_TYPE_RAW_TRACEPOINT:
+ return true;
+ default:
+ return false;
+ }
+}
+
/* If kernel subsystem is allowing eBPF programs to call this function,
* inside its own verifier_ops->get_func_proto() callback it should return
* bpf_map_lookup_elem_proto, so that verifier can properly check the arguments
@@ -173,10 +186,18 @@ BPF_CALL_0(bpf_ktime_get_coarse_ns)
return ktime_get_coarse_ns();
}
+static bool bpf_ktime_get_coarse_ns_allowed(const struct bpf_prog *prog)
+{
+ // Forbid prog types that might be non-safe for non-fast variants of time accessors
+
+ return !is_tracing_prog_type(prog->type);
+}
+
const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto = {
.func = bpf_ktime_get_coarse_ns,
.gpl_only = false,
.ret_type = RET_INTEGER,
+ .allowed = bpf_ktime_get_coarse_ns_allowed,
};
BPF_CALL_0(bpf_get_current_pid_tgid)
@@ -1140,6 +1161,11 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
return ret;
}
+static bool bpf_timer_allowed(const struct bpf_prog *prog)
+{
+ return !is_tracing_prog_type(prog->type);
+}
+
static const struct bpf_func_proto bpf_timer_init_proto = {
.func = bpf_timer_init,
.gpl_only = true,
@@ -1147,6 +1173,7 @@ static const struct bpf_func_proto bpf_timer_init_proto = {
.arg1_type = ARG_PTR_TO_TIMER,
.arg2_type = ARG_CONST_MAP_PTR,
.arg3_type = ARG_ANYTHING,
+ .allowed = bpf_timer_allowed,
};
BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callback_fn,
@@ -1200,6 +1227,7 @@ static const struct bpf_func_proto bpf_timer_set_callback_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_TIMER,
.arg2_type = ARG_PTR_TO_FUNC,
+ .allowed = bpf_timer_allowed,
};
BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, flags)
@@ -1230,6 +1258,7 @@ static const struct bpf_func_proto bpf_timer_start_proto = {
.arg1_type = ARG_PTR_TO_TIMER,
.arg2_type = ARG_ANYTHING,
.arg3_type = ARG_ANYTHING,
+ .allowed = bpf_timer_allowed,
};
static void drop_prog_refcnt(struct bpf_hrtimer *t)
@@ -1279,6 +1308,7 @@ static const struct bpf_func_proto bpf_timer_cancel_proto = {
.gpl_only = true,
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_TIMER,
+ .allowed = bpf_timer_allowed,
};
/* This function is called by map_delete/update_elem for individual element and
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers
2021-11-08 16:46 [PATCH bpf 0/2] Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs Dmitrii Banshchikov
2021-11-08 16:46 ` [PATCH bpf 1/2] bpf: " Dmitrii Banshchikov
@ 2021-11-08 16:46 ` Dmitrii Banshchikov
2021-11-09 6:48 ` Dmitrii Banshchikov
1 sibling, 1 reply; 9+ messages in thread
From: Dmitrii Banshchikov @ 2021-11-08 16:46 UTC (permalink / raw)
To: bpf
Cc: Dmitrii Banshchikov, ast, daniel, andrii, kafai, songliubraving,
yhs, john.fastabend, kpsingh, netdev, rdna, john.stultz, sboyd,
peterz, mark.rutland, rosted
This patch adds tests that bpf_ktime_get_coarse_ns() and bpf_timer_* and
bpf_spin_lock()/bpf_spin_unlock() helpers are forbidden in tracing
progs as it may result in various locking issues.
Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
---
tools/testing/selftests/bpf/test_verifier.c | 36 +++-
.../selftests/bpf/verifier/helper_allowed.c | 196 ++++++++++++++++++
2 files changed, 231 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/verifier/helper_allowed.c
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 25afe423b3f0..e16eab6fc3a9 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -92,6 +92,7 @@ struct bpf_test {
int fixup_map_event_output[MAX_FIXUPS];
int fixup_map_reuseport_array[MAX_FIXUPS];
int fixup_map_ringbuf[MAX_FIXUPS];
+ int fixup_map_timer[MAX_FIXUPS];
/* Expected verifier log output for result REJECT or VERBOSE_ACCEPT.
* Can be a tab-separated sequence of expected strings. An empty string
* means no log verification.
@@ -605,7 +606,7 @@ static int create_cgroup_storage(bool percpu)
* struct bpf_spin_lock l;
* };
*/
-static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l";
+static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l\0bpf_timer\0";
static __u32 btf_raw_types[] = {
/* int */
BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
@@ -616,6 +617,8 @@ static __u32 btf_raw_types[] = {
BTF_TYPE_ENC(15, BTF_INFO_ENC(BTF_KIND_STRUCT, 0, 2), 8),
BTF_MEMBER_ENC(19, 1, 0), /* int cnt; */
BTF_MEMBER_ENC(23, 2, 32),/* struct bpf_spin_lock l; */
+ /* struct bpf_timer */ /* [4] */
+ BTF_TYPE_ENC(25, BTF_INFO_ENC(BTF_KIND_STRUCT, 0, 0), 16),
};
static int load_btf(void)
@@ -696,6 +699,29 @@ static int create_sk_storage_map(void)
return fd;
}
+static int create_map_timer(void)
+{
+ struct bpf_create_map_attr attr = {
+ .name = "test_map",
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .key_size = 4,
+ .value_size = 16,
+ .max_entries = 1,
+ .btf_key_type_id = 1,
+ .btf_value_type_id = 4,
+ };
+ int fd, btf_fd;
+
+ btf_fd = load_btf();
+ if (btf_fd < 0)
+ return -1;
+ attr.btf_fd = btf_fd;
+ fd = bpf_create_map_xattr(&attr);
+ if (fd < 0)
+ printf("Failed to create map with timer\n");
+ return fd;
+}
+
static char bpf_vlog[UINT_MAX >> 8];
static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
@@ -722,6 +748,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
int *fixup_map_event_output = test->fixup_map_event_output;
int *fixup_map_reuseport_array = test->fixup_map_reuseport_array;
int *fixup_map_ringbuf = test->fixup_map_ringbuf;
+ int *fixup_map_timer = test->fixup_map_timer;
if (test->fill_helper) {
test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -907,6 +934,13 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
fixup_map_ringbuf++;
} while (*fixup_map_ringbuf);
}
+ if (*fixup_map_timer) {
+ map_fds[21] = create_map_timer();
+ do {
+ prog[*fixup_map_timer].imm = map_fds[21];
+ fixup_map_timer++;
+ } while (*fixup_map_timer);
+ }
}
struct libcap {
diff --git a/tools/testing/selftests/bpf/verifier/helper_allowed.c b/tools/testing/selftests/bpf/verifier/helper_allowed.c
new file mode 100644
index 000000000000..aeba8ef866a9
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/helper_allowed.c
@@ -0,0 +1,196 @@
+{
+ "bpf_ktime_get_coarse_ns isn't allowed in BPF_PROG_TYPE_KPROBE",
+ .insns = {
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ktime_get_coarse_ns),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .errstr = "helper call is not allowed in probe",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_KPROBE,
+},
+{
+ "bpf_ktime_get_coarse_ns isn't allowed in BPF_PROG_TYPE_TRACEPOINT",
+ .insns = {
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ktime_get_coarse_ns),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .errstr = "helper call is not allowed in probe",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_TRACEPOINT,
+},
+{
+ "bpf_ktime_get_coarse_ns isn't allowed in BPF_PROG_TYPE_PERF_EVENT",
+ .insns = {
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ktime_get_coarse_ns),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .errstr = "helper call is not allowed in probe",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_PERF_EVENT,
+},
+{
+ "bpf_ktime_get_coarse_ns isn't allowed in BPF_PROG_TYPE_RAW_TRACEPOINT",
+ .insns = {
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ktime_get_coarse_ns),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .errstr = "helper call is not allowed in probe",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_RAW_TRACEPOINT,
+},
+{
+ "bpf_timer_init isn't allowed in BPF_PROG_TYPE_KPROBE",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_LD_MAP_FD(BPF_REG_2, 0),
+ BPF_MOV64_IMM(BPF_REG_3, 1),
+ BPF_EMIT_CALL(BPF_FUNC_timer_init),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_timer = { 3, 8 },
+ .errstr = "helper call is not allowed in probe",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_KPROBE,
+},
+{
+ "bpf_timer_init isn't allowed in BPF_PROG_TYPE_PERF_EVENT",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_LD_MAP_FD(BPF_REG_2, 0),
+ BPF_MOV64_IMM(BPF_REG_3, 1),
+ BPF_EMIT_CALL(BPF_FUNC_timer_init),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_timer = { 3, 8 },
+ .errstr = "helper call is not allowed in probe",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_PERF_EVENT,
+},
+{
+ "bpf_timer_init isn't allowed in BPF_PROG_TYPE_TRACEPOINT",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_LD_MAP_FD(BPF_REG_2, 0),
+ BPF_MOV64_IMM(BPF_REG_3, 1),
+ BPF_EMIT_CALL(BPF_FUNC_timer_init),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_timer = { 3, 8 },
+ .errstr = "helper call is not allowed in probe",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_TRACEPOINT,
+},
+{
+ "bpf_timer_init isn't allowed in BPF_PROG_TYPE_RAW_TRACEPOINT",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_LD_MAP_FD(BPF_REG_2, 0),
+ BPF_MOV64_IMM(BPF_REG_3, 1),
+ BPF_EMIT_CALL(BPF_FUNC_timer_init),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_timer = { 3, 8 },
+ .errstr = "helper call is not allowed in probe",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_RAW_TRACEPOINT,
+},
+{
+ "bpf_spin_lock isn't allowed in BPF_PROG_TYPE_KPROBE",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_spin_lock),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_spin_lock = { 3 },
+ .errstr = "tracing progs cannot use bpf_spin_lock yet",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_KPROBE,
+},
+{
+ "bpf_spin_lock isn't allowed in BPF_PROG_TYPE_TRACEPOINT",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_spin_lock),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_spin_lock = { 3 },
+ .errstr = "tracing progs cannot use bpf_spin_lock yet",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_TRACEPOINT,
+},
+{
+ "bpf_spin_lock isn't allowed in BPF_PROG_TYPE_PERF_EVENT",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_spin_lock),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_spin_lock = { 3 },
+ .errstr = "tracing progs cannot use bpf_spin_lock yet",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_PERF_EVENT,
+},
+{
+ "bpf_spin_lock isn't allowed in BPF_PROG_TYPE_RAW_TRACEPOINT",
+ .insns = {
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_spin_lock),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_spin_lock = { 3 },
+ .errstr = "tracing progs cannot use bpf_spin_lock yet",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_RAW_TRACEPOINT,
+},
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] bpf: Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs
2021-11-08 16:46 ` [PATCH bpf 1/2] bpf: " Dmitrii Banshchikov
@ 2021-11-08 16:55 ` Denis Kirjanov
2021-11-09 21:52 ` Alexei Starovoitov
1 sibling, 0 replies; 9+ messages in thread
From: Denis Kirjanov @ 2021-11-08 16:55 UTC (permalink / raw)
To: Dmitrii Banshchikov, bpf
Cc: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
kpsingh, netdev, rdna, john.stultz, sboyd, peterz, mark.rutland,
rosted, syzbot+43fd005b5a1b4d10781e
11/8/21 7:46 PM, Dmitrii Banshchikov пишет:
> bpf_ktime_get_coarse_ns() helper uses ktime_get_coarse_ns() time
> accessor that isn't safe for any context.
> This results in locking issues:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.15.0-syzkaller #0 Not tainted
> ------------------------------------------------------
> syz-executor.4/14877 is trying to acquire lock:
> ffffffff8cb30008 (tk_core.seq.seqcount){----}-{0:0}, at: ktime_get_coarse_ts64+0x25/0x110 kernel/time/timekeeping.c:2255
>
> but task is already holding lock:
> ffffffff90dbf200 (&obj_hash[i].lock){-.-.}-{2:2}, at: debug_object_deactivate+0x61/0x400 lib/debugobjects.c:735
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&obj_hash[i].lock){-.-.}-{2:2}:
> lock_acquire+0x19f/0x4d0 kernel/locking/lockdep.c:5625
> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> _raw_spin_lock_irqsave+0xd1/0x120 kernel/locking/spinlock.c:162
> __debug_object_init+0xd9/0x1860 lib/debugobjects.c:569
> debug_hrtimer_init kernel/time/hrtimer.c:414 [inline]
> debug_init kernel/time/hrtimer.c:468 [inline]
> hrtimer_init+0x20/0x40 kernel/time/hrtimer.c:1592
> ntp_init_cmos_sync kernel/time/ntp.c:676 [inline]
> ntp_init+0xa1/0xad kernel/time/ntp.c:1095
> timekeeping_init+0x512/0x6bf kernel/time/timekeeping.c:1639
> start_kernel+0x267/0x56e init/main.c:1030
> secondary_startup_64_no_verify+0xb1/0xbb
>
> -> #0 (tk_core.seq.seqcount){----}-{0:0}:
> check_prev_add kernel/locking/lockdep.c:3051 [inline]
> check_prevs_add kernel/locking/lockdep.c:3174 [inline]
> validate_chain+0x1dfb/0x8240 kernel/locking/lockdep.c:3789
> __lock_acquire+0x1382/0x2b00 kernel/locking/lockdep.c:5015
> lock_acquire+0x19f/0x4d0 kernel/locking/lockdep.c:5625
> seqcount_lockdep_reader_access+0xfe/0x230 include/linux/seqlock.h:103
> ktime_get_coarse_ts64+0x25/0x110 kernel/time/timekeeping.c:2255
> ktime_get_coarse include/linux/timekeeping.h:120 [inline]
> ktime_get_coarse_ns include/linux/timekeeping.h:126 [inline]
> ____bpf_ktime_get_coarse_ns kernel/bpf/helpers.c:173 [inline]
> bpf_ktime_get_coarse_ns+0x7e/0x130 kernel/bpf/helpers.c:171
> bpf_prog_a99735ebafdda2f1+0x10/0xb50
> bpf_dispatcher_nop_func include/linux/bpf.h:721 [inline]
> __bpf_prog_run include/linux/filter.h:626 [inline]
> bpf_prog_run include/linux/filter.h:633 [inline]
> BPF_PROG_RUN_ARRAY include/linux/bpf.h:1294 [inline]
> trace_call_bpf+0x2cf/0x5d0 kernel/trace/bpf_trace.c:127
> perf_trace_run_bpf_submit+0x7b/0x1d0 kernel/events/core.c:9708
> perf_trace_lock+0x37c/0x440 include/trace/events/lock.h:39
> trace_lock_release+0x128/0x150 include/trace/events/lock.h:58
> lock_release+0x82/0x810 kernel/locking/lockdep.c:5636
> __raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:149 [inline]
> _raw_spin_unlock_irqrestore+0x75/0x130 kernel/locking/spinlock.c:194
> debug_hrtimer_deactivate kernel/time/hrtimer.c:425 [inline]
> debug_deactivate kernel/time/hrtimer.c:481 [inline]
> __run_hrtimer kernel/time/hrtimer.c:1653 [inline]
> __hrtimer_run_queues+0x2f9/0xa60 kernel/time/hrtimer.c:1749
> hrtimer_interrupt+0x3b3/0x1040 kernel/time/hrtimer.c:1811
> local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1086 [inline]
> __sysvec_apic_timer_interrupt+0xf9/0x270 arch/x86/kernel/apic/apic.c:1103
> sysvec_apic_timer_interrupt+0x8c/0xb0 arch/x86/kernel/apic/apic.c:1097
> asm_sysvec_apic_timer_interrupt+0x12/0x20
> __raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:152 [inline]
> _raw_spin_unlock_irqrestore+0xd4/0x130 kernel/locking/spinlock.c:194
> try_to_wake_up+0x702/0xd20 kernel/sched/core.c:4118
> wake_up_process kernel/sched/core.c:4200 [inline]
> wake_up_q+0x9a/0xf0 kernel/sched/core.c:953
> futex_wake+0x50f/0x5b0 kernel/futex/waitwake.c:184
> do_futex+0x367/0x560 kernel/futex/syscalls.c:127
> __do_sys_futex kernel/futex/syscalls.c:199 [inline]
> __se_sys_futex+0x401/0x4b0 kernel/futex/syscalls.c:180
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Locking issues are also possible with bpf_timer_* set of helpers:
>
> hrtimer_start()
> lock_base();
> trace_hrtimer...()
> perf_event()
> bpf_run()
> bpf_timer_start()
> hrtimer_start()
> lock_base() <- DEADLOCK
>
> Forbid use of bpf_ktime_get_coarse_ns() and bpf_timer_* helpers in
> BPF_PROG_TYPE_KPROBE, BPF_PROG_TYPE_TRACEPOINT, BPF_PROG_TYPE_PERF_EVENT
> and BPF_PROG_TYPE_RAW_TRACEPOINT prog types as it may result locking
> issues.
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> Reported-by: syzbot+43fd005b5a1b4d10781e@syzkaller.appspotmail.com
Add Fixes tag
> ---
> kernel/bpf/helpers.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 1ffd469c217f..3de24928166e 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -18,6 +18,19 @@
>
> #include "../../lib/kstrtox.h"
>
> +static bool is_tracing_prog_type(enum bpf_prog_type type)
> +{
> + switch (type) {
> + case BPF_PROG_TYPE_KPROBE:
> + case BPF_PROG_TYPE_TRACEPOINT:
> + case BPF_PROG_TYPE_PERF_EVENT:
> + case BPF_PROG_TYPE_RAW_TRACEPOINT:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> /* If kernel subsystem is allowing eBPF programs to call this function,
> * inside its own verifier_ops->get_func_proto() callback it should return
> * bpf_map_lookup_elem_proto, so that verifier can properly check the arguments
> @@ -173,10 +186,18 @@ BPF_CALL_0(bpf_ktime_get_coarse_ns)
> return ktime_get_coarse_ns();
> }
>
> +static bool bpf_ktime_get_coarse_ns_allowed(const struct bpf_prog *prog)
> +{
> + // Forbid prog types that might be non-safe for non-fast variants of time accessors
The comments embaced with /* */
> +
> + return !is_tracing_prog_type(prog->type);
> +}
> +
> const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto = {
> .func = bpf_ktime_get_coarse_ns,
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> + .allowed = bpf_ktime_get_coarse_ns_allowed,
> };
>
> BPF_CALL_0(bpf_get_current_pid_tgid)
> @@ -1140,6 +1161,11 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
> return ret;
> }
>
> +static bool bpf_timer_allowed(const struct bpf_prog *prog)
> +{
> + return !is_tracing_prog_type(prog->type);
> +}
> +
> static const struct bpf_func_proto bpf_timer_init_proto = {
> .func = bpf_timer_init,
> .gpl_only = true,
> @@ -1147,6 +1173,7 @@ static const struct bpf_func_proto bpf_timer_init_proto = {
> .arg1_type = ARG_PTR_TO_TIMER,
> .arg2_type = ARG_CONST_MAP_PTR,
> .arg3_type = ARG_ANYTHING,
> + .allowed = bpf_timer_allowed,
> };
>
> BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callback_fn,
> @@ -1200,6 +1227,7 @@ static const struct bpf_func_proto bpf_timer_set_callback_proto = {
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_TIMER,
> .arg2_type = ARG_PTR_TO_FUNC,
> + .allowed = bpf_timer_allowed,
> };
>
> BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, flags)
> @@ -1230,6 +1258,7 @@ static const struct bpf_func_proto bpf_timer_start_proto = {
> .arg1_type = ARG_PTR_TO_TIMER,
> .arg2_type = ARG_ANYTHING,
> .arg3_type = ARG_ANYTHING,
> + .allowed = bpf_timer_allowed,
> };
>
> static void drop_prog_refcnt(struct bpf_hrtimer *t)
> @@ -1279,6 +1308,7 @@ static const struct bpf_func_proto bpf_timer_cancel_proto = {
> .gpl_only = true,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_TIMER,
> + .allowed = bpf_timer_allowed,
> };
>
> /* This function is called by map_delete/update_elem for individual element and
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers
2021-11-08 16:46 ` [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers Dmitrii Banshchikov
@ 2021-11-09 6:48 ` Dmitrii Banshchikov
2021-11-10 1:16 ` Andrii Nakryiko
0 siblings, 1 reply; 9+ messages in thread
From: Dmitrii Banshchikov @ 2021-11-09 6:48 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
kpsingh, netdev, rdna, john.stultz, sboyd, peterz, mark.rutland,
rosted
On Mon, Nov 08, 2021 at 08:46:20PM +0400, Dmitrii Banshchikov wrote:
> This patch adds tests that bpf_ktime_get_coarse_ns() and bpf_timer_* and
> bpf_spin_lock()/bpf_spin_unlock() helpers are forbidden in tracing
> progs as it may result in various locking issues.
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 36 +++-
> .../selftests/bpf/verifier/helper_allowed.c | 196 ++++++++++++++++++
> 2 files changed, 231 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/helper_allowed.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 25afe423b3f0..e16eab6fc3a9 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -92,6 +92,7 @@ struct bpf_test {
> int fixup_map_event_output[MAX_FIXUPS];
> int fixup_map_reuseport_array[MAX_FIXUPS];
> int fixup_map_ringbuf[MAX_FIXUPS];
> + int fixup_map_timer[MAX_FIXUPS];
> /* Expected verifier log output for result REJECT or VERBOSE_ACCEPT.
> * Can be a tab-separated sequence of expected strings. An empty string
> * means no log verification.
> @@ -605,7 +606,7 @@ static int create_cgroup_storage(bool percpu)
> * struct bpf_spin_lock l;
> * };
> */
> -static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l";
> +static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l\0bpf_timer\0";
There is extra null byte at the end.
--
Dmitrii Banshchikov
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] bpf: Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs
2021-11-08 16:46 ` [PATCH bpf 1/2] bpf: " Dmitrii Banshchikov
2021-11-08 16:55 ` Denis Kirjanov
@ 2021-11-09 21:52 ` Alexei Starovoitov
1 sibling, 0 replies; 9+ messages in thread
From: Alexei Starovoitov @ 2021-11-09 21:52 UTC (permalink / raw)
To: Dmitrii Banshchikov
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Network Development, Andrey Ignatov, John Stultz,
sboyd, Peter Zijlstra, Mark Rutland, Steven Rostedt, syzbot
On Mon, Nov 8, 2021 at 8:46 AM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> bpf_ktime_get_coarse_ns() helper uses ktime_get_coarse_ns() time
> accessor that isn't safe for any context.
> This results in locking issues:
Please trim the cc. vger thinks it's a spam.
The patch didn't reach the mailing list or patchwork.
> const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto = {
> .func = bpf_ktime_get_coarse_ns,
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> + .allowed = bpf_ktime_get_coarse_ns_allowed,
I think it's easier to move to networking prog types
instead of going through a callback.
> static const struct bpf_func_proto bpf_timer_init_proto = {
> .func = bpf_timer_init,
> .gpl_only = true,
> @@ -1147,6 +1173,7 @@ static const struct bpf_func_proto bpf_timer_init_proto = {
> .arg1_type = ARG_PTR_TO_TIMER,
> .arg2_type = ARG_CONST_MAP_PTR,
> .arg3_type = ARG_ANYTHING,
> + .allowed = bpf_timer_allowed,
I think disabling timers in check_map_prog_compatibility
(similar to how bpf_spin_lock is restricted) would be cleaner.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers
2021-11-09 6:48 ` Dmitrii Banshchikov
@ 2021-11-10 1:16 ` Andrii Nakryiko
2021-11-10 7:32 ` Dmitrii Banshchikov
0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2021-11-10 1:16 UTC (permalink / raw)
To: Dmitrii Banshchikov
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin Lau, Song Liu, Yonghong Song, john fastabend, KP Singh,
Networking, Andrey Ignatov, john.stultz, sboyd, Peter Ziljstra,
Mark Rutland, rosted
On Mon, Nov 8, 2021 at 10:48 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> On Mon, Nov 08, 2021 at 08:46:20PM +0400, Dmitrii Banshchikov wrote:
> > This patch adds tests that bpf_ktime_get_coarse_ns() and bpf_timer_* and
> > bpf_spin_lock()/bpf_spin_unlock() helpers are forbidden in tracing
> > progs as it may result in various locking issues.
> >
> > Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> > ---
> > tools/testing/selftests/bpf/test_verifier.c | 36 +++-
> > .../selftests/bpf/verifier/helper_allowed.c | 196 ++++++++++++++++++
> > 2 files changed, 231 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/bpf/verifier/helper_allowed.c
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 25afe423b3f0..e16eab6fc3a9 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -92,6 +92,7 @@ struct bpf_test {
> > int fixup_map_event_output[MAX_FIXUPS];
> > int fixup_map_reuseport_array[MAX_FIXUPS];
> > int fixup_map_ringbuf[MAX_FIXUPS];
> > + int fixup_map_timer[MAX_FIXUPS];
> > /* Expected verifier log output for result REJECT or VERBOSE_ACCEPT.
> > * Can be a tab-separated sequence of expected strings. An empty string
> > * means no log verification.
> > @@ -605,7 +606,7 @@ static int create_cgroup_storage(bool percpu)
> > * struct bpf_spin_lock l;
> > * };
> > */
> > -static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l";
> > +static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l\0bpf_timer\0";
>
> There is extra null byte at the end.
Won't hurt, probably. But I wonder if it will be much easier to add
all those programs as C code and test from test_progs? Instead of all
this assembly.
You can put all of them into a single file and have loop that disabled
all but one program at a time (using bpf_program__set_autoload()) and
loading it and validating that the load failed. WDYT?
>
>
> --
>
> Dmitrii Banshchikov
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers
2021-11-10 1:16 ` Andrii Nakryiko
@ 2021-11-10 7:32 ` Dmitrii Banshchikov
2021-11-10 16:52 ` Alexei Starovoitov
0 siblings, 1 reply; 9+ messages in thread
From: Dmitrii Banshchikov @ 2021-11-10 7:32 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin Lau, Song Liu, Yonghong Song, john fastabend, KP Singh,
Networking, Andrey Ignatov, john.stultz, sboyd, Peter Ziljstra,
Mark Rutland, rosted
On Tue, Nov 09, 2021 at 05:16:14PM -0800, Andrii Nakryiko wrote:
> On Mon, Nov 8, 2021 at 10:48 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
> >
> > On Mon, Nov 08, 2021 at 08:46:20PM +0400, Dmitrii Banshchikov wrote:
> > > This patch adds tests that bpf_ktime_get_coarse_ns() and bpf_timer_* and
> > > bpf_spin_lock()/bpf_spin_unlock() helpers are forbidden in tracing
> > > progs as it may result in various locking issues.
> > >
> > > Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> > > ---
> > > tools/testing/selftests/bpf/test_verifier.c | 36 +++-
> > > .../selftests/bpf/verifier/helper_allowed.c | 196 ++++++++++++++++++
> > > 2 files changed, 231 insertions(+), 1 deletion(-)
> > > create mode 100644 tools/testing/selftests/bpf/verifier/helper_allowed.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > > index 25afe423b3f0..e16eab6fc3a9 100644
> > > --- a/tools/testing/selftests/bpf/test_verifier.c
> > > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > > @@ -92,6 +92,7 @@ struct bpf_test {
> > > int fixup_map_event_output[MAX_FIXUPS];
> > > int fixup_map_reuseport_array[MAX_FIXUPS];
> > > int fixup_map_ringbuf[MAX_FIXUPS];
> > > + int fixup_map_timer[MAX_FIXUPS];
> > > /* Expected verifier log output for result REJECT or VERBOSE_ACCEPT.
> > > * Can be a tab-separated sequence of expected strings. An empty string
> > > * means no log verification.
> > > @@ -605,7 +606,7 @@ static int create_cgroup_storage(bool percpu)
> > > * struct bpf_spin_lock l;
> > > * };
> > > */
> > > -static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l";
> > > +static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l\0bpf_timer\0";
> >
> > There is extra null byte at the end.
>
> Won't hurt, probably. But I wonder if it will be much easier to add
> all those programs as C code and test from test_progs? Instead of all
> this assembly.
>
> You can put all of them into a single file and have loop that disabled
> all but one program at a time (using bpf_program__set_autoload()) and
> loading it and validating that the load failed. WDYT?
Will give it a try, thanks.
--
Dmitrii Banshchikov
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers
2021-11-10 7:32 ` Dmitrii Banshchikov
@ 2021-11-10 16:52 ` Alexei Starovoitov
0 siblings, 0 replies; 9+ messages in thread
From: Alexei Starovoitov @ 2021-11-10 16:52 UTC (permalink / raw)
To: Dmitrii Banshchikov
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
john fastabend, KP Singh, Networking, Andrey Ignatov,
John Stultz, sboyd, Peter Ziljstra, Mark Rutland, Steven Rostedt
On Tue, Nov 9, 2021 at 11:32 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
> >
> > Won't hurt, probably. But I wonder if it will be much easier to add
> > all those programs as C code and test from test_progs? Instead of all
> > this assembly.
> >
> > You can put all of them into a single file and have loop that disabled
> > all but one program at a time (using bpf_program__set_autoload()) and
> > loading it and validating that the load failed. WDYT?
>
> Will give it a try, thanks.
Please keep asm tests as well. They're useful too.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-10 16:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-08 16:46 [PATCH bpf 0/2] Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs Dmitrii Banshchikov
2021-11-08 16:46 ` [PATCH bpf 1/2] bpf: " Dmitrii Banshchikov
2021-11-08 16:55 ` Denis Kirjanov
2021-11-09 21:52 ` Alexei Starovoitov
2021-11-08 16:46 ` [PATCH bpf 2/2] selftests/bpf: Add tests for allowed helpers Dmitrii Banshchikov
2021-11-09 6:48 ` Dmitrii Banshchikov
2021-11-10 1:16 ` Andrii Nakryiko
2021-11-10 7:32 ` Dmitrii Banshchikov
2021-11-10 16:52 ` Alexei Starovoitov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.