Re: [PATCH bpf-next v3 03/11] bpf: refactor check_func_call() to allow callback function

[Yonghong Song <yhs@fb.com>]

On 2/25/21 2:31 PM, Andrii Nakryiko wrote:
> On Thu, Feb 25, 2021 at 2:05 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Thu, Feb 25, 2021 at 1:35 AM Yonghong Song <yhs@fb.com> wrote:
>>>
>>> Later proposed bpf_for_each_map_elem() helper has callback
>>> function as one of its arguments. This patch refactored
>>> check_func_call() to permit callback function which sets
>>> callee state. Different callback functions may have
>>> different callee states.
>>>
>>> There is no functionality change for this patch except
>>> it added a case to handle where subprog number is known
>>> and there is no need to do find_subprog(). This case
>>> is used later by implementing bpf_for_each_map() helper.
>>>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>> ---
>>>   kernel/bpf/verifier.c | 54 ++++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 41 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index a657860ecba5..092d2c734dd8 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -5250,13 +5250,19 @@ static void clear_caller_saved_regs(struct bpf_verifier_env *env,
>>>          }
>>>   }
>>>
>>> -static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>> -                          int *insn_idx)
>>> +typedef int (*set_callee_state_fn)(struct bpf_verifier_env *env,
>>> +                                  struct bpf_func_state *caller,
>>> +                                  struct bpf_func_state *callee,
>>> +                                  int insn_idx);
>>> +
>>> +static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>> +                            int *insn_idx, int subprog,
> 
> ok, patch #4 confused me because of this `int *insn_idx`. You don't
> seem to be ever updating it, so why pass it by pointer?... What did I
> miss?

We do have something later:

         /* and go analyze first insn of the callee */
         *insn_idx = target_insn;

which is the old code and probably did not show up in the diff.
The above statement changed insn_idx such that when done with
examining the func call, the control will jump (*insn_idx)++ instruction.

> 
>>> +                            set_callee_state_fn set_callee_st)
>>
>> nit: s/set_callee_st/set_callee_state_cb|set_calle_state_fn/
>>
>> _st is quite an unusual suffix
>>
>>>   {
>>>          struct bpf_verifier_state *state = env->cur_state;
>>>          struct bpf_func_info_aux *func_info_aux;
>>>          struct bpf_func_state *caller, *callee;
>>> -       int i, err, subprog, target_insn;
>>> +       int err, target_insn;
>>>          bool is_global = false;
>>>
>>>          if (state->curframe + 1 >= MAX_CALL_FRAMES) {
>>> @@ -5265,12 +5271,16 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>                  return -E2BIG;
>>>          }
>>>
>>> -       target_insn = *insn_idx + insn->imm;
>>> -       subprog = find_subprog(env, target_insn + 1);
>>>          if (subprog < 0) {
>>> -               verbose(env, "verifier bug. No program starts at insn %d\n",
>>> -                       target_insn + 1);
>>> -               return -EFAULT;
>>> +               target_insn = *insn_idx + insn->imm;
>>> +               subprog = find_subprog(env, target_insn + 1);
>>> +               if (subprog < 0) {
>>> +                       verbose(env, "verifier bug. No program starts at insn %d\n",
>>> +                               target_insn + 1);
>>> +                       return -EFAULT;
>>> +               }
>>> +       } else {
>>> +               target_insn = env->subprog_info[subprog].start - 1;
>>>          }
>>>
>>>          caller = state->frame[state->curframe];
>>> @@ -5327,11 +5337,9 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>          if (err)
>>>                  return err;
>>>
>>> -       /* copy r1 - r5 args that callee can access.  The copy includes parent
>>> -        * pointers, which connects us up to the liveness chain
>>> -        */
>>> -       for (i = BPF_REG_1; i <= BPF_REG_5; i++)
>>> -               callee->regs[i] = caller->regs[i];
>>> +       err = set_callee_st(env, caller, callee, *insn_idx);
>>> +       if (err)
>>> +               return err;
>>>
>>>          clear_caller_saved_regs(env, caller->regs);
>>>
>>> @@ -5350,6 +5358,26 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>          return 0;
>>>   }
>>>
>>> +static int set_callee_state(struct bpf_verifier_env *env,
>>> +                           struct bpf_func_state *caller,
>>> +                           struct bpf_func_state *callee, int insn_idx)
>>> +{
>>> +       int i;
>>> +
>>> +       /* copy r1 - r5 args that callee can access.  The copy includes parent
>>> +        * pointers, which connects us up to the liveness chain
>>> +        */
>>> +       for (i = BPF_REG_1; i <= BPF_REG_5; i++)
>>> +               callee->regs[i] = caller->regs[i];
>>> +       return 0;
>>> +}
>>> +
>>> +static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>> +                          int *insn_idx)
>>> +{
>>> +       return __check_func_call(env, insn, insn_idx, -1, set_callee_state);
>>
>> I think it would be much cleaner to not have this -1 special case in
>> __check_func_call and instead search for the right subprog right here
>> in check_func_call(). Related question, is meta.subprogno (in patch
>> #4) expected to sometimes be < 0? If not, then I think
>> __check_func_call() definitely shouldn't support -1 case at all.
>>
>>
>>> +}
>>> +
>>>   static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
>>>   {
>>>          struct bpf_verifier_state *state = env->cur_state;
>>> --
>>> 2.24.1
>>>