| Message ID | 20230801203630.3581291-7-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | BPF Refcount followups 3: bpf_mem_free_rcu refcounted nodes | expand |
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@fb.com> > --- > 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)) {
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@fb.com> > --- > 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 >
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)) {
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? ] Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- kernel/bpf/verifier.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)