Re: [External] Re: [PATCH bpf-next v2 1/9] bpf: tracing: add support to record and check the accessed args

[梦龙董]

On Tue, Mar 12, 2024 at 10:09 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Mar 11, 2024 at 7:01 PM 梦龙董 <dongmenglong.8@xxxxxxxxxxxxx> wrote:
> >
> > On Tue, Mar 12, 2024 at 9:46 AM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Mon, Mar 11, 2024 at 2:34 AM Menglong Dong
> > > <dongmenglong.8@xxxxxxxxxxxxx> wrote:
> > > >
> > > > In this commit, we add the 'accessed_args' field to struct bpf_prog_aux,
> > > > which is used to record the accessed index of the function args in
> > > > btf_ctx_access().
> > > >
> > > > Meanwhile, we add the function btf_check_func_part_match() to compare the
> > > > accessed function args of two function prototype. This function will be
> > > > used in the following commit.
> > > >
> > > > Signed-off-by: Menglong Dong <dongmenglong.8@xxxxxxxxxxxxx>
> > > > ---
> > > >  include/linux/bpf.h |   4 ++
> > > >  kernel/bpf/btf.c    | 108 +++++++++++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 110 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index 95e07673cdc1..0f677fdcfcc7 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -1461,6 +1461,7 @@ struct bpf_prog_aux {
> > > >         const struct btf_type *attach_func_proto;
> > > >         /* function name for valid attach_btf_id */
> > > >         const char *attach_func_name;
> > > > +       u64 accessed_args;
> > > >         struct bpf_prog **func;
> > > >         void *jit_data; /* JIT specific data. arch dependent */
> > > >         struct bpf_jit_poke_descriptor *poke_tab;
> > > > @@ -2565,6 +2566,9 @@ struct bpf_reg_state;
> > > >  int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog);
> > > >  int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog,
> > > >                          struct btf *btf, const struct btf_type *t);
> > > > +int btf_check_func_part_match(struct btf *btf1, const struct btf_type *t1,
> > > > +                             struct btf *btf2, const struct btf_type *t2,
> > > > +                             u64 func_args);
> > > >  const char *btf_find_decl_tag_value(const struct btf *btf, const struct btf_type *pt,
> > > >                                     int comp_idx, const char *tag_key);
> > > >  int btf_find_next_decl_tag(const struct btf *btf, const struct btf_type *pt,
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 170d017e8e4a..c2a0299d4358 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -6125,19 +6125,24 @@ static bool is_int_ptr(struct btf *btf, const struct btf_type *t)
> > > >  }
> > > >
> > > >  static u32 get_ctx_arg_idx(struct btf *btf, const struct btf_type *func_proto,
> > > > -                          int off)
> > > > +                          int off, int *aligned_idx)
> > > >  {
> > > >         const struct btf_param *args;
> > > >         const struct btf_type *t;
> > > >         u32 offset = 0, nr_args;
> > > >         int i;
> > > >
> > > > +       if (aligned_idx)
> > > > +               *aligned_idx = -ENOENT;
> > > > +
> > > >         if (!func_proto)
> > > >                 return off / 8;
> > > >
> > > >         nr_args = btf_type_vlen(func_proto);
> > > >         args = (const struct btf_param *)(func_proto + 1);
> > > >         for (i = 0; i < nr_args; i++) {
> > > > +               if (aligned_idx && offset == off)
> > > > +                       *aligned_idx = i;
> > > >                 t = btf_type_skip_modifiers(btf, args[i].type, NULL);
> > > >                 offset += btf_type_is_ptr(t) ? 8 : roundup(t->size, 8);
> > > >                 if (off < offset)
> > > > @@ -6207,7 +6212,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > >                         tname, off);
> > > >                 return false;
> > > >         }
> > > > -       arg = get_ctx_arg_idx(btf, t, off);
> > > > +       arg = get_ctx_arg_idx(btf, t, off, NULL);
> > > >         args = (const struct btf_param *)(t + 1);
> > > >         /* if (t == NULL) Fall back to default BPF prog with
> > > >          * MAX_BPF_FUNC_REG_ARGS u64 arguments.
> > > > @@ -6217,6 +6222,9 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > >                 /* skip first 'void *__data' argument in btf_trace_##name typedef */
> > > >                 args++;
> > > >                 nr_args--;
> > > > +               prog->aux->accessed_args |= (1 << (arg + 1));
> > > > +       } else {
> > > > +               prog->aux->accessed_args |= (1 << arg);
> > >
> > > What do you need this aligned_idx for ?
> > > I'd expect that above "accessed_args |= (1 << arg);" is enough.
> > >
> >
> > Which aligned_idx? No aligned_idx in the btf_ctx_access(), and
> > aligned_idx is only used in the btf_check_func_part_match().
> >
> > In the btf_check_func_part_match(), I need to compare the
> > t1->args[i] and t2->args[j], which have the same offset. And
> > the aligned_idx is to find the "j" according to the offset of
> > t1->args[i].
>
> And that's my question.
> Why you don't do the max of accessed_args across all attach
> points and do btf_check_func_type_match() to that argno
> instead of nargs1.
> This 'offset += btf_type_is_ptr(t1) ? 8 : roundup...
> is odd.

Hi, I'm trying to make the bpf flexible enough. Let's take an example,
now we have the bpf program:

int test1_result = 0;
int BPF_PROG(test1, int a, long b, char c)
{
    test1_result = a + c;
    return 0;
}

In this program, only the 1st and 3rd arg is accessed. So all kernel
functions whose 1st arg is int and 3rd arg is char can be attached
by this bpf program, even if their 2nd arg is different.

And let's take another example for struct. This is our bpf program:

int test1_result = 0;
int BPF_PROG(test1, long a, long b, char c)
{
    test1_result = c;
    return 0;
}

Only the 3rd arg is accessed. And we have following kernel function:

int kernel_function1(long a, long b, char c)
{
xxx
}

struct test1 {
    long a;
    long b;
};
int kernel_function2(struct test1 a, char b)
{
xxx
}

The kernel_function1 and kernel_function2 should be compatible,
as the bpf program only accessed the ctx[2], whose offset is 16.
And the arg in kernel_function1() with offset 16 is "char c", the
arg in kernel_function2() with offset 16 is "char b", which is
compatible.

That's why we need to check the consistency of accessed args
by offset instead of function arg index.

I'm not sure if I express my idea clearly, is this what you are
asking?

Thanks!
Menglong Dong