On 8/1/23 1:36 PM, Dave Marchevsky wrote:
Commit 9e7a4d9831e8 ("bpf: Allow LSM programs to use bpf spin locks")
disabled bpf_spin_lock usage in sleepable progs, stating:
Sleepable LSM programs can be preempted which means that allowng spin
locks will need more work (disabling preemption and the verifier
ensuring that no sleepable helpers are called when a spin lock is
held).
It seems that some of this 'ensuring that no sleepable helpers are
called' was done for RCU critical section in commit 9bb00b2895cb ("bpf:
Add kfunc bpf_rcu_read_lock/unlock()"), specifically the check which
fails with verbose "sleepable helper %s#%d in rcu_read_lock region"
message in check_helper_call and similar in check_kfunc_call. These
checks prevent sleepable helper and kfunc calls in RCU critical
sections. Accordingly, it should be safe to allow bpf_spin_{lock,unlock}
in RCU CS. This patch does so, replacing the broad "sleepable progs cannot use
bpf_spin_lock yet" check with a more targeted !in_rcu_cs.
[
RFC: Does preemption still need to be disabled here?
Good question. My hunch is that we do not need it since
- spin lock is inside rcu cs, which is similar to a
spin lock in a non-sleepable program.
I could be wrong as preemption with a sleepable program
may allow explicit blocking.
]
Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
---
kernel/bpf/verifier.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4bda365000d3..d1b8e8964aec 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8270,6 +8270,10 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
verbose(env, "can't spin_{lock,unlock} in rbtree cb\n");
return -EACCES;
}
+ if (!in_rcu_cs(env)) {
+ verbose(env, "sleepable progs may only spin_{lock,unlock} in RCU CS\n");
+ return -EACCES;
+ }
if (meta->func_id == BPF_FUNC_spin_lock) {
err = process_spin_lock(env, regno, true);
if (err)
@@ -16972,11 +16976,6 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
verbose(env, "tracing progs cannot use bpf_spin_lock yet\n");
return -EINVAL;
}
-
- if (prog->aux->sleepable) {
- verbose(env, "sleepable progs cannot use bpf_spin_lock yet\n");
- return -EINVAL;
- }
}
if (btf_record_has_field(map->record, BPF_TIMER)) {
