On Mon, 17 Jul 2023 at 22:55, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Jul 17, 2023 at 9:22 AM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > On Sat, 15 Jul 2023 at 03:28, Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 08:02:26AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > Introduce support in the verifier for generating a subprogram and
> > > > include it as part of a BPF program dynamically after the do_check
> > > > phase is complete. The appropriate place of invocation would be
> > > > do_misc_fixups.
> > > >
> > > > Since they are always appended to the end of the instruction sequence of
> > > > the program, it becomes relatively inexpensive to do the related
> > > > adjustments to the subprog_info of the program. Only the fake exit
> > > > subprogram is shifted forward by 1, making room for our invented subprog.
> > > >
> > > > This is useful to insert a new subprogram and obtain its function
> > > > pointer. The next patch will use this functionality to insert a default
> > > > exception callback which will be invoked after unwinding the stack.
> > > >
> > > > Note that these invented subprograms are invisible to userspace, and
> > > > never reported in BPF_OBJ_GET_INFO_BY_ID etc. For now, only a single
> > > > invented program is supported, but more can be easily supported in the
> > > > future.
> > > >
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > > ---
> > > > include/linux/bpf.h | 1 +
> > > > include/linux/bpf_verifier.h | 4 +++-
> > > > kernel/bpf/core.c | 4 ++--
> > > > kernel/bpf/syscall.c | 19 ++++++++++++++++++-
> > > > kernel/bpf/verifier.c | 29 ++++++++++++++++++++++++++++-
> > > > 5 files changed, 52 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index 360433f14496..70f212dddfbf 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -1385,6 +1385,7 @@ struct bpf_prog_aux {
> > > > bool sleepable;
> > > > bool tail_call_reachable;
> > > > bool xdp_has_frags;
> > > > + bool invented_prog;
> > > > /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
> > > > const struct btf_type *attach_func_proto;
> > > > /* function name for valid attach_btf_id */
> > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > > > index f70f9ac884d2..360aa304ec09 100644
> > > > --- a/include/linux/bpf_verifier.h
> > > > +++ b/include/linux/bpf_verifier.h
> > > > @@ -540,6 +540,7 @@ struct bpf_subprog_info {
> > > > bool has_tail_call;
> > > > bool tail_call_reachable;
> > > > bool has_ld_abs;
> > > > + bool invented_prog;
> > > > bool is_async_cb;
> > > > };
> > > >
> > > > @@ -594,10 +595,11 @@ struct bpf_verifier_env {
> > > > bool bypass_spec_v1;
> > > > bool bypass_spec_v4;
> > > > bool seen_direct_write;
> > > > + bool invented_prog;
> > >
> > > Instead of a flag in two places how about adding aux->func_cnt_real
> > > and use it in JITing and free-ing while get_info*() keep using aux->func_cnt.
> > >
> >
> > That does seem better, thanks. I'll make the change in v2.
> >
> > > > +/* The function requires that first instruction in 'patch' is insnsi[prog->len - 1] */
> > > > +static int invent_subprog(struct bpf_verifier_env *env, struct bpf_insn *patch, int len)
> > > > +{
> > > > + struct bpf_subprog_info *info = env->subprog_info;
> > > > + int cnt = env->subprog_cnt;
> > > > + struct bpf_prog *prog;
> > > > +
> > > > + if (env->invented_prog) {
> > > > + verbose(env, "verifier internal error: only one invented prog supported\n");
> > > > + return -EFAULT;
> > > > + }
> > > > + prog = bpf_patch_insn_data(env, env->prog->len - 1, patch, len);
> > >
> > > The actual patching is not necessary.
> > > bpf_prog_realloc() and memcpy would be enough, no?
> > >
> >
> > Yes, it should be fine. But I didn't want to special case things here
> > just to make sure assumptions elsewhere don't break.
> > E.g. code readily assumes every insn has its own insn_aux_data which
> > might be broken if we don't expand it.
> > I think bpf_patch_insn_single is already doing a realloc (and reusing
> > trailing space in current allocation if available), so it didn't seem
> > worth it to me.
> >
> > If you still feel it's better I can analyze if anything might break
> > and make the change.
>
> bpf_patch_insn_data() is a known performance bottleneck.
> Folks have been trying to optimize it in the past.
> It's certainly delicate code.
> I guess since this extra subprog will only be added once
> we can live with unnecessary overhead of bpf_patch_insn_data().
> Just add the comment that we're not patching existing insn and
> all of adjust* ops are nop.
Ack.
