Skip to content

Commit 859a7db

Browse files
author
Alexei Starovoitov
committed
Merge branch 'bpf-allow-bpf_for-bpf_repeat-while-holding-spin'
Emil Tsalapatis says: ==================== In BPF programs, kfunc calls while holding a lock are not allowed because kfuncs may sleep by default. The exception to this rule are the functions in special_kfunc_list, which are guaranteed to not sleep. The bpf_iter_num_* functions used by the bpf_for and bpf_repeat macros make no function calls themselves, and as such are guaranteed to not sleep. Add them to special_kfunc_list to allow them within BPF spinlock critical sections. Signed-off-by: Emil Tsalapatis (Meta) <[email protected]> ==================== Link: https://patch.msgid.link/[email protected] Signed-off-by: Alexei Starovoitov <[email protected]>
2 parents 2532608 + 87091dd commit 859a7db

File tree

2 files changed

+45
-1
lines changed

2 files changed

+45
-1
lines changed

kernel/bpf/verifier.c

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11690,6 +11690,9 @@ enum special_kfunc_type {
1169011690
KF_bpf_get_kmem_cache,
1169111691
KF_bpf_local_irq_save,
1169211692
KF_bpf_local_irq_restore,
11693+
KF_bpf_iter_num_new,
11694+
KF_bpf_iter_num_next,
11695+
KF_bpf_iter_num_destroy,
1169311696
};
1169411697

1169511698
BTF_SET_START(special_kfunc_set)
@@ -11765,6 +11768,9 @@ BTF_ID_UNUSED
1176511768
BTF_ID(func, bpf_get_kmem_cache)
1176611769
BTF_ID(func, bpf_local_irq_save)
1176711770
BTF_ID(func, bpf_local_irq_restore)
11771+
BTF_ID(func, bpf_iter_num_new)
11772+
BTF_ID(func, bpf_iter_num_next)
11773+
BTF_ID(func, bpf_iter_num_destroy)
1176811774

1176911775
static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
1177011776
{
@@ -12151,12 +12157,24 @@ static bool is_bpf_rbtree_api_kfunc(u32 btf_id)
1215112157
btf_id == special_kfunc_list[KF_bpf_rbtree_first];
1215212158
}
1215312159

12160+
static bool is_bpf_iter_num_api_kfunc(u32 btf_id)
12161+
{
12162+
return btf_id == special_kfunc_list[KF_bpf_iter_num_new] ||
12163+
btf_id == special_kfunc_list[KF_bpf_iter_num_next] ||
12164+
btf_id == special_kfunc_list[KF_bpf_iter_num_destroy];
12165+
}
12166+
1215412167
static bool is_bpf_graph_api_kfunc(u32 btf_id)
1215512168
{
1215612169
return is_bpf_list_api_kfunc(btf_id) || is_bpf_rbtree_api_kfunc(btf_id) ||
1215712170
btf_id == special_kfunc_list[KF_bpf_refcount_acquire_impl];
1215812171
}
1215912172

12173+
static bool kfunc_spin_allowed(u32 btf_id)
12174+
{
12175+
return is_bpf_graph_api_kfunc(btf_id) || is_bpf_iter_num_api_kfunc(btf_id);
12176+
}
12177+
1216012178
static bool is_sync_callback_calling_kfunc(u32 btf_id)
1216112179
{
1216212180
return btf_id == special_kfunc_list[KF_bpf_rbtree_add_impl];
@@ -19048,7 +19066,7 @@ static int do_check(struct bpf_verifier_env *env)
1904819066
if (env->cur_state->active_locks) {
1904919067
if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
1905019068
(insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
19051-
(insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
19069+
(insn->off != 0 || !kfunc_spin_allowed(insn->imm)))) {
1905219070
verbose(env, "function calls are not allowed while holding a lock\n");
1905319071
return -EINVAL;
1905419072
}

tools/testing/selftests/bpf/progs/verifier_spin_lock.c

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,4 +530,30 @@ l1_%=: exit; \
530530
: __clobber_all);
531531
}
532532

533+
SEC("tc")
534+
__description("spin_lock: loop within a locked region")
535+
__success __failure_unpriv __msg_unpriv("")
536+
__retval(0)
537+
int bpf_loop_inside_locked_region(void)
538+
{
539+
const int zero = 0;
540+
struct val *val;
541+
int i, j = 0;
542+
543+
val = bpf_map_lookup_elem(&map_spin_lock, &zero);
544+
if (!val)
545+
return -1;
546+
547+
bpf_spin_lock(&val->l);
548+
bpf_for(i, 0, 10) {
549+
j++;
550+
/* Silence "unused variable" warnings. */
551+
if (j == 10)
552+
break;
553+
}
554+
bpf_spin_unlock(&val->l);
555+
556+
return 0;
557+
}
558+
533559
char _license[] SEC("license") = "GPL";

0 commit comments

Comments
 (0)