Re: [PATCH bpf-next 1/2] libbpf: Skip zeroed or null fields if not found in the kernel type.

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

On 3/12/24 16:01, Andrii Nakryiko wrote:
> On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> Accept additional fields of a struct_ops type with all zero values even if
>> these fields are not in the corresponding type in the kernel. This provides
>> a way to be backward compatible. User space programs can use the same map
>> on a machine running an old kernel by clearing fields that do not exist in
>> the kernel.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/lib/bpf/libbpf.c | 30 +++++++++++++++++++++++-------
>>   1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index efab29b8935b..715879796046 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -1131,11 +1131,33 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                  __u32 kern_member_idx;
>>                  const char *mname;
>>
>> +               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
> 
> you don't need to move this up here, btf__resolve_size() can skip
> modifiers and typedefs just fine, so let's keep this one below and
> just pass member->type to btf__resolve_size()

Ok!

> 
>>                  mname = btf__name_by_offset(btf, member->name_off);
>> +               moff = member->offset / 8;
>> +               mdata = data + moff;
>> +               msize = btf__resolve_size(btf, mtype_id);
>> +               if (msize < 0) {
>> +                       pr_warn("struct_ops init_kern %s: fails to resolve the size of member %s\n",
> 
> s/fails/failed/
> 
>> +                               map->name, mname);
>> +                       return msize;
>> +               }
>> +
>>                  kern_member = find_member_by_name(kern_btf, kern_type, mname);
>>                  if (!kern_member) {
>>                          pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
>>                                  map->name, mname);
>> +
>> +                       /* Skip all zeros or null fields if they are not
>> +                        * presented in the kernel BTF.
>> +                        */
>> +                       if (btf_is_ptr(mtype)) {
>> +                               if (!st_ops->progs[i])
>> +                                       continue;
> 
> so, this is both unnecessary to check for NULL (libbpf_is_mem_zeroed
> will do it just fine for pointers as well), but it's also wrong
> because user could have set this program pointer through skeleton's
> shadow type, while here you won't yet see. So let's drop this
> btf_is_ptr() special case, and just do generic libbpf_is_mem_zeroed()
> check

You are right! I will fix it.

> 
>> +                       } else {
>> +                               if (libbpf_is_mem_zeroed(mdata, msize))
>> +                                       continue;
> 
> I think it's worth emitting informational message here, something like
> 
> pr_info("struct_ops %s: member %s not found in kernel, skipping it as
> it's set to zero\n", ...")
> 
> ?
> 
> and move that pr_warn("Cannot find member %s in kernel BTF") after
> this check, so we don't have scary-looking error-level message

Sure!

> 
>> +                       }
>> +
>>                          return -ENOTSUP;
>>                  }
>>
>> @@ -1147,13 +1169,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                          return -ENOTSUP;
>>                  }
>>
>> -               moff = member->offset / 8;
>>                  kern_moff = kern_member->offset / 8;
>> -
>> -               mdata = data + moff;
>>                  kern_mdata = kern_data + kern_moff;
>> -
>> -               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
>>                  kern_mtype = skip_mods_and_typedefs(kern_btf, kern_member->type,
>>                                                      &kern_mtype_id);
>>                  if (BTF_INFO_KIND(mtype->info) !=
>> @@ -1230,9 +1247,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                          continue;
>>                  }
>>
>> -               msize = btf__resolve_size(btf, mtype_id);
>>                  kern_msize = btf__resolve_size(kern_btf, kern_mtype_id);
>> -               if (msize < 0 || kern_msize < 0 || msize != kern_msize) {
>> +               if (kern_msize < 0 || msize != kern_msize) {
>>                          pr_warn("struct_ops init_kern %s: Error in size of member %s: %zd != %zd(kernel)\n",
>>                                  map->name, mname, (ssize_t)msize,
>>                                  (ssize_t)kern_msize);
>> --
>> 2.34.1
>>