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.
[...]
