netdev - Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <552493f9-179d-9a02-d5e7-5f78b56a18c3@isovalent.com>
Date:   Fri, 23 Jul 2021 18:44:58 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as
 btf__load_from_kernel_by_id()

2021-07-23 10:18 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> On Fri, Jul 23, 2021 at 9:13 AM Quentin Monnet <quentin@...valent.com> wrote:
>>
>> 2021-07-23 08:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
>>> On Fri, Jul 23, 2021 at 2:31 AM Quentin Monnet <quentin@...valent.com> wrote:
>>>>
>>>> 2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
>>>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@...valent.com> wrote:
>>>>>>
>>>>>> Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
>>>>>> better indicate what the function does. Change the new function so that,
>>>>>> instead of requiring a pointer to the pointer to update and returning
>>>>>> with an error code, it takes a single argument (the id of the BTF
>>>>>> object) and returns the corresponding pointer. This is more in line with
>>>>>> the existing constructors.
>>>>>>
>>>>>> The other tools calling the deprecated btf__get_from_id() function will
>>>>>> be updated in a future commit.
>>>>>>
>>>>>> References:
>>>>>>
>>>>>> - https://github.com/libbpf/libbpf/issues/278
>>>>>> - https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
>>>>>>
>>
>>>>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>>>>> index 7e0de560490e..6654bdee7ad7 100644
>>>>>> --- a/tools/lib/bpf/btf.c
>>>>>> +++ b/tools/lib/bpf/btf.c
>>>>>> @@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
>>>>>>         return btf;
>>>>>>  }
>>>>>>
>>>>>> +struct btf *btf__load_from_kernel_by_id(__u32 id)
>>>>>> +{
>>>>>> +       struct btf *btf;
>>>>>> +       int btf_fd;
>>>>>> +
>>>>>> +       btf_fd = bpf_btf_get_fd_by_id(id);
>>>>>> +       if (btf_fd < 0)
>>>>>> +               return ERR_PTR(-errno);
>>>>>
>>>>> please use libbpf_err_ptr() for consistency, see
>>>>> bpf_object__open_mem() for an example
>>>>
>>>> I can do that, but I'll need to uncouple btf__get_from_id() from the new
>>>> function. If it calls btf__load_from_kernel_by_id() and
>>>> LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.
>>>
>>> No it won't, if libbpf_get_error() is used right after the API call.
>>
>> But we cannot be sure that users currently call libbpf_get_error() after
>> btf__get_from_id()? I'm fine if we assume they do (users currently
>> selecting the CLEAN_PTRS are probably savvy enough to call it I guess),
>> I'll update as you suggest.
> 
> I think you are still confused.

OK, I think I was.
I'm not arguing against the contract, but I thought your suggestion
would introduce a change in btf__get_from_id()'s behaviour. Reading
again through the code and your explanations, there should be no change
indeed, I just misunderstood in the first place. Apologies, and thanks
for your patience :). I'll prepare v3 soon.

Quentin