netdev - Re: [PATCH bpf-next 0/3] bpf: cleanups on managing subprog information

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 2 May 2018 20:22:01 +0100
From:   Jiong Wang <jiong.wang@...ronome.com>
To:     John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     borkmann@...earbox.net, ecree@...arflare.com,
        netdev@...r.kernel.org, oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next 0/3] bpf: cleanups on managing subprog
 information

On 02/05/2018 18:24, John Fastabend wrote:
> On 05/02/2018 09:59 AM, Jiong Wang wrote:
>> On 01/05/2018 23:22, Alexei Starovoitov wrote:
>> ...
>>> [   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,
>> Jiong
>>
> Also 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

>
> @@ -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;