From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiong Wang Subject: Re: [PATCH bpf-next 0/3] bpf: cleanups on managing subprog information Date: Wed, 2 May 2018 20:22:01 +0100 Message-ID: <22dff72c-531b-76f0-afd0-be1f7ff414f8@netronome.com> References: <1525127296-3573-1-git-send-email-jiong.wang@netronome.com> <20180501222257.cogbkcsncrmg54p5@ast-mbp> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: borkmann@iogearbox.net, ecree@solarflare.com, netdev@vger.kernel.org, oss-drivers@netronome.com To: John Fastabend , Alexei Starovoitov Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:36534 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750948AbeEBTW3 (ORCPT ); Wed, 2 May 2018 15:22:29 -0400 Received: by mail-wm0-f68.google.com with SMTP id n10so26243228wmc.1 for ; Wed, 02 May 2018 12:22:28 -0700 (PDT) In-Reply-To: Content-Language: en-GB Sender: netdev-owner@vger.kernel.org List-ID: 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;