[PATCH bpf-next v1 0/2] Introduce bpf_preempt_{disable,enable}

[PATCH bpf-next v1 0/2] Introduce bpf_preempt_{disable,enable}

[Kumar Kartikeya Dwivedi]

This set introduces two kfuncs, bpf_preempt_disable and
bpf_preempt_enable, which are wrappers around preempt_disable and
preempt_enable in the kernel. These functions allow a BPF program to
have code sections where preemption is disabled. There are multiple use
cases that are served by such a feature, a few are listed below:

1. Writing safe per-CPU alogrithms/data structures that work correctly
   across different contexts.
2. Writing safe per-CPU allocators similar to bpf_memalloc on top of
   array/arena memory blobs.
3. Writing locking algorithms in BPF programs natively.

Note that local_irq_disable/enable equivalent is also needed for proper
IRQ context protection, but that is a more involved change and will be
sent later.

While bpf_preempt_{disable,enable} is not sufficient for all of these
usage scenarios on its own, it is still necessary.

The same effect as these kfuncs can in some sense be already achieved
using the bpf_spin_lock or rcu_read_lock APIs, therefore from the
standpoint of kernel functionality exposure in the verifier, this is
well understood territory.

Note that these helpers do allow calling kernel helpers and kfuncs from
within the non-preemptible region (unless sleepable). Otherwise, any
locks built using the preemption helpers will be as limited as
existing bpf_spin_lock.

Nesting is allowed by keeping a counter for tracking remaining enables
required to be performed. Similar approach can be applied to
rcu_read_locks in a follow up.

Kumar Kartikeya Dwivedi (2):
  bpf: Introduce bpf_preempt_[disable,enable] kfuncs
  selftests/bpf: Add tests for preempt kfuncs

 include/linux/bpf_verifier.h                  |   1 +
 kernel/bpf/helpers.c                          |  12 ++
 kernel/bpf/verifier.c                         |  72 ++++++++++-
 .../selftests/bpf/prog_tests/preempt_lock.c   |   9 ++
 .../selftests/bpf/progs/preempt_lock.c        | 119 ++++++++++++++++++
 5 files changed, 211 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/preempt_lock.c
 create mode 100644 tools/testing/selftests/bpf/progs/preempt_lock.c


base-commit: a7de265cb2d849f8986a197499ad58dca0a4f209
-- 
2.43.0

[PATCH bpf-next v1 1/2] bpf: Introduce bpf_preempt_[disable,enable] kfuncs

[Kumar Kartikeya Dwivedi]

Introduce two new BPF kfuncs, bpf_preempt_disable and
bpf_preempt_enable. These kfuncs allow disabling preemption in BPF
programs. Nesting is allowed, since the intended use cases includes
building native BPF spin locks without kernel helper involvement. Apart
from that, this can be used to per-CPU data structures for cases where
programs (or userspace) may preempt one or the other. Currently, while
per-CPU access is stable, whether it will be consistent is not
guaranteed, as only migration is disabled for BPF programs.

