Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs

[Daniel Borkmann <daniel@iogearbox.net>]

On 1/12/21 7:41 AM, Andrii Nakryiko wrote:
> On Mon, Jan 11, 2021 at 5:16 PM Yonghong Song <yhs@fb.com> wrote:
>> On 1/11/21 12:51 PM, Andrii Nakryiko wrote:
>>> On Mon, Jan 11, 2021 at 10:13 AM Yonghong Song <yhs@fb.com> wrote:
>>>> On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
>>>>> Empty BTFs do come up (e.g., simple kernel modules with no new types and
>>>>> strings, compared to the vmlinux BTF) and there is nothing technically wrong
>>>>> with them. So remove unnecessary check preventing loading empty BTFs.
>>>>>
>>>>> Reported-by: Christopher William Snowhill <chris@kode54.net>
>>>>> Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")

Fixed up Fixes tag ^^^^^ while applying. ;-)

>>>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>>>> ---
>>>>>     tools/lib/bpf/btf.c | 5 -----
>>>>>     1 file changed, 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>>>> index 3c3f2bc6c652..9970a288dda5 100644
>>>>> --- a/tools/lib/bpf/btf.c
>>>>> +++ b/tools/lib/bpf/btf.c
>>>>> @@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
>>>>>         }
>>>>>
>>>>>         meta_left = btf->raw_size - sizeof(*hdr);
>>>>> -     if (!meta_left) {
>>>>> -             pr_debug("BTF has no data\n");
>>>>> -             return -EINVAL;
>>>>> -     }
>>>>
>>>> Previous kernel patch allows empty btf only if that btf is module (not
>>>> base/vmlinux) btf. Here it seems we allow any empty non-module btf to be
>>>> loaded into the kernel. In such cases, loading may fail? Maybe we should
>>>> detect such cases in libbpf and error out instead of going to kernel and
>>>> get error back?
>>>
>>> I did this consciously. Kernel is more strict, because there is no
>>> reasonable case when vmlinux BTF or BPF program's BTF can be empty (at
>>> least not that now we have FUNCs in BTF). But allowing libbpf to load
>>> empty BTF generically is helpful for bpftool, as one example, for
>>> inspection. If you do `bpftool btf dump` on empty BTF, it will just
>>> print nothing and you'll know that it's a valid (from BTF header
>>> perspective) BTF, just doesn't have any types (besides VOID). If we
>>> don't allow it, then we'll just get an error and then you'll have to
>>> do painful hex dumping and decoding to see what's wrong.
>>
>> It is totally okay to allow empty btf in libbpf. I just want to check
>> if this btf is going to be loaded into the kernel, right before it is
>> loading whether libbpf could check whether it is a non-module empty btf
>> or not, if it is, do not go to kernel.
> 
> Ok, I see what you are proposing. We can do that, but it's definitely
> separate from these bug fixes. But, to be honest, I wouldn't bother
> because libbpf will return BTF verification log with a very readable
> "No data" message in it.

Right, seems okay to me for this particular case given the user will be
able to make some sense of it from the log.

Thanks,
Daniel