Skip to content

Commit

Permalink
Merge branch 'fix-resource-leak-checks-for-tail-calls'
Browse files Browse the repository at this point in the history
Kumar Kartikeya Dwivedi says:

====================
Fix resource leak checks for tail calls

This set contains a fix for detecting unreleased RCU read locks or
unfinished preempt_disable sections when performing a tail call. Spin
locks are prevented by accident since they don't allow any function
calls, including tail calls (modelled as call instruction to a helper),
so we ensure they are checked as well, in preparation for relaxing
function call restricton for critical sections in the future.

Then, in the second patch, all the checks for reference leaks and locks
are unified into a single function that can be called from different
places. This unification patch is kept separate and placed after the fix
to allow independent backport of the fix to older kernels without a
depdendency on the clean up.

Naturally, this creates a divergence in the disparate error messages,
therefore selftests that rely on the exact error strings need to be
updated to match the new verifier log message.

A selftest is included to ensure no regressions occur wrt this behavior.
====================

Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Alexei Starovoitov <[email protected]>
  • Loading branch information
Alexei Starovoitov committed Nov 4, 2024
2 parents 77017b9 + 711df09 commit f2daa5a
Show file tree
Hide file tree
Showing 7 changed files with 118 additions and 53 deletions.
75 changes: 34 additions & 41 deletions kernel/bpf/verifier.c
Original file line number Diff line number Diff line change
Expand Up @@ -10352,6 +10352,34 @@ static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exi
return refs_lingering ? -EINVAL : 0;
}

static int check_resource_leak(struct bpf_verifier_env *env, bool exception_exit, bool check_lock, const char *prefix)
{
int err;

if (check_lock && env->cur_state->active_lock.ptr) {
verbose(env, "%s cannot be used inside bpf_spin_lock-ed region\n", prefix);
return -EINVAL;
}

err = check_reference_leak(env, exception_exit);
if (err) {
verbose(env, "%s would lead to reference leak\n", prefix);
return err;
}

if (check_lock && env->cur_state->active_rcu_lock) {
verbose(env, "%s cannot be used inside bpf_rcu_read_lock-ed region\n", prefix);
return -EINVAL;
}

if (check_lock && env->cur_state->active_preempt_lock) {
verbose(env, "%s cannot be used inside bpf_preempt_disable-ed region\n", prefix);
return -EINVAL;
}

return 0;
}

static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
struct bpf_reg_state *regs)
{
Expand Down Expand Up @@ -10620,11 +10648,9 @@ 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);
if (err) {
verbose(env, "tail_call would lead to reference leak\n");
err = check_resource_leak(env, false, true, "tail_call");
if (err)
return err;
}
break;
case BPF_FUNC_get_local_storage:
/* check that flags argument in get_local_storage(map, flags) is 0,
Expand Down Expand Up @@ -15786,26 +15812,9 @@ 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);
if (err) {
verbose(env, "BPF_LD_[ABS|IND] cannot be mixed with socket references\n");
err = check_resource_leak(env, false, true, "BPF_LD_[ABS|IND]");
if (err)
return err;
}

if (env->cur_state->active_lock.ptr) {
verbose(env, "BPF_LD_[ABS|IND] cannot be used inside bpf_spin_lock-ed region\n");
return -EINVAL;
}

if (env->cur_state->active_rcu_lock) {
verbose(env, "BPF_LD_[ABS|IND] cannot be used inside bpf_rcu_read_lock-ed region\n");
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,
Expand Down Expand Up @@ -18591,30 +18600,14 @@ static int do_check(struct bpf_verifier_env *env)
return -EINVAL;
}
process_bpf_exit_full:
if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {
verbose(env, "bpf_spin_unlock is missing\n");
return -EINVAL;
}

if (env->cur_state->active_rcu_lock && !env->cur_state->curframe) {
verbose(env, "bpf_rcu_read_unlock is missing\n");
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
* function, for which reference_state must
* match caller reference state when it exits.
*/
err = check_reference_leak(env, exception_exit);
err = check_resource_leak(env, exception_exit, !env->cur_state->curframe,
"BPF_EXIT instruction");
if (err)
return err;

Expand Down
8 changes: 8 additions & 0 deletions tools/testing/selftests/bpf/prog_tests/tailcalls.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "tailcall_bpf2bpf_hierarchy3.skel.h"
#include "tailcall_freplace.skel.h"
#include "tc_bpf2bpf.skel.h"
#include "tailcall_fail.skel.h"

