On Mon Jan 6, 2025 at 9:31 PM CET, Eduard Zingerman wrote:
> On Mon, 2025-01-06 at 18:15 +0100, Arthur Fabre wrote:
[...]
> This patch is correct as far as I can tell.
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
Thanks for the review!
> [...]
>
> > @@ -18770,6 +18780,21 @@ static int do_check(struct bpf_verifier_env *env)
> > return err;
> >
> > mark_reg_scratched(env, BPF_REG_0);
> > +
> > + if (insn->src_reg == 0 && insn->imm == BPF_FUNC_tail_call) {
> > + /* Explore both cases: tail_call fails and we fallthrough,
> > + * or it succeeds and we exit the current function.
> > + */
> > + if (!push_stack(env, env->insn_idx + 1, env->insn_idx, false))
> > + return -ENOMEM;
> > + /* bpf_tail_call() doesn't set r0 on failure / in the fallthrough case.
> > + * But it does on success, so we have to mark it after queueing the
> > + * fallthrough case, but before prepare_func_exit().
> > + */
> > + __mark_reg_unknown(env, &state->frame[state->curframe]->regs[BPF_REG_0]);
> > + exit = BPF_EXIT_TAIL_CALL;
> > + goto process_bpf_exit_full;
> > + }
>
> Nit: it's a bit unfortunate, that this logic is inside do_check,
> instead of check_helper_call() and check_ld_abs().
> But it makes BPF_EXIT_* propagation simpler.
Agreed, it's unfortunate to add more to do_check().
I tried passing exit / BPF_EXIT_* by pointer to check_helper_call() and
check_ld_abs(), but that still means we need a conditional in do_check()
to see if it's set:
if (exit != NULL)
oto process_bpf_exit_full;
Happy to switch to that if you think it's cleaner.
[...]
