[RFC,v1,11/14] bpf: Release references in verifier state when throwing exceptions

Commit Message

Kumar Kartikeya Dwivedi Feb. 1, 2024, 4:21 a.m. UTC

Reflect in the verifier state that references would be released whenever
we throw a BPF exception. Now that we support generating frame
descriptors, and performing the runtime cleanup, whenever processing an
entry corresponding to an acquired reference, make sure we release its
reference state. Note that we only release this state for the current
frame, as the acquired refs are only checked against that when
processing an exceptional exit.

This would ensure that for acquired resources apart from locks and RCU
read sections, BPF programs never fail in case of lingering resources
during verification.

While at it, we can tweak check_reference_leak to drop the
exception_exit parameter, and fix selftests that will fail due to the
changed behaviour.

Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
---
 kernel/bpf/verifier.c                          | 18 ++++++++++++------
 .../selftests/bpf/progs/exceptions_fail.c      |  2 +-
 2 files changed, 13 insertions(+), 7 deletions(-)

Comments

Eduard Zingerman Feb. 16, 2024, 12:21 p.m. UTC | #1

On Thu, 2024-02-01 at 04:21 +0000, Kumar Kartikeya Dwivedi wrote:
> Reflect in the verifier state that references would be released whenever
> we throw a BPF exception. Now that we support generating frame
> descriptors, and performing the runtime cleanup, whenever processing an
> entry corresponding to an acquired reference, make sure we release its
> reference state. Note that we only release this state for the current
> frame, as the acquired refs are only checked against that when
> processing an exceptional exit.
> 
> This would ensure that for acquired resources apart from locks and RCU
> read sections, BPF programs never fail in case of lingering resources
> during verification.
> 
> While at it, we can tweak check_reference_leak to drop the
> exception_exit parameter, and fix selftests that will fail due to the
> changed behaviour.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3e3b8a20451c..8edefcd999ea 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10221,6 +10221,8 @@  static int gen_exception_frame_desc_reg_entry(struct bpf_verifier_env *env, stru
 		verbose(env, "frame_desc: frame%d: failed to simulate cleanup for frame desc entry\n", frameno);
 		return -EFAULT;
 	}
+	if (reg->ref_obj_id && frameno == cur_func(env)->frameno)
+		WARN_ON_ONCE(release_reference(env, reg->ref_obj_id));
 	return push_exception_frame_desc(env, frameno, &fd);
 }
 
@@ -10251,6 +10253,8 @@  static int gen_exception_frame_desc_dynptr_entry(struct bpf_verifier_env *env, s
 		verbose(env, "frame_desc: frame%d: failed to simulate cleanup for frame desc entry\n", frameno);
 		return -EFAULT;
 	}
+	if (frameno == cur_func(env)->frameno)
+		WARN_ON_ONCE(release_reference(env, reg->ref_obj_id));
 	return push_exception_frame_desc(env, frameno, &fd);
 }
 
@@ -10283,6 +10287,8 @@  static int gen_exception_frame_desc_iter_entry(struct bpf_verifier_env *env, str
 		verbose(env, "frame_desc: frame%d: failed to simulate cleanup for frame desc entry\n", frameno);
 		return -EFAULT;
 	}
+	if (frameno == cur_func(env)->frameno)
+		WARN_ON_ONCE(release_reference(env, reg->ref_obj_id));
 	return push_exception_frame_desc(env, frameno, &fd);
 }
 
@@ -10393,17 +10399,17 @@  static int gen_exception_frame_descs(struct bpf_verifier_env *env)
 	return 0;
 }
 
-static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exit)
+static int check_reference_leak(struct bpf_verifier_env *env)
 {
 	struct bpf_func_state *state = cur_func(env);
 	bool refs_lingering = false;
 	int i;
 
-	if (!exception_exit && state->frameno && !state->in_callback_fn)
+	if (state->frameno && !state->in_callback_fn)
 		return 0;
 
 	for (i = 0; i < state->acquired_refs; i++) {
-		if (!exception_exit && state->in_callback_fn && state->refs[i].callback_ref != state->frameno)
+		if (state->in_callback_fn && state->refs[i].callback_ref != state->frameno)
 			continue;
 		verbose(env, "Unreleased reference id=%d alloc_insn=%d\n",
 			state->refs[i].id, state->refs[i].insn_idx);
@@ -10658,7 +10664,7 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 
 	switch (func_id) {
 	case BPF_FUNC_tail_call:
-		err = check_reference_leak(env, false);
+		err = check_reference_leak(env);
 		if (err) {
 			verbose(env, "tail_call would lead to reference leak\n");
 			return err;
@@ -15593,7 +15599,7 @@  static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
 	 * gen_ld_abs() may terminate the program at runtime, leading to
 	 * reference leak.
 	 */
-	err = check_reference_leak(env, false);
+	err = check_reference_leak(env);
 	if (err) {
 		verbose(env, "BPF_LD_[ABS|IND] cannot be mixed with socket references\n");
 		return err;
@@ -18149,7 +18155,7 @@  static int do_check(struct bpf_verifier_env *env)
 				 * function, for which reference_state must
 				 * match caller reference state when it exits.
 				 */
-				err = check_reference_leak(env, exception_exit);
+				err = check_reference_leak(env);
 				if (err)
 					return err;
 
diff --git a/tools/testing/selftests/bpf/progs/exceptions_fail.c b/tools/testing/selftests/bpf/progs/exceptions_fail.c
index 5a517065b4e6..dfd164a7a261 100644
--- a/tools/testing/selftests/bpf/progs/exceptions_fail.c
+++ b/tools/testing/selftests/bpf/progs/exceptions_fail.c
@@ -354,7 +354,7 @@  int reject_exception_throw_cb_diff(struct __sk_buff *ctx)
 }
 
 SEC("?tc")
-__failure __msg("exploring program path where exception is thrown")
+__success __log_level(2) __msg("exploring program path where exception is thrown")
 int reject_exception_throw_ref_call_throwing_global(struct __sk_buff *ctx)
 {
 	struct { long a; } *p = bpf_obj_new(typeof(*p));