Global functions are disallowed from being called, but support for them
will be added as a follow up not just preempt kfuncs, but rcu_read_lock
kfuncs as well. Static subprog calls are permitted. Sleepable helpers
and kfuncs are disallowed in non-preemptible regions.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf_verifier.h |  1 +
 kernel/bpf/helpers.c         | 12 ++++++
 kernel/bpf/verifier.c        | 72 +++++++++++++++++++++++++++++++++++-
 3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 36d19cd32eb5..25451d8a0063 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -421,6 +421,7 @@ struct bpf_verifier_state {
 	struct bpf_active_lock active_lock;
 	bool speculative;
 	bool active_rcu_lock;
+	u32 active_preempt_lock;
 	/* If this state was ever pointed-to by other state's loop_entry field
 	 * this flag would be set to true. Used to avoid freeing such states
 	 * while they are still in use.
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 61126180d398..c8e4be4c0d88 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2549,6 +2549,16 @@ __bpf_kfunc void bpf_throw(u64 cookie)
 	WARN(1, "A call to BPF exception callback should never return\n");
 }
 
+__bpf_kfunc void bpf_preempt_disable(void)
+{
+	preempt_disable();
+}
+
+__bpf_kfunc void bpf_preempt_enable(void)
+{
+	preempt_enable();
+}
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(generic_btf_ids)
@@ -2626,6 +2636,8 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
 BTF_ID_FLAGS(func, bpf_dynptr_size)
 BTF_ID_FLAGS(func, bpf_dynptr_clone)
 BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
+BTF_ID_FLAGS(func, bpf_preempt_disable)
+BTF_ID_FLAGS(func, bpf_preempt_enable)
 BTF_KFUNCS_END(common_btf_ids)
 
 static const struct btf_kfunc_id_set common_kfunc_set = {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5a7e34e83a5b..961f8c007cbc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1425,6 +1425,7 @@ static int copy_verifier_state(struct bpf_verifier_state *dst_state,
 	}
 	dst_state->speculative = src->speculative;
 	dst_state->active_rcu_lock = src->active_rcu_lock;
+	dst_state->active_preempt_lock = src->active_preempt_lock;
 	dst_state->curframe = src->curframe;
 	dst_state->active_lock.ptr = src->active_lock.ptr;
 	dst_state->active_lock.id = src->active_lock.id;
@@ -9567,6 +9568,13 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			return -EINVAL;
 		}
 
+		/* Only global subprogs cannot be called with preemption disabled. */
+		if (env->cur_state->active_preempt_lock) {
+			verbose(env, "global function calls are not allowed with preemption disabled,\n"
+				     "use static function instead\n");
+			return -EINVAL;
+		}
+
 		if (err) {
 			verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
 				subprog, sub_name);
@@ -10253,6 +10261,17 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			env->insn_aux_data[insn_idx].storage_get_func_atomic = true;
 	}
 
+	if (env->cur_state->active_preempt_lock) {
+		if (fn->might_sleep) {
+			verbose(env, "sleepable helper %s#%d in non-preemptible region\n",
+				func_id_name(func_id), func_id);
+			return -EINVAL;
+		}
+
+		if (in_sleepable(env) && is_storage_get_function(func_id))
+			env->insn_aux_data[insn_idx].storage_get_func_atomic = true;
+	}
+
 	meta.func_id = func_id;
 	/* check args */
 	for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
@@ -10987,6 +11006,8 @@ enum special_kfunc_type {
 	KF_bpf_percpu_obj_drop_impl,
 	KF_bpf_throw,
 	KF_bpf_iter_css_task_new,
+	KF_bpf_preempt_disable,
+	KF_bpf_preempt_enable,
 };
 
 BTF_SET_START(special_kfunc_set)
@@ -11043,6 +11064,8 @@ BTF_ID(func, bpf_iter_css_task_new)
 #else
 BTF_ID_UNUSED
 #endif
+BTF_ID(func, bpf_preempt_disable)
+BTF_ID(func, bpf_preempt_enable)
 
 static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
 {
@@ -11064,6 +11087,16 @@ static bool is_kfunc_bpf_rcu_read_unlock(struct bpf_kfunc_call_arg_meta *meta)
 	return meta->func_id == special_kfunc_list[KF_bpf_rcu_read_unlock];
 }
 
+static bool is_kfunc_bpf_preempt_disable(struct bpf_kfunc_call_arg_meta *meta)
+{
+	return meta->func_id == special_kfunc_list[KF_bpf_preempt_disable];
+}
+
+static bool is_kfunc_bpf_preempt_enable(struct bpf_kfunc_call_arg_meta *meta)
+{
+	return meta->func_id == special_kfunc_list[KF_bpf_preempt_enable];
+}
+
 static enum kfunc_ptr_arg_type
 get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
 		       struct bpf_kfunc_call_arg_meta *meta,
