On Tue, Aug 01, 2023 at 01:36:29PM -0700, 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?
Yes. __bpf_spin_lock() needs to disable it before arch_spin_lock.
Since some sleepable progs are reentrable we need to make sure the bpf prog
isn't preempted while spin_lock is held. Otherwise dead lock is possible.
> ]
>
> 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;
> + }
I don't see the point requiring bpf_spin_lock only under RCU CS.
It seems below !sleepable check can be dropped without adding above hunk.
> 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)) {
> --
> 2.34.1
>
