Re: [RFC bpf-next v4 2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types.

[Kui-Feng Lee <sinquersw@xxxxxxxxx>]

On 2/2/24 16:52, Martin KaFai Lau wrote:
> On 2/2/24 2:05 PM, thinker.li@xxxxxxxxx wrote:
> > From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> >
> > The verifier calls btf_struct_access() to check the access for
> > PTR_TO_BTF_ID. btf_struct_access() supported only pointer to struct types
> > (including union). We add the support of scalar types and array types.
> >
> > btf_reloc_array_access() is responsible for relocating the access from 
> > the
> > whole array to an element in the array. That means to adjust the offset
> > relatively to the start of an element and change the type to the type of
> > the element. With this relocation, we can check the access against the
> > element type instead of the array type itself.
> >
> > After relocation, the struct types, including union types, will continue
> > the loop of btf_struct_walk(). Other types are treated as scalar types,
> > including pointers, and return from btf_struct_access().
>
> Unless there is an immediate use case to support PTR_MAYBE_NULL on a 
> non-struct pointer, I would suggest to separate the other pointer type 
> support from the current PTR_MAYBE_NULL feature patchset. afaik, they 
> are orthogonal.


Totally agree!


> > Signed-off-by: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> > ---
> >   kernel/bpf/btf.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 61 insertions(+)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 0847035bba99..d3f94d04c69d 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6590,6 +6590,61 @@ static int btf_struct_walk(struct 
> > bpf_verifier_log *log, const struct btf *btf,
> >       return -EINVAL;
> >   }
> > +/* Relocate the access relatively to the beginning of an element in an
> > + * array.
> > + *
> > + * The offset is adjusted relatively to the beginning of the element 
> > and the
> > + * type is adjusted to the type of the element.
> > + *
> > + * Return NULL for scalar, enum, and pointer type.
> > + * Return a btf_type pointer for struct and union.
> > + */
> > +static const struct btf_type *
> > +btf_reloc_array_access(struct bpf_verifier_log *log, const struct btf 
> > *btf,
> > +               const struct btf_type *t, int *off, int size)
> > +{
> > +    const struct btf_type *rt, *elem_type;
> > +    u32 rt_size, elem_id, total_nelems, rt_id, elem_size;
> > +    u32 elem_idx;
> > +
> > +    rt = __btf_resolve_size(btf, t, &rt_size, &elem_type, &elem_id,
> > +                &total_nelems, &rt_id);
> > +    if (IS_ERR(rt))
> > +        return rt;
> > +    if (btf_type_is_array(rt)) {
> > +        if (*off >= rt_size) {
> > +            bpf_log(log, "access out of range of type %s with offset 
> > %d and size %u\n",
> > +                __btf_name_by_offset(btf, t->name_off), *off, rt_size);
> > +            return ERR_PTR(-EACCES);
> > +        }
> > +
> > +        /* Multi-dimensional arrays are flattened by
> > +         * __btf_resolve_size(). Check the comment in
> > +         * btf_struct_walk().
> > +         */
> > +        elem_size = rt_size / total_nelems;
> > +        elem_idx = *off / elem_size;
> > +        /* Relocate the offset relatively to the start of the
> > +         * element at elem_idx.
> > +         */
> > +        *off -= elem_idx * elem_size;
> > +        rt = elem_type;
> > +        rt_size = elem_size;
> > +    }
> > +
> > +    if (btf_type_is_struct(rt))
> > +        return rt;
> > +
> > +    if (*off + size > rt_size) {
> > +        bpf_log(log, "access beyond the range of type %s with offset 
> > %d and size %d\n",
> > +            __btf_name_by_offset(btf, rt->name_off), *off, size);
> > +        return ERR_PTR(-EACCES);
> > +    }
> > +
> > +    /* The access is accepted as a scalar. */
> > +    return NULL;
> > +}
> > +
> >   int btf_struct_access(struct bpf_verifier_log *log,
> >                 const struct bpf_reg_state *reg,
> >                 int off, int size, enum bpf_access_type atype 
> > __maybe_unused,
> > @@ -6625,6 +6680,12 @@ int btf_struct_access(struct bpf_verifier_log 
> > *log,
> >       }
> >       t = btf_type_by_id(btf, id);
> > +    t = btf_reloc_array_access(log, btf, t, &off, size);
> > +    if (IS_ERR(t))
> > +        return PTR_ERR(t);
> > +    if (!t)
> > +        return SCALAR_VALUE;
> > +
> >       do {
> >           err = btf_struct_walk(log, btf, t, off, size, &id, 
> > &tmp_flag, field_name);