@@ -12095,11 +12128,11 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
 static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			    int *insn_idx_p)
 {
-	const struct btf_type *t, *ptr_type;
+	bool sleepable, rcu_lock, rcu_unlock, preempt_disable, preempt_enable;
 	u32 i, nargs, ptr_type_id, release_ref_obj_id;
 	struct bpf_reg_state *regs = cur_regs(env);
 	const char *func_name, *ptr_type_name;
-	bool sleepable, rcu_lock, rcu_unlock;
+	const struct btf_type *t, *ptr_type;
 	struct bpf_kfunc_call_arg_meta meta;
 	struct bpf_insn_aux_data *insn_aux;
 	int err, insn_idx = *insn_idx_p;
@@ -12150,6 +12183,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	rcu_lock = is_kfunc_bpf_rcu_read_lock(&meta);
 	rcu_unlock = is_kfunc_bpf_rcu_read_unlock(&meta);
 
+	preempt_disable = is_kfunc_bpf_preempt_disable(&meta);
+	preempt_enable = is_kfunc_bpf_preempt_enable(&meta);
+
 	if (env->cur_state->active_rcu_lock) {
 		struct bpf_func_state *state;
 		struct bpf_reg_state *reg;
@@ -12182,6 +12218,22 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		return -EINVAL;
 	}
 
+	if (env->cur_state->active_preempt_lock) {
+		if (preempt_disable) {
+			env->cur_state->active_preempt_lock++;
+		} else if (preempt_enable) {
+			env->cur_state->active_preempt_lock--;
+		} else if (sleepable) {
+			verbose(env, "kernel func %s is sleepable within non-preemptible region\n", func_name);
+			return -EACCES;
+		}
+	} else if (preempt_disable) {
+		env->cur_state->active_preempt_lock++;
+	} else if (preempt_enable) {
+		verbose(env, "unmatched attempt to enable preemption (kernel function %s)\n", func_name);
+		return -EINVAL;
+	}
+
 	/* In case of release function, we get register number of refcounted
 	 * PTR_TO_BTF_ID in bpf_kfunc_arg_meta, do the release now.
 	 */
@@ -15329,6 +15381,11 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
 		return -EINVAL;
 	}
 
+	if (env->cur_state->active_preempt_lock) {
+		verbose(env, "BPF_LD_[ABS|IND] cannot be used inside bpf_preempt_disable-ed region\n");
+		return -EINVAL;
+	}
+
 	if (regs[ctx_reg].type != PTR_TO_CTX) {
 		verbose(env,
 			"at the time of BPF_LD_ABS|IND R6 != pointer to skb\n");
@@ -16896,6 +16953,9 @@ static bool states_equal(struct bpf_verifier_env *env,
 	if (old->active_rcu_lock != cur->active_rcu_lock)
 		return false;
 
+	if (old->active_preempt_lock != cur->active_preempt_lock)
+		return false;
+
 	/* for states to be equal callsites have to be the same
 	 * and all frame states need to be equivalent
 	 */
@@ -17793,6 +17853,7 @@ static int do_check(struct bpf_verifier_env *env)
 						return -EINVAL;
 					}
 				}
+
 				if (insn->src_reg == BPF_PSEUDO_CALL) {
 					err = check_func_call(env, insn, &env->insn_idx);
 				} else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
@@ -17844,6 +17905,13 @@ static int do_check(struct bpf_verifier_env *env)
 					return -EINVAL;
 				}
 
