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.
> > + u32 cnt;
> > + u32 field_mask;
> > + struct btf_field fields[];
> > };
> >
> > -struct bpf_map_off_arr {
> > +struct btf_type_fields_off {
>
> struct btf_field_offs ?
>
> > u32 cnt;
> > u32 field_off[BPF_MAP_OFF_ARR_MAX];
> > u8 field_sz[BPF_MAP_OFF_ARR_MAX];
> > @@ -214,7 +220,7 @@ struct bpf_map {
> > u64 map_extra; /* any per-map-type extra fields */
> > u32 map_flags;
> > int spin_lock_off; /* >=0 valid offset, <0 error */
> > - struct bpf_map_value_off *kptr_off_tab;
> > + struct btf_type_fields *fields_tab;
>
> struct btf_record *record; ?
> The '_tab' suffix suppose to mean 'fieldS table' ?
> Just 'record' seems clear enough.
> Or
> struct btf_record *btf_record; ?
> if just 'record' ambiguous.
>
Ack. I think just 'record' is ok.
> > int timer_off; /* >=0 valid offset, <0 error */
> > u32 id;
> > int numa_node;
> > @@ -226,7 +232,7 @@ struct bpf_map {
> > struct obj_cgroup *objcg;
> > #endif
> > char name[BPF_OBJ_NAME_LEN];
> > - struct bpf_map_off_arr *off_arr;
> > + struct btf_type_fields_off *off_arr;
>
> 'off_arr' should probably be renamed as well.
> How about 'struct btf_field_offs *field_offs;' ?
>
Ack.
> > static inline void check_and_init_map_value(struct bpf_map *map, void *dst)
> > {
> > if (unlikely(map_value_has_spin_lock(map)))
> > memset(dst + map->spin_lock_off, 0, sizeof(struct bpf_spin_lock));
> > if (unlikely(map_value_has_timer(map)))
> > memset(dst + map->timer_off, 0, sizeof(struct bpf_timer));
> > - if (unlikely(map_value_has_kptrs(map))) {
> > - struct bpf_map_value_off *tab = map->kptr_off_tab;
> > + if (!IS_ERR_OR_NULL(map->fields_tab)) {
> > + struct btf_field *fields = map->fields_tab->fields;
>
> will become
> struct btf_field *fields = map->record->fields;
>
Ack for this and the rest below.
> [...]
