BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jiong Wang <jiong.wang@netronome.com>
Cc: Andrii Nakryiko <andriin@fb.com>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Edward Cree <ecree@solarflare.com>,
	"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	bpf <bpf@vger.kernel.org>, Networking <netdev@vger.kernel.org>,
	oss-drivers@netronome.com
Subject: Re: [oss-drivers] Re: [RFC bpf-next 2/8] bpf: extend list based insn patching infra to verification layer
Date: Fri, 12 Jul 2019 12:51:38 -0700
Message-ID: <CAEf4BzZX45+QJLnHdwW-Cmo1uAFd+4zzds0jJoQVWFLrUVABgA@mail.gmail.com> (raw)
In-Reply-To: <87muhk2264.fsf@netronome.com>

On Thu, Jul 11, 2019 at 5:20 AM Jiong Wang <jiong.wang@netronome.com> wrote:
>
>
> Jiong Wang writes:
>
> > Andrii Nakryiko writes:
> >
> >> On Thu, Jul 4, 2019 at 2:32 PM Jiong Wang <jiong.wang@netronome.com> wrote:
> >>>
> >>> Verification layer also needs to handle auxiliar info as well as adjusting
> >>> subprog start.
> >>>
> >>> At this layer, insns inside patch buffer could be jump, but they should
> >>> have been resolved, meaning they shouldn't jump to insn outside of the
> >>> patch buffer. Lineration function for this layer won't touch insns inside
> >>> patch buffer.
> >>>
> >>> Adjusting subprog is finished along with adjusting jump target when the
> >>> input will cover bpf to bpf call insn, re-register subprog start is cheap.
> >>> But adjustment when there is insn deleteion is not considered yet.
> >>>
> >>> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
> >>> ---
> >>>  kernel/bpf/verifier.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 150 insertions(+)
> >>>
> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >>> index a2e7637..2026d64 100644
> >>> --- a/kernel/bpf/verifier.c
> >>> +++ b/kernel/bpf/verifier.c
> >>> @@ -8350,6 +8350,156 @@ static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
> >>>         }
> >>>  }
> >>>
> >>> +/* Linearize bpf list insn to array (verifier layer). */
> >>> +static struct bpf_verifier_env *
> >>> +verifier_linearize_list_insn(struct bpf_verifier_env *env,
> >>> +                            struct bpf_list_insn *list)
> >>
> >> It's unclear why this returns env back? It's not allocating a new env,
> >> so it's weird and unnecessary. Just return error code.
> >
> > The reason is I was thinking we have two layers in BPF, the core and the
> > verifier.
> >
> > For core layer (the relevant file is core.c), when doing patching, the
> > input is insn list and bpf_prog, the linearization should linearize the
> > insn list into insn array, and also whatever others affect inside bpf_prog
> > due to changing on insns, for example line info inside prog->aux. So the
> > return value is bpf_prog for core layer linearization hook.
> >
> > For verifier layer, it is similar, but the context if bpf_verifier_env, the
> > linearization hook should linearize the insn list, and also those affected
> > inside env, for example bpf_insn_aux_data, so the return value is
> > bpf_verifier_env, meaning returning an updated verifier context
> > (bpf_verifier_env) after insn list linearization.
>
> Realized your point is no new env is allocated, so just return error
> code. Yes, the env pointer is not changed, just internal data is
> updated. Return bpf_verifier_env mostly is trying to make the hook more
> clear that it returns an updated "context" where the linearization happens,
> for verifier layer, it is bpf_verifier_env, and for core layer, it is
> bpf_prog, so return value was designed to return these two types.

Oh, I missed that core layer returns bpf_prog*. I think this is
confusing as hell and is very contrary to what one would expect. If
the function doesn't allocate those objects, it shouldn't return them,
except for rare cases of some accessor functions. Me reading this,
I'll always be suprised and will have to go skim code just to check
whether those functions really return new bpf_prog or
bpf_verifier_env, respectively.

Please change them both to just return error code.

