Re: [PATCH bpf-next v1 1/2] bpf: Allow calling static subprogs while holding a bpf_spin_lock

[Yonghong Song <yonghong.song@linux.dev>]

On 2/4/24 2:10 PM, Kumar Kartikeya Dwivedi wrote:
> On Sun, 4 Feb 2024 at 22:33, David Vernet <void@manifault.com> wrote:
>> On Sun, Feb 04, 2024 at 12:02:05PM +0000, Kumar Kartikeya Dwivedi wrote:
>>> Currently, calling any helpers, kfuncs, or subprogs except the graph
>>> data structure (lists, rbtrees) API kfuncs while holding a bpf_spin_lock
>>> is not allowed. One of the original motivations of this decision was to
>>> force the BPF programmer's hand into keeping the bpf_spin_lock critical
>>> section small, and to ensure the execution time of the program does not
>>> increase due to lock waiting times. In addition to this, some of the
>>> helpers and kfuncs may be unsafe to call while holding a bpf_spin_lock.
>>>
>>> However, when it comes to subprog calls, atleast for static subprogs,
>>> the verifier is able to explore their instructions during verification.
>>> Therefore, it is similar in effect to having the same code inlined into
>>> the critical section. Hence, not allowing static subprog calls in the
>>> bpf_spin_lock critical section is mostly an annoyance that needs to be
>>> worked around, without providing any tangible benefit.
>>>
>>> Unlike static subprog calls, global subprog calls are not safe to permit
>>> within the critical section, as the verifier does not explore them
>>> during verification, therefore whether the same lock will be taken
>>> again, or unlocked, cannot be ascertained.
>>>
>>> Therefore, allow calling static subprogs within a bpf_spin_lock critical
>>> section, and only reject it in case the subprog linkage is global.
>>>
>>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> Looks good, thanks for this improvement. I had the same suggestion as
>> Yonghong in [0], and also left a question below.
>>
>> [0]: https://lore.kernel.org/all/2e008ab1-44b8-4d1b-a86d-1f347d7630e6@linux.dev/
>>
>> Acked-by: David Vernet <void@manifault.com>
>>
>>> ---
>>>   kernel/bpf/verifier.c                                  | 10 +++++++---
>>>   tools/testing/selftests/bpf/progs/verifier_spin_lock.c |  2 +-
>>>   2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 64fa188d00ad..f858c959753b 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -9493,6 +9493,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>        if (subprog_is_global(env, subprog)) {
>>>                const char *sub_name = subprog_name(env, subprog);
>>>
>>> +             /* Only global subprogs cannot be called with a lock held. */
>>> +             if (env->cur_state->active_lock.ptr) {
>>> +                     verbose(env, "function calls are not allowed while holding a lock\n");
>>> +                     return -EINVAL;
>>> +             }
>>> +
>>>                if (err) {
>>>                        verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
>>>                                subprog, sub_name);
>>> @@ -17644,7 +17650,6 @@ static int do_check(struct bpf_verifier_env *env)
>>>
>>>                                if (env->cur_state->active_lock.ptr) {
>>>                                        if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
>>> -                                         (insn->src_reg == BPF_PSEUDO_CALL) ||
>>>                                            (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
>>>                                             (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
>>>                                                verbose(env, "function calls are not allowed while holding a lock\n");
>>> @@ -17692,8 +17697,7 @@ static int do_check(struct bpf_verifier_env *env)
>>>                                        return -EINVAL;
>>>                                }
>>>   process_bpf_exit_full:
>>> -                             if (env->cur_state->active_lock.ptr &&
>>> -                                 !in_rbtree_lock_required_cb(env)) {
>>> +                             if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {
>> Can we do the same thing here for the RCU check below? It seems like the
>> exact same issue, as we're already allowed to call subprogs from within
>> an RCU read region, but the verifier will get confused and think we
>> haven't unlocked by the time we return to the caller.
>>
>> Assuming that's the case, we can take care of it in a separate patch
>> set.
> Makes sense, I'll send a separate patch for the RCU fix.
> Thanks for the review.

The following is what I recommended as well in another thread:

https://lore.kernel.org/bpf/20240131145454.86990-1-laoar.shao@gmail.com/T/#mff17cd64eeb1e17bd0e3e046fb52efeef9c86c25

>