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