+				if (env->cur_state->active_preempt_lock && !env->cur_state->curframe) {
+					verbose(env, "%d bpf_preempt_enable%s missing\n",
+						env->cur_state->active_preempt_lock,
+						env->cur_state->active_preempt_lock == 1 ? " is" : "(s) are");
+					return -EINVAL;
+				}
+
 				/* We must do check_reference_leak here before
 				 * prepare_func_exit to handle the case when
 				 * state->curframe > 0, it may be a callback
-- 
2.43.0

Re: [PATCH bpf-next v1 1/2] bpf: Introduce bpf_preempt_[disable,enable] kfuncs

[Alexei Starovoitov]

On Mon, Apr 22, 2024 at 11:19 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
> @@ -10987,6 +11006,8 @@ enum special_kfunc_type {
>         KF_bpf_percpu_obj_drop_impl,
>         KF_bpf_throw,
>         KF_bpf_iter_css_task_new,
> +       KF_bpf_preempt_disable,
> +       KF_bpf_preempt_enable,
>  };
>
>  BTF_SET_START(special_kfunc_set)
> @@ -11043,6 +11064,8 @@ BTF_ID(func, bpf_iter_css_task_new)
>  #else
>  BTF_ID_UNUSED
>  #endif
> +BTF_ID(func, bpf_preempt_disable)
> +BTF_ID(func, bpf_preempt_enable)

I suspect this is broken on !CONFIG_CGROUPS,
since KF_bpf_preempt_disable number won't match the ID in the list.
The simplest fix is to move these two up before bpf_iter_css_task_new.

Re: [PATCH bpf-next v1 1/2] bpf: Introduce bpf_preempt_[disable,enable] kfuncs

[Kumar Kartikeya Dwivedi]

On Tue, 23 Apr 2024 at 16:59, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Apr 22, 2024 at 11:19 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> > @@ -10987,6 +11006,8 @@ enum special_kfunc_type {
> >         KF_bpf_percpu_obj_drop_impl,
> >         KF_bpf_throw,
> >         KF_bpf_iter_css_task_new,
> > +       KF_bpf_preempt_disable,
> > +       KF_bpf_preempt_enable,
> >  };
> >
> >  BTF_SET_START(special_kfunc_set)
> > @@ -11043,6 +11064,8 @@ BTF_ID(func, bpf_iter_css_task_new)
> >  #else
> >  BTF_ID_UNUSED
> >  #endif
> > +BTF_ID(func, bpf_preempt_disable)
> > +BTF_ID(func, bpf_preempt_enable)
>
> I suspect this is broken on !CONFIG_CGROUPS,
> since KF_bpf_preempt_disable number won't match the ID in the list.
> The simplest fix is to move these two up before bpf_iter_css_task_new.

Will fix, thanks for noticing.

[PATCH bpf-next v1 2/2] selftests/bpf: Add tests for preempt kfuncs

[Kumar Kartikeya Dwivedi]

Add tests for nested cases, nested count preservation upon different
subprog calls that disable/enable preemption, and test sleepable helper
call in non-preemptible regions.

181/1   preempt_lock/preempt_lock_missing_1:OK
181/2   preempt_lock/preempt_lock_missing_2:OK
181/3   preempt_lock/preempt_lock_missing_3:OK
181/4   preempt_lock/preempt_lock_missing_3_minus_2:OK
181/5   preempt_lock/preempt_lock_missing_1_subprog:OK
181/6   preempt_lock/preempt_lock_missing_2_subprog:OK
181/7   preempt_lock/preempt_lock_missing_2_minus_1_subprog:OK
181/8   preempt_lock/preempt_balance:OK
181/9   preempt_lock/preempt_balance_subprog_test:OK
181/10  preempt_lock/preempt_sleepable_helper:OK
181     preempt_lock:OK
Summary: 1/10 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/preempt_lock.c   |   9 ++
 .../selftests/bpf/progs/preempt_lock.c        | 119 ++++++++++++++++++
 2 files changed, 128 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/preempt_lock.c
 create mode 100644 tools/testing/selftests/bpf/progs/preempt_lock.c

diff --git a/tools/testing/selftests/bpf/prog_tests/preempt_lock.c b/tools/testing/selftests/bpf/prog_tests/preempt_lock.c
new file mode 100644
index 000000000000..02917c672441
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/preempt_lock.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+#include <preempt_lock.skel.h>
+
+void test_preempt_lock(void)
+{
+	RUN_TESTS(preempt_lock);
+}
diff --git a/tools/testing/selftests/bpf/progs/preempt_lock.c b/tools/testing/selftests/bpf/progs/preempt_lock.c
new file mode 100644
index 000000000000..53320ea80fa4
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/preempt_lock.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+void bpf_preempt_disable(void) __ksym;
+void bpf_preempt_enable(void) __ksym;
+
+SEC("?tc")
+__failure __msg("1 bpf_preempt_enable is missing")
+int preempt_lock_missing_1(struct __sk_buff *ctx)
+{
+	bpf_preempt_disable();
+	return 0;
+}
+
+SEC("?tc")
+__failure __msg("2 bpf_preempt_enable(s) are missing")
+int preempt_lock_missing_2(struct __sk_buff *ctx)
+{
+	bpf_preempt_disable();
+	bpf_preempt_disable();
+	return 0;
+}
+
+SEC("?tc")
+__failure __msg("3 bpf_preempt_enable(s) are missing")
+int preempt_lock_missing_3(struct __sk_buff *ctx)
+{
+	bpf_preempt_disable();
+	bpf_preempt_disable();
+	bpf_preempt_disable();
+	return 0;
+}
+
+SEC("?tc")
+__failure __msg("1 bpf_preempt_enable is missing")
+int preempt_lock_missing_3_minus_2(struct __sk_buff *ctx)
+{
+	bpf_preempt_disable();
+	bpf_preempt_disable();
+	bpf_preempt_disable();
+	bpf_preempt_enable();
+	bpf_preempt_enable();
+	return 0;
+}
+
+static __noinline void preempt_disable(void)
+{
+	bpf_preempt_disable();
+}
+
+static __noinline void preempt_enable(void)
+{
+	bpf_preempt_enable();
+}
+
+SEC("?tc")
+__failure __msg("1 bpf_preempt_enable is missing")
+int preempt_lock_missing_1_subprog(struct __sk_buff *ctx)
+{
+	preempt_disable();
+	return 0;
+}
+
+SEC("?tc")
+__failure __msg("2 bpf_preempt_enable(s) are missing")
+int preempt_lock_missing_2_subprog(struct __sk_buff *ctx)
+{
+	preempt_disable();
+	preempt_disable();
+	return 0;
+}
+
+SEC("?tc")
+__failure __msg("1 bpf_preempt_enable is missing")
+int preempt_lock_missing_2_minus_1_subprog(struct __sk_buff *ctx)
+{
+	preempt_disable();
+	preempt_disable();
+	preempt_enable();
+	return 0;
+}
+
+static __noinline void preempt_balance_subprog(void)
+{
+	preempt_disable();
+	preempt_enable();
+}
+
+SEC("?tc")
+__success int preempt_balance(struct __sk_buff *ctx)
+{
+	bpf_preempt_disable();
+	bpf_preempt_enable();
+	return 0;
+}
+
+SEC("?tc")
+__success int preempt_balance_subprog_test(struct __sk_buff *ctx)
+{
+	preempt_balance_subprog();
+	return 0;
+}
+
+SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
+__failure __msg("sleepable helper bpf_copy_from_user#")
+int preempt_sleepable_helper(void *ctx)
+{
+	u32 data;
+
+	bpf_preempt_disable();
+	bpf_copy_from_user(&data, sizeof(data), NULL);
+	bpf_preempt_enable();
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.43.0

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add tests for preempt kfuncs

[Jiri Olsa]

On Tue, Apr 23, 2024 at 06:19:22AM +0000, Kumar Kartikeya Dwivedi wrote:
> Add tests for nested cases, nested count preservation upon different
> subprog calls that disable/enable preemption, and test sleepable helper
> call in non-preemptible regions.
> 
> 181/1   preempt_lock/preempt_lock_missing_1:OK
> 181/2   preempt_lock/preempt_lock_missing_2:OK
> 181/3   preempt_lock/preempt_lock_missing_3:OK
> 181/4   preempt_lock/preempt_lock_missing_3_minus_2:OK
> 181/5   preempt_lock/preempt_lock_missing_1_subprog:OK
> 181/6   preempt_lock/preempt_lock_missing_2_subprog:OK
> 181/7   preempt_lock/preempt_lock_missing_2_minus_1_subprog:OK
> 181/8   preempt_lock/preempt_balance:OK
> 181/9   preempt_lock/preempt_balance_subprog_test:OK
> 181/10  preempt_lock/preempt_sleepable_helper:OK

should we also check that the global function call is not allowed?

jirka

> 181     preempt_lock:OK
> Summary: 1/10 PASSED, 0 SKIPPED, 0 FAILED
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/preempt_lock.c   |   9 ++
>  .../selftests/bpf/progs/preempt_lock.c        | 119 ++++++++++++++++++
>  2 files changed, 128 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/preempt_lock.c
>  create mode 100644 tools/testing/selftests/bpf/progs/preempt_lock.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/preempt_lock.c b/tools/testing/selftests/bpf/prog_tests/preempt_lock.c
> new file mode 100644
> index 000000000000..02917c672441
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/preempt_lock.c
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include <preempt_lock.skel.h>
> +
> +void test_preempt_lock(void)
> +{
> +	RUN_TESTS(preempt_lock);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/preempt_lock.c b/tools/testing/selftests/bpf/progs/preempt_lock.c
> new file mode 100644
> index 000000000000..53320ea80fa4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/preempt_lock.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "bpf_misc.h"
> +
> +void bpf_preempt_disable(void) __ksym;
> +void bpf_preempt_enable(void) __ksym;
> +
> +SEC("?tc")
> +__failure __msg("1 bpf_preempt_enable is missing")
> +int preempt_lock_missing_1(struct __sk_buff *ctx)
> +{
> +	bpf_preempt_disable();
> +	return 0;
> +}
> +
> +SEC("?tc")
> +__failure __msg("2 bpf_preempt_enable(s) are missing")
> +int preempt_lock_missing_2(struct __sk_buff *ctx)
> +{
> +	bpf_preempt_disable();
> +	bpf_preempt_disable();
> +	return 0;
> +}
> +
> +SEC("?tc")
> +__failure __msg("3 bpf_preempt_enable(s) are missing")
> +int preempt_lock_missing_3(struct __sk_buff *ctx)
> +{
> +	bpf_preempt_disable();
> +	bpf_preempt_disable();
> +	bpf_preempt_disable();
> +	return 0;
> +}
> +
> +SEC("?tc")
> +__failure __msg("1 bpf_preempt_enable is missing")
> +int preempt_lock_missing_3_minus_2(struct __sk_buff *ctx)
> +{
> +	bpf_preempt_disable();
> +	bpf_preempt_disable();
> +	bpf_preempt_disable();
> +	bpf_preempt_enable();
> +	bpf_preempt_enable();
> +	return 0;
> +}
> +
> +static __noinline void preempt_disable(void)
> +{
> +	bpf_preempt_disable();
> +}
> +
> +static __noinline void preempt_enable(void)
> +{
> +	bpf_preempt_enable();
> +}
> +
> +SEC("?tc")
> +__failure __msg("1 bpf_preempt_enable is missing")
> +int preempt_lock_missing_1_subprog(struct __sk_buff *ctx)
> +{
> +	preempt_disable();
> +	return 0;
> +}
> +
> +SEC("?tc")
> +__failure __msg("2 bpf_preempt_enable(s) are missing")
> +int preempt_lock_missing_2_subprog(struct __sk_buff *ctx)
> +{
> +	preempt_disable();
> +	preempt_disable();
> +	return 0;
> +}
> +
> +SEC("?tc")
> +__failure __msg("1 bpf_preempt_enable is missing")
> +int preempt_lock_missing_2_minus_1_subprog(struct __sk_buff *ctx)
> +{
> +	preempt_disable();
> +	preempt_disable();
> +	preempt_enable();
> +	return 0;
> +}
> +
> +static __noinline void preempt_balance_subprog(void)
> +{
> +	preempt_disable();
> +	preempt_enable();
> +}
> +
> +SEC("?tc")
> +__success int preempt_balance(struct __sk_buff *ctx)
> +{
> +	bpf_preempt_disable();
> +	bpf_preempt_enable();
> +	return 0;
> +}
> +
> +SEC("?tc")
> +__success int preempt_balance_subprog_test(struct __sk_buff *ctx)
> +{
> +	preempt_balance_subprog();
> +	return 0;
> +}
> +
> +SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
> +__failure __msg("sleepable helper bpf_copy_from_user#")
> +int preempt_sleepable_helper(void *ctx)
> +{
> +	u32 data;
> +
> +	bpf_preempt_disable();
> +	bpf_copy_from_user(&data, sizeof(data), NULL);
> +	bpf_preempt_enable();
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> -- 
> 2.43.0
> 
> 

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add tests for preempt kfuncs

[Kumar Kartikeya Dwivedi]

On Tue, 23 Apr 2024 at 13:17, Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Apr 23, 2024 at 06:19:22AM +0000, Kumar Kartikeya Dwivedi wrote:
> > Add tests for nested cases, nested count preservation upon different
> > subprog calls that disable/enable preemption, and test sleepable helper
> > call in non-preemptible regions.
> >
> > 181/1   preempt_lock/preempt_lock_missing_1:OK
> > 181/2   preempt_lock/preempt_lock_missing_2:OK
> > 181/3   preempt_lock/preempt_lock_missing_3:OK
> > 181/4   preempt_lock/preempt_lock_missing_3_minus_2:OK
> > 181/5   preempt_lock/preempt_lock_missing_1_subprog:OK
> > 181/6   preempt_lock/preempt_lock_missing_2_subprog:OK
> > 181/7   preempt_lock/preempt_lock_missing_2_minus_1_subprog:OK
> > 181/8   preempt_lock/preempt_balance:OK
> > 181/9   preempt_lock/preempt_balance_subprog_test:OK
> > 181/10  preempt_lock/preempt_sleepable_helper:OK
>
> should we also check that the global function call is not allowed?
>

Good point, that is missing, I'll wait for more reviews and then
respin with a failure test for this.

> jirka
>

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add tests for preempt kfuncs

[Alexei Starovoitov]

On Tue, Apr 23, 2024 at 5:06 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Tue, 23 Apr 2024 at 13:17, Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2024 at 06:19:22AM +0000, Kumar Kartikeya Dwivedi wrote:
> > > Add tests for nested cases, nested count preservation upon different
> > > subprog calls that disable/enable preemption, and test sleepable helper
> > > call in non-preemptible regions.
> > >
> > > 181/1   preempt_lock/preempt_lock_missing_1:OK
> > > 181/2   preempt_lock/preempt_lock_missing_2:OK
> > > 181/3   preempt_lock/preempt_lock_missing_3:OK
> > > 181/4   preempt_lock/preempt_lock_missing_3_minus_2:OK
> > > 181/5   preempt_lock/preempt_lock_missing_1_subprog:OK
> > > 181/6   preempt_lock/preempt_lock_missing_2_subprog:OK
> > > 181/7   preempt_lock/preempt_lock_missing_2_minus_1_subprog:OK
> > > 181/8   preempt_lock/preempt_balance:OK
> > > 181/9   preempt_lock/preempt_balance_subprog_test:OK
> > > 181/10  preempt_lock/preempt_sleepable_helper:OK
> >
> > should we also check that the global function call is not allowed?
> >
>
> Good point, that is missing, I'll wait for more reviews and then
> respin with a failure test for this.

I couldn't find the check in patch 1 that does:
"Global functions are disallowed from being called".

And I agree that we need to allow global funcs in preempt disabled region.
Sounds like you're planning that in the follow up.

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add tests for preempt kfuncs

[Kumar Kartikeya Dwivedi]

On Tue, 23 Apr 2024 at 17:02, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Apr 23, 2024 at 5:06 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Tue, 23 Apr 2024 at 13:17, Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Tue, Apr 23, 2024 at 06:19:22AM +0000, Kumar Kartikeya Dwivedi wrote:
> > > > Add tests for nested cases, nested count preservation upon different
> > > > subprog calls that disable/enable preemption, and test sleepable helper
> > > > call in non-preemptible regions.
> > > >
> > > > 181/1   preempt_lock/preempt_lock_missing_1:OK
> > > > 181/2   preempt_lock/preempt_lock_missing_2:OK
> > > > 181/3   preempt_lock/preempt_lock_missing_3:OK
> > > > 181/4   preempt_lock/preempt_lock_missing_3_minus_2:OK
> > > > 181/5   preempt_lock/preempt_lock_missing_1_subprog:OK
> > > > 181/6   preempt_lock/preempt_lock_missing_2_subprog:OK
> > > > 181/7   preempt_lock/preempt_lock_missing_2_minus_1_subprog:OK
> > > > 181/8   preempt_lock/preempt_balance:OK
> > > > 181/9   preempt_lock/preempt_balance_subprog_test:OK
> > > > 181/10  preempt_lock/preempt_sleepable_helper:OK
> > >
> > > should we also check that the global function call is not allowed?
> > >
> >
> > Good point, that is missing, I'll wait for more reviews and then
> > respin with a failure test for this.
>
> I couldn't find the check in patch 1 that does:
> "Global functions are disallowed from being called".

See this part：

+ /* Only global subprogs cannot be called with preemption disabled. */
+ if (env->cur_state->active_preempt_lock) {
+ verbose(env, "global function calls are not allowed with preemption
disabled,\n"
+     "use static function instead\n");
+ return -EINVAL;
+ }

>
> And I agree that we need to allow global funcs in preempt disabled region.
> Sounds like you're planning that in the follow up.

Yeah, but it's not very simple. I noticed a few more problems with
global functions before that can be done.
They need to be marked !sleepable before do_check and only verified
with !sleepable. Otherwise if that is done later in do_check the
global function will be seen in the non-preemptible section but may
have been already verified.
We need some summarization pass for both preempt and rcu_read_lock kfuncs.

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add tests for preempt kfuncs

[Kumar Kartikeya Dwivedi]

On Tue, 23 Apr 2024 at 18:17, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Tue, 23 Apr 2024 at 17:02, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2024 at 5:06 AM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Tue, 23 Apr 2024 at 13:17, Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 23, 2024 at 06:19:22AM +0000, Kumar Kartikeya Dwivedi wrote:
> > > > > Add tests for nested cases, nested count preservation upon different
> > > > > subprog calls that disable/enable preemption, and test sleepable helper
> > > > > call in non-preemptible regions.
> > > > >
> > > > > 181/1   preempt_lock/preempt_lock_missing_1:OK
> > > > > 181/2   preempt_lock/preempt_lock_missing_2:OK
> > > > > 181/3   preempt_lock/preempt_lock_missing_3:OK
> > > > > 181/4   preempt_lock/preempt_lock_missing_3_minus_2:OK
> > > > > 181/5   preempt_lock/preempt_lock_missing_1_subprog:OK
> > > > > 181/6   preempt_lock/preempt_lock_missing_2_subprog:OK
> > > > > 181/7   preempt_lock/preempt_lock_missing_2_minus_1_subprog:OK
> > > > > 181/8   preempt_lock/preempt_balance:OK
> > > > > 181/9   preempt_lock/preempt_balance_subprog_test:OK
> > > > > 181/10  preempt_lock/preempt_sleepable_helper:OK
> > > >
> > > > should we also check that the global function call is not allowed?
> > > >
> > >
> > > Good point, that is missing, I'll wait for more reviews and then
> > > respin with a failure test for this.
> >
> > I couldn't find the check in patch 1 that does:
> > "Global functions are disallowed from being called".
>
> See this part：
>
> + /* Only global subprogs cannot be called with preemption disabled. */
> + if (env->cur_state->active_preempt_lock) {
> + verbose(env, "global function calls are not allowed with preemption
> disabled,\n"
> +     "use static function instead\n");
> + return -EINVAL;
> + }
>
> >
> > And I agree that we need to allow global funcs in preempt disabled region.
> > Sounds like you're planning that in the follow up.
>
> Yeah, but it's not very simple. I noticed a few more problems with
> global functions before that can be done.
> They need to be marked !sleepable before do_check and only verified
> with !sleepable. Otherwise if that is done later in do_check the
> global function will be seen in the non-preemptible section but may
> have been already verified.
> We need some summarization pass for both preempt and rcu_read_lock kfuncs.

E.g. I also have in my TODO list (from hitting this a while ago):

+ * Global functions may clear packet pointers, but data, data_end
remain unclobbered in the caller.
They may call a helper on ctx that invalidates pkt pointers but the
caller may not see that during verification and continue accessing
them.
This needs to be known for the global function before their callers
are verified.

Similarly, the context they are called from is atomic or not
throughput the program needs to be known before their verification is
done.
Otherwise they are verified as sleepable for sleepable programs, while
they maybe called from non-preemptible region.

I will fix the latter first, but a summarization pass for them is
needed and useful to fix the former as well.