Re: [PATCH bpf-next 0/3] bpf: cleanups on managing subprog information
From: Jiong Wang <hidden>
Date: 2018-05-02 19:22:29
On 02/05/2018 18:24, John Fastabend wrote:
On 05/02/2018 09:59 AM, Jiong Wang wrote:quoted
On 01/05/2018 23:22, Alexei Starovoitov wrote: ...quoted
[ 27.784931] ? bpf_int_jit_compile+0x7ac/0xab0 [ 27.785475] bpf_int_jit_compile+0x2b6/0xab0 [ 27.786001] ? do_jit+0x6020/0x6020 [ 27.786428] ? kasan_kmalloc+0xa0/0xd0 [ 27.786885] bpf_check+0x2c05/0x4c40 [ 27.787346] ? fixup_bpf_calls+0x1140/0x1140 [ 27.787865] ? kasan_unpoison_shadow+0x30/0x40 [ 27.788406] ? kasan_kmalloc+0xa0/0xd0 [ 27.788865] ? memset+0x1f/0x40 [ 27.789255] ? bpf_obj_name_cpy+0x2d/0x200 [ 27.789750] bpf_prog_load+0xb07/0xeb0 simply running test_verifier with JIT and kasan on.Ah, sorry, I should add "sysctl net/core/bpf_jit_enable=1" to my test script, error reproduced. convert_ctx_accesses and fixup_bpf_calls might insert ebpf insns that prog->len would change. The new fake "exit" subprog whose .start offset is prog->len should be updated as well. The "for" condition in adjust_subprog_starts: for (i = 0; i < env->subprog_cnt; i++) { need to be changed into: for (i = 0; i <= env->subprog_cnt; i++) { Will respin the patch set. Thanks. Regards, JiongAlso a bit of a nit, but if you are doing a respin. How about consider renaming BPF_MAX_SUBPROGS -> BPF_MAX_PROGS. It will make the naming more accurate and also avoid some diffs below where changing '>=' to '>' is required.
I have been pondering renaming BPF_MAX_SUBPROGS to other name like what you suggested, but failed to convince myself, mostly due to there are quite a few other variables etc that are using the "subprog" name convention, so I am thinking use subprog is also fine as traditional main prog/func is also a sub prog/func, it is just the entry one. So I am thinking it might be not worth renaming everything related, and tend to just keep it as is. Thanks. Regards, Jiong
quoted hunk ↗ jump to hunk
@@ -191,7 +191,7 @@ struct bpf_verifier_env { bool seen_direct_write; struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */ struct bpf_verifier_log log; - u32 subprog_starts[BPF_MAX_SUBPROGS]; + u32 subprog_starts[BPF_MAX_SUBPROGS + 1]; /* computes the stack depth of each bpf function */ u16 subprog_stack_depth[BPF_MAX_SUBPROGS + 1]; u32 subprog_cnt;