/* test_tailcall_1 checks basic functionality by patching multiple locations
* in a single program for a single tail call slot with nop->jmp, jmp->nop
Expand Down Expand Up @@ -1646,6 +1647,11 @@ static void test_tailcall_bpf2bpf_freplace(void)
tc_bpf2bpf__destroy(tc_skel);
}

static void test_tailcall_failure()
{
RUN_TESTS(tailcall_fail);
}

void test_tailcalls(void)
{
if (test__start_subtest("tailcall_1"))
Expand Down Expand Up @@ -1698,4 +1704,6 @@ void test_tailcalls(void)
test_tailcall_freplace();
if (test__start_subtest("tailcall_bpf2bpf_freplace"))
test_tailcall_bpf2bpf_freplace();
if (test__start_subtest("tailcall_failure"))
test_tailcall_failure();
}
4 changes: 2 additions & 2 deletions tools/testing/selftests/bpf/progs/exceptions_fail.c
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ int reject_subprog_with_lock(void *ctx)
}

SEC("?tc")
__failure __msg("bpf_rcu_read_unlock is missing")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_rcu_read_lock-ed region")
int reject_with_rcu_read_lock(void *ctx)
{
bpf_rcu_read_lock();
Expand All @@ -147,7 +147,7 @@ __noinline static int throwing_subprog(struct __sk_buff *ctx)
}

SEC("?tc")
__failure __msg("bpf_rcu_read_unlock is missing")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_rcu_read_lock-ed region")
int reject_subprog_with_rcu_read_lock(void *ctx)
{
bpf_rcu_read_lock();
Expand Down
14 changes: 7 additions & 7 deletions tools/testing/selftests/bpf/progs/preempt_lock.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
#include "bpf_experimental.h"

SEC("?tc")
__failure __msg("1 bpf_preempt_enable is missing")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_preempt_disable-ed region")
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")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_preempt_disable-ed region")
int preempt_lock_missing_2(struct __sk_buff *ctx)
{
bpf_preempt_disable();
Expand All @@ -23,7 +23,7 @@ int preempt_lock_missing_2(struct __sk_buff *ctx)
}

SEC("?tc")
__failure __msg("3 bpf_preempt_enable(s) are missing")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_preempt_disable-ed region")
int preempt_lock_missing_3(struct __sk_buff *ctx)
{
bpf_preempt_disable();
Expand All @@ -33,7 +33,7 @@ int preempt_lock_missing_3(struct __sk_buff *ctx)
}

SEC("?tc")
__failure __msg("1 bpf_preempt_enable is missing")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_preempt_disable-ed region")
int preempt_lock_missing_3_minus_2(struct __sk_buff *ctx)
{
bpf_preempt_disable();
Expand All @@ -55,15 +55,15 @@ static __noinline void preempt_enable(void)
}

SEC("?tc")
__failure __msg("1 bpf_preempt_enable is missing")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_preempt_disable-ed region")
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")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_preempt_disable-ed region")
int preempt_lock_missing_2_subprog(struct __sk_buff *ctx)
{
preempt_disable();
Expand All @@ -72,7 +72,7 @@ int preempt_lock_missing_2_subprog(struct __sk_buff *ctx)
}

SEC("?tc")
__failure __msg("1 bpf_preempt_enable is missing")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_preempt_disable-ed region")
int preempt_lock_missing_2_minus_1_subprog(struct __sk_buff *ctx)
{
preempt_disable();
Expand Down
64 changes: 64 additions & 0 deletions tools/testing/selftests/bpf/progs/tailcall_fail.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
#include <vmlinux.h>
#include <bpf/bpf_tracing.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_core_read.h>

#include "bpf_misc.h"
#include "bpf_experimental.h"

extern void bpf_rcu_read_lock(void) __ksym;
extern void bpf_rcu_read_unlock(void) __ksym;

#define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))

private(A) struct bpf_spin_lock lock;

struct {
__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
__uint(max_entries, 3);
__uint(key_size, sizeof(__u32));
__uint(value_size, sizeof(__u32));
} jmp_table SEC(".maps");

SEC("?tc")
__failure __msg("function calls are not allowed while holding a lock")
int reject_tail_call_spin_lock(struct __sk_buff *ctx)
{
bpf_spin_lock(&lock);
bpf_tail_call_static(ctx, &jmp_table, 0);
return 0;
}

SEC("?tc")
__failure __msg("tail_call cannot be used inside bpf_rcu_read_lock-ed region")
int reject_tail_call_rcu_lock(struct __sk_buff *ctx)
{
bpf_rcu_read_lock();
bpf_tail_call_static(ctx, &jmp_table, 0);
bpf_rcu_read_unlock();
return 0;
}

SEC("?tc")
__failure __msg("tail_call cannot be used inside bpf_preempt_disable-ed region")
int reject_tail_call_preempt_lock(struct __sk_buff *ctx)
{
bpf_guard_preempt();
bpf_tail_call_static(ctx, &jmp_table, 0);
return 0;
}

SEC("?tc")
__failure __msg("tail_call would lead to reference leak")
int reject_tail_call_ref(struct __sk_buff *ctx)
{
struct foo { int i; } *p;

p = bpf_obj_new(typeof(*p));
bpf_tail_call_static(ctx, &jmp_table, 0);
return 0;
}

char _license[] SEC("license") = "GPL";
4 changes: 2 additions & 2 deletions tools/testing/selftests/bpf/progs/verifier_ref_tracking.c
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ l0_%=: r0 = *(u8*)skb[0]; \

SEC("tc")
__description("reference tracking: forbid LD_ABS while holding reference")
__failure __msg("BPF_LD_[ABS|IND] cannot be mixed with socket references")
__failure __msg("BPF_LD_[ABS|IND] would lead to reference leak")
__naked void ld_abs_while_holding_reference(void)
{
asm volatile (" \
Expand Down Expand Up @@ -836,7 +836,7 @@ l0_%=: r7 = 1; \

SEC("tc")
__description("reference tracking: forbid LD_IND while holding reference")
__failure __msg("BPF_LD_[ABS|IND] cannot be mixed with socket references")
__failure __msg("BPF_LD_[ABS|IND] would lead to reference leak")
__naked void ld_ind_while_holding_reference(void)
{
asm volatile (" \
Expand Down
2 changes: 1 addition & 1 deletion tools/testing/selftests/bpf/progs/verifier_spin_lock.c
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ l0_%=: r6 = r0; \

SEC("cgroup/skb")
__description("spin_lock: test6 missing unlock")
__failure __msg("unlock is missing")
__failure __msg("BPF_EXIT instruction cannot be used inside bpf_spin_lock-ed region")
__failure_unpriv __msg_unpriv("")
__naked void spin_lock_test6_missing_unlock(void)
{
Expand Down

0 comments on commit f2daa5a

Please sign in to comment.