On Wed, Oct 19, 2022 at 09:24:18PM IST, Alexei Starovoitov wrote:
> On Tue, Oct 18, 2022 at 10:43 PM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > On Wed, Oct 19, 2022 at 07:05:26AM IST, Alexei Starovoitov wrote:
> > > On Thu, Oct 13, 2022 at 11:52:44AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > To prepare the BPF verifier to handle special fields in both map values
> > > > and program allocated types coming from program BTF, we need to refactor
> > > > the kptr_off_tab handling code into something more generic and reusable
> > > > across both cases to avoid code duplication.
> > > >
> > > > Later patches also require passing this data to helpers at runtime, so
> > > > that they can work on user defined types, initialize them, destruct
> > > > them, etc.
> > > >
> > > > The main observation is that both map values and such allocated types
> > > > point to a type in program BTF, hence they can be handled similarly. We
> > > > can prepare a field metadata table for both cases and store them in
> > > > struct bpf_map or struct btf depending on the use case.
> > > >
> > > > Hence, refactor the code into generic btf_type_fields and btf_field
> > > > member structs. The btf_type_fields represents the fields of a specific
> > > > btf_type in user BTF. The cnt indicates the number of special fields we
> > > > successfully recognized, and field_mask is a bitmask of fields that were
> > > > found, to enable quick determination of availability of a certain field.
> > > >
> > > > Subsequently, refactor the rest of the code to work with these generic
> > > > types, remove assumptions about kptr and kptr_off_tab, rename variables
> > > > to more meaningful names, etc.
> > > >
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > > ---
> > > > include/linux/bpf.h | 103 +++++++++++++-------
> > > > include/linux/btf.h | 4 +-
> > > > kernel/bpf/arraymap.c | 13 ++-
> > > > kernel/bpf/btf.c | 64 ++++++-------
> > > > kernel/bpf/hashtab.c | 14 ++-
> > > > kernel/bpf/map_in_map.c | 13 ++-
> > > > kernel/bpf/syscall.c | 203 +++++++++++++++++++++++-----------------
> > > > kernel/bpf/verifier.c | 96 ++++++++++---------
> > > > 8 files changed, 289 insertions(+), 221 deletions(-)
> > > >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index 9e7d46d16032..25e77a172d7c 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -164,35 +164,41 @@ struct bpf_map_ops {
> > > > };
> > > >
> > > > enum {
> > > > - /* Support at most 8 pointers in a BPF map value */
> > > > - BPF_MAP_VALUE_OFF_MAX = 8,
> > > > - BPF_MAP_OFF_ARR_MAX = BPF_MAP_VALUE_OFF_MAX +
> > > > + /* Support at most 8 pointers in a BTF type */
> > > > + BTF_FIELDS_MAX = 8,
> > > > + BPF_MAP_OFF_ARR_MAX = BTF_FIELDS_MAX +
> > > > 1 + /* for bpf_spin_lock */
> > > > 1, /* for bpf_timer */
> > > > };
> > > >
> > > > -enum bpf_kptr_type {
> > > > - BPF_KPTR_UNREF,
> > > > - BPF_KPTR_REF,
> > > > +enum btf_field_type {
> > > > + BPF_KPTR_UNREF = (1 << 2),
> > > > + BPF_KPTR_REF = (1 << 3),
> > > > + BPF_KPTR = BPF_KPTR_UNREF | BPF_KPTR_REF,
> > > > };
> > > >
> > > > -struct bpf_map_value_off_desc {
> > > > +struct btf_field_kptr {
> > > > + struct btf *btf;
> > > > + struct module *module;
> > > > + btf_dtor_kfunc_t dtor;
> > > > + u32 btf_id;
> > > > +};
> > > > +
> > > > +struct btf_field {
> > > > u32 offset;
> > > > - enum bpf_kptr_type type;
> > > > - struct {
> > > > - struct btf *btf;
> > > > - struct module *module;
> > > > - btf_dtor_kfunc_t dtor;
> > > > - u32 btf_id;
> > > > - } kptr;
> > > > + enum btf_field_type type;
> > > > + union {
> > > > + struct btf_field_kptr kptr;
> > > > + };
> > > > };
> > > >
> > > > -struct bpf_map_value_off {
> > > > - u32 nr_off;
> > > > - struct bpf_map_value_off_desc off[];
> > > > +struct btf_type_fields {
> > >
> > > How about btf_record instead ?
> > > Then btf_type_fields_has_field() will become btf_record_has_field() ?
> > >
> >
> > I guess btf_record is ok. I thought of just making it btf_fields, but then
> > bpf_map_free_fields (for freeing this struct) and bpf_obj_free_fields (for
> > freeing actual fields of object) gets confusing.
> >
> > Or to be more precise I could name the struct btf_type_record,
> > but the member variable record in all places.
>
> What "_type_" prefix adds to btf_record ?
>
> btf already has Type in the abbrev.
>
Well, it's the record of a btf_type, so btf_type_record.
> And from the other email:
>
> > I agree, what do you think of calling it btf_type_has_field? You pass > in the
> > btf_type_record and the field type.
>
> btf_type_has_field doesn't sound right.
Ok, let's go with just the btf_record naming.
