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.