>
> >
> > Make sense?
> >
> > Regards,
> > Jiong
> >
> >>
> >>> +{
> >>> +       u32 *idx_map, idx, orig_cnt, fini_cnt = 0;
> >>> +       struct bpf_subprog_info *new_subinfo;
> >>> +       struct bpf_insn_aux_data *new_data;
> >>> +       struct bpf_prog *prog = env->prog;
> >>> +       struct bpf_verifier_env *ret_env;
> >>> +       struct bpf_insn *insns, *insn;
> >>> +       struct bpf_list_insn *elem;
> >>> +       int ret;
> >>> +
> >>> +       /* Calculate final size. */
> >>> +       for (elem = list; elem; elem = elem->next)
> >>> +               if (!(elem->flag & LIST_INSN_FLAG_REMOVED))
> >>> +                       fini_cnt++;
> >>> +
> >>> +       orig_cnt = prog->len;
> >>> +       insns = prog->insnsi;
> >>> +       /* If prog length remains same, nothing else to do. */
> >>> +       if (fini_cnt == orig_cnt) {
> >>> +               for (insn = insns, elem = list; elem; elem = elem->next, insn++)
> >>> +                       *insn = elem->insn;
> >>> +               return env;
> >>> +       }
> >>> +       /* Realloc insn buffer when necessary. */
> >>> +       if (fini_cnt > orig_cnt)
> >>> +               prog = bpf_prog_realloc(prog, bpf_prog_size(fini_cnt),
> >>> +                                       GFP_USER);
> >>> +       if (!prog)
> >>> +               return ERR_PTR(-ENOMEM);
> >>> +       insns = prog->insnsi;
> >>> +       prog->len = fini_cnt;
> >>> +       ret_env = env;
> >>> +
> >>> +       /* idx_map[OLD_IDX] = NEW_IDX */
> >>> +       idx_map = kvmalloc(orig_cnt * sizeof(u32), GFP_KERNEL);
> >>> +       if (!idx_map)
> >>> +               return ERR_PTR(-ENOMEM);
> >>> +       memset(idx_map, 0xff, orig_cnt * sizeof(u32));
> >>> +
> >>> +       /* Use the same alloc method used when allocating env->insn_aux_data. */
> >>> +       new_data = vzalloc(array_size(sizeof(*new_data), fini_cnt));
> >>> +       if (!new_data) {
> >>> +               kvfree(idx_map);
> >>> +               return ERR_PTR(-ENOMEM);
> >>> +       }
> >>> +
> >>> +       /* Copy over insn + calculate idx_map. */
> >>> +       for (idx = 0, elem = list; elem; elem = elem->next) {
> >>> +               int orig_idx = elem->orig_idx - 1;
> >>> +
> >>> +               if (orig_idx >= 0) {
> >>> +                       idx_map[orig_idx] = idx;
> >>> +
> >>> +                       if (elem->flag & LIST_INSN_FLAG_REMOVED)
> >>> +                               continue;
> >>> +
> >>> +                       new_data[idx] = env->insn_aux_data[orig_idx];
> >>> +
> >>> +                       if (elem->flag & LIST_INSN_FLAG_PATCHED)
> >>> +                               new_data[idx].zext_dst =
> >>> +                                       insn_has_def32(env, &elem->insn);
> >>> +               } else {
> >>> +                       new_data[idx].seen = true;
> >>> +                       new_data[idx].zext_dst = insn_has_def32(env,
> >>> +                                                               &elem->insn);
> >>> +               }
> >>> +               insns[idx++] = elem->insn;
> >>> +       }
> >>> +
> >>> +       new_subinfo = kvzalloc(sizeof(env->subprog_info), GFP_KERNEL);
> >>> +       if (!new_subinfo) {
> >>> +               kvfree(idx_map);
> >>> +               vfree(new_data);
> >>> +               return ERR_PTR(-ENOMEM);
> >>> +       }
> >>> +       memcpy(new_subinfo, env->subprog_info, sizeof(env->subprog_info));
> >>> +       memset(env->subprog_info, 0, sizeof(env->subprog_info));
> >>> +       env->subprog_cnt = 0;
> >>> +       env->prog = prog;
> >>> +       ret = add_subprog(env, 0);
> >>> +       if (ret < 0) {
> >>> +               ret_env = ERR_PTR(ret);
> >>> +               goto free_all_ret;
> >>> +       }
> >>> +       /* Relocate jumps using idx_map.
> >>> +        *   old_dst = jmp_insn.old_target + old_pc + 1;
> >>> +        *   new_dst = idx_map[old_dst] = jmp_insn.new_target + new_pc + 1;
> >>> +        *   jmp_insn.new_target = new_dst - new_pc - 1;
> >>> +        */
> >>> +       for (idx = 0, elem = list; elem; elem = elem->next) {
> >>> +               int orig_idx = elem->orig_idx;
> >>> +
> >>> +               if (elem->flag & LIST_INSN_FLAG_REMOVED)
> >>> +                       continue;
> >>> +               if ((elem->flag & LIST_INSN_FLAG_PATCHED) || !orig_idx) {
> >>> +                       idx++;
> >>> +                       continue;
> >>> +               }
> >>> +
> >>> +               ret = bpf_jit_adj_imm_off(&insns[idx], orig_idx - 1, idx,
> >>> +                                         idx_map);
> >>> +               if (ret < 0) {
> >>> +                       ret_env = ERR_PTR(ret);
> >>> +                       goto free_all_ret;
> >>> +               }
> >>> +               /* Recalculate subprog start as we are at bpf2bpf call insn. */
> >>> +               if (ret > 0) {
> >>> +                       ret = add_subprog(env, idx + insns[idx].imm + 1);
> >>> +                       if (ret < 0) {
> >>> +                               ret_env = ERR_PTR(ret);
> >>> +                               goto free_all_ret;
> >>> +                       }
> >>> +               }
> >>> +               idx++;
> >>> +       }
> >>> +       if (ret < 0) {
> >>> +               ret_env = ERR_PTR(ret);
> >>> +               goto free_all_ret;
> >>> +       }
> >>> +
> >>> +       env->subprog_info[env->subprog_cnt].start = fini_cnt;
> >>> +       for (idx = 0; idx <= env->subprog_cnt; idx++)
> >>> +               new_subinfo[idx].start = env->subprog_info[idx].start;
> >>> +       memcpy(env->subprog_info, new_subinfo, sizeof(env->subprog_info));
> >>> +
> >>> +       /* Adjust linfo.
> >>> +        * FIXME: no support for insn removal at the moment.
> >>> +        */
> >>> +       if (prog->aux->nr_linfo) {
> >>> +               struct bpf_line_info *linfo = prog->aux->linfo;
> >>> +               u32 nr_linfo = prog->aux->nr_linfo;
> >>> +
> >>> +               for (idx = 0; idx < nr_linfo; idx++)
> >>> +                       linfo[idx].insn_off = idx_map[linfo[idx].insn_off];
> >>> +       }
> >>> +       vfree(env->insn_aux_data);
> >>> +       env->insn_aux_data = new_data;
> >>> +       goto free_mem_list_ret;
> >>> +free_all_ret:
> >>> +       vfree(new_data);
> >>> +free_mem_list_ret:
> >>> +       kvfree(new_subinfo);
> >>> +       kvfree(idx_map);
> >>> +       return ret_env;
> >>> +}
> >>> +
> >>>  static int opt_remove_dead_code(struct bpf_verifier_env *env)
> >>>  {
> >>>         struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
> >>> --
> >>> 2.7.4
> >>>
>

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-04 21:26 [RFC bpf-next 0/8] bpf: accelerate insn patching speed Jiong Wang
2019-07-04 21:26 ` [RFC bpf-next 1/8] bpf: introducing list based insn patching infra to core layer Jiong Wang
2019-07-10 17:49   ` Andrii Nakryiko
2019-07-11 11:53     ` Jiong Wang
2019-07-12 19:48       ` Andrii Nakryiko
2019-07-15  9:58         ` Jiong Wang
2019-07-04 21:26 ` [RFC bpf-next 2/8] bpf: extend list based insn patching infra to verification layer Jiong Wang
2019-07-10 17:50   ` Andrii Nakryiko
2019-07-11 11:59     ` [oss-drivers] " Jiong Wang
2019-07-11 12:20       ` Jiong Wang
2019-07-12 19:51         ` Andrii Nakryiko [this message]
2019-07-15 10:02           ` Jiong Wang
2019-07-15 22:29             ` Andrii Nakryiko
2019-07-16  8:12               ` Jiong Wang
2019-07-04 21:26 ` [RFC bpf-next 3/8] bpf: migrate jit blinding to list patching infra Jiong Wang
2019-07-04 21:26 ` [RFC bpf-next 4/8] bpf: migrate convert_ctx_accesses " Jiong Wang
2019-07-04 21:26 ` [RFC bpf-next 5/8] bpf: migrate fixup_bpf_calls " Jiong Wang
2019-07-04 21:26 ` [RFC bpf-next 6/8] bpf: migrate zero extension opt " Jiong Wang
2019-07-04 21:26 ` [RFC bpf-next 7/8] bpf: migrate insn remove " Jiong Wang
2019-07-04 21:26 ` [RFC bpf-next 8/8] bpf: delete all those code around old insn patching infrastructure Jiong Wang
2019-07-10 17:39 ` [RFC bpf-next 0/8] bpf: accelerate insn patching speed Andrii Nakryiko
2019-07-11 11:22   ` Jiong Wang
2019-07-12 19:43     ` Andrii Nakryiko
2019-07-15  9:21       ` Jiong Wang
2019-07-15 22:55         ` Andrii Nakryiko
2019-07-15 23:00           ` Andrii Nakryiko
2019-07-16  8:50           ` Jiong Wang
2019-07-16 16:17             ` Alexei Starovoitov
2019-07-16 19:39               ` Jiong Wang
2019-07-16 22:12               ` Jakub Kicinski
2019-07-17  1:17                 ` Alexei Starovoitov
2019-07-16 17:49             ` Andrii Nakryiko

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAEf4BzZX45+QJLnHdwW-Cmo1uAFd+4zzds0jJoQVWFLrUVABgA@mail.gmail.com \
    --to=andrii.nakryiko@gmail.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andriin@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=ecree@solarflare.com \
    --cc=jakub.kicinski@netronome.com \
    --cc=jiong.wang@netronome.com \
    --cc=naveen.n.rao@linux.vnet.ibm.com \
    --cc=netdev@vger.kernel.org \
    --cc=oss-drivers@netronome.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org bpf@archiver.kernel.org
	public-inbox-index bpf


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/ public-inbox