On Thu, Jan 09, 2020 at 06:09:08PM +0000, Song Liu wrote:
>
>
> > On Jan 8, 2020, at 10:37 PM, Alexei Starovoitov <ast@xxxxxxxxxx> wrote:
>
> [...]
>
> >
> > Note that the stack limit of 512 still applies to the call chain regardless whether
> > functions were static or global. The nested level of 8 also still applies. The
> > same recursion prevention checks are in place as well.
> >
> > The type information and static/global kind is preserved after the verification
> > hence in the above example global function f2() and f3() can be replaced later
> > by equivalent functions with the same types that are loaded and verified later
> > without affecting safety of this main() program. Such replacement (re-linking)
> > of global functions is a subject of future patches.
> >
> > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> Acked-by: Song Liu <songliubraving@xxxxxx>
>
> With one nit below.
>
> [...]
>
> > +
> > +static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > +{
> > + struct bpf_verifier_state *state;
> > + struct bpf_reg_state *regs;
> > + int ret, i;
> > +
> > + env->prev_linfo = NULL;
> > + env->pass_cnt++;
> > +
> > + state = kzalloc(sizeof(struct bpf_verifier_state), GFP_KERNEL);
> > + if (!state)
> > + return -ENOMEM;
> > + state->curframe = 0;
> > + state->speculative = false;
> > + state->branches = 1;
> > + state->frame[0] = kzalloc(sizeof(struct bpf_func_state), GFP_KERNEL);
> > + if (!state->frame[0]) {
> > + kfree(state);
> > + return -ENOMEM;
> > + }
> > + env->cur_state = state;
> > + init_func_state(env, state->frame[0],
> > + BPF_MAIN_FUNC /* callsite */,
> > + 0 /* frameno */,
> > + subprog);
> > +
> > + regs = state->frame[state->curframe]->regs;
> > + if (subprog) {
> > + ret = btf_prepare_func_args(env, subprog, regs);
> > + if (ret)
> > + goto out;
> > + for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> > + if (regs[i].type == PTR_TO_CTX)
> > + mark_reg_known_zero(env, regs, i);
> > + else if (regs[i].type == SCALAR_VALUE)
> > + mark_reg_unknown(env, regs, i);
> > + }
> > + } else {
> > + /* 1st arg to a function */
> > + regs[BPF_REG_1].type = PTR_TO_CTX;
> > + mark_reg_known_zero(env, regs, BPF_REG_1);
> > + ret = btf_check_func_arg_match(env, subprog, regs);
> > + if (ret == -EFAULT)
> > + /* unlikely verifier bug. abort.
> > + * ret == 0 and ret < 0 are sadly acceptable for
> > + * main() function due to backward compatibility.
> > + * Like socket filter program may be written as:
> > + * int bpf_prog(struct pt_regs *ctx)
> > + * and never dereference that ctx in the program.
> > + * 'struct pt_regs' is a type mismatch for socket
> > + * filter that should be using 'struct __sk_buff'.
> > + */
> > + goto out;
> > + }
> > +
> > + ret = do_check(env);
> > +out:
> > + if (env->cur_state) {
>
> I think env->cur_state will never be NULL here. This check is necessary
> before this patch (when we allocate cur_state in do_check()).
yeah. good catch. 'if' can be dropped. I'll follow up with a clean up patch or
will fold it if respin is necessary for other reasons.
