Re: [RFC bpf-next v3 06/11] bpf: validate value_type

[Kui-Feng Lee <sinquersw@gmail.com>]

On 9/25/23 18:03, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> A value_type should has three members; refcnt, state, and data.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 75 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index ef8a1edec891..fb684d2ee99d 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -99,6 +99,79 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
>>   static const struct btf_type *module_type;
>> +static bool check_value_member(struct btf *btf,
>> +                   const struct btf_member *member,
>> +                   int index,
>> +                   const char *value_name,
>> +                   const char *name, const char *type_name,
>> +                   u16 kind)
>> +{
>> +    const char *mname, *mtname;
>> +    const struct btf_type *mt;
>> +    s32 mtype_id;
>> +
>> +    mname = btf_name_by_offset(btf, member->name_off);
>> +    if (!*mname) {
>> +        pr_warn("The member %d of %s should have a name\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (strcmp(mname, name)) {
>> +        pr_warn("The member %d of %s should be refcnt\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    mtype_id = member->type;
>> +    mt = btf_type_by_id(btf, mtype_id);
>> +    mtname = btf_name_by_offset(btf, mt->name_off);
>> +    if (!*mtname) {
>> +        pr_warn("The type of the member %d of %s should have a name\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (strcmp(mtname, type_name)) {
>> +        pr_warn("The type of the member %d of %s should be 
>> refcount_t\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (btf_kind(mt) != kind) {
>> +        pr_warn("The type of the member %d of %s should be %d\n",
>> +            index, value_name, btf_kind(mt));
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static bool is_valid_value_type(struct btf *btf, s32 value_id,
>> +                const char *type_name, const char *value_name)
>> +{
>> +    const struct btf_member *member;
>> +    const struct btf_type *vt;
>> +
>> +    vt = btf_type_by_id(btf, value_id);
>> +    if (btf_vlen(vt) != 3) {
>> +        pr_warn("The number of %s's members should be 3, but we get 
>> %d\n",
>> +            value_name, btf_vlen(vt));
>> +        return false;
>> +    }
>> +    member = btf_type_member(vt);
>> +    if (!check_value_member(btf, member, 0, value_name,
>> +                "refcnt", "refcount_t", BTF_KIND_TYPEDEF))
>> +        return false;
>> +    member++;
>> +    if (!check_value_member(btf, member, 1, value_name,
>> +                "state", "bpf_struct_ops_state",
>> +                BTF_KIND_ENUM))
>> +        return false;
>> +    member++;
> 
> I wonder if giving BPF_STRUCT_OPS_COMMON_VALUE a proper struct will make 
> the validation cleaner. Like,
> 
> struct bpf_struct_ops_common {
>      refcount_t refcnt;
>      enum bpf_struct_ops_state state;
> };
> 
> wdyt?

It should work.

> 
>> +    if (!check_value_member(btf, member, 2, value_name,
>> +                "data", type_name, BTF_KIND_STRUCT))
> 
> Instead of checking name, I think this can directly check with the 
> st_ops->type.

Make sense

> 
>> +        return false;
>> +
>> +    return true;
>> +}
> 
>