On Wed, Nov 15, 2023 at 9:18 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> Move code for simulated stack frame creation to a separate utility
> function. This function would be used in the follow-up change for
> callbacks handling.
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 87 +++++++++++++++++++++++++------------------
> 1 file changed, 51 insertions(+), 36 deletions(-)
>
LGTM, minor nit below.
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0576fc1ddc4d..d9513fd58c7c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9542,11 +9542,10 @@ static int set_callee_state(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,
> - set_callee_state_fn set_callee_state_cb)
> +static int setup_func_entry(struct bpf_verifier_env *env, int subprog, int callsite,
> + set_callee_state_fn set_callee_state_cb,
> + struct bpf_verifier_state *state)
> {
> - struct bpf_verifier_state *state = env->cur_state;
> struct bpf_func_state *caller, *callee;
> int err;
>
> @@ -9556,13 +9555,56 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> return -E2BIG;
> }
>
> - caller = state->frame[state->curframe];
> if (state->frame[state->curframe + 1]) {
> verbose(env, "verifier bug. Frame %d already allocated\n",
> state->curframe + 1);
> return -EFAULT;
> }
>
> + caller = state->frame[state->curframe];
> + callee = kzalloc(sizeof(*callee), GFP_KERNEL);
> + if (!callee)
> + return -ENOMEM;
> + state->frame[state->curframe + 1] = callee;
> +
> + /* callee cannot access r0, r6 - r9 for reading and has to write
> + * into its own stack before reading from it.
> + * callee can read/write into caller's stack
> + */
> + init_func_state(env, callee,
> + /* remember the callsite, it will be used by bpf_exit */
> + callsite,
> + state->curframe + 1 /* frameno within this callchain */,
> + subprog /* subprog number within this prog */);
> + /* Transfer references to the callee */
> + err = copy_reference_state(callee, caller);
> + if (err)
> + goto err_out;
> +
> + err = set_callee_state_cb(env, caller, callee, callsite);
> + if (err)
> + goto err_out;
given we are touching and moving this code, it might make sense to
make it a bit more succinct with this pattern:
err = copy_reference_state(...);
err = err ?: set_callee_state_cb();
if (err)
goto err_out;
Error handling is a bit less distracting this way.
> +
> + /* only increment it after check_reg_arg() finished */
> + state->curframe++;
> +
> + return 0;
> +
> +err_out:
> + free_func_state(callee);
> + state->frame[state->curframe + 1] = NULL;
> + return err;
> +}
> +
[...]
