Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Martin KaFai Lau <martin.lau@linux.dev>]

On 2/28/24 4:30 PM, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 4:25 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 2/28/24 3:55 PM, Andrii Nakryiko wrote:
>>> On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>
>>>> On 2/27/24 12:45 PM, Eduard Zingerman wrote:
>>>>> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
>>>>> state for referenced programs.
>>>>>
>>>>> E.g. for the BPF code below:
>>>>>
>>>>>        SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
>>>>>        SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }
>>>>>
>>>>>        SEC(".struct_ops.link")
>>>>>        struct test_ops___v1 A = {
>>>>>            .foo = (void *)foo
>>>>>        };
>>>>>
>>>>>        SEC(".struct_ops.link")
>>>>>        struct test_ops___v2 B = {
>>>>>            .foo = (void *)foo,
>>>>>            .bar = (void *)bar,
>>>>>        };
>>>>>
>>>>> And the following libbpf API calls:
>>>>>
>>>>>        bpf_map__set_autocreate(skel->maps.A, true);
>>>>>        bpf_map__set_autocreate(skel->maps.B, false);
>>>>>
>>>>> The autoload would be enabled for program 'foo' and disabled for
>>>>> program 'bar'.
>>>>>
>>>>> Do not apply such toggling if program autoload state is set by a call
>>>>> to bpf_program__set_autoload().
>>>>>
>>>>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
>>>>> ---
>>>>>     tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
>>>>>     1 file changed, 34 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>>> index b39d3f2898a1..1ea3046724f8 100644
>>>>> --- a/tools/lib/bpf/libbpf.c
>>>>> +++ b/tools/lib/bpf/libbpf.c
>>>>> @@ -446,13 +446,18 @@ struct bpf_program {
>>>>>         struct bpf_object *obj;
>>>>>
>>>>>         int fd;
>>>>> -     bool autoload;
>>>>> +     bool autoload:1;
>>>>> +     bool autoload_user_set:1;
>>>>>         bool autoattach;
>>>>>         bool sym_global;
>>>>>         bool mark_btf_static;
>>>>>         enum bpf_prog_type type;
>>>>>         enum bpf_attach_type expected_attach_type;
>>>>>         int exception_cb_idx;
>>>>> +     /* total number of struct_ops maps with autocreate == true
>>>>> +      * that reference this program
>>>>> +      */
>>>>> +     __u32 struct_ops_refs;
>>>>
>>>> Instead of adding struct_ops_refs and autoload_user_set,
>>>>
>>>> for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking
>>>> prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at
>>>> load time and is only set if it is used by at least one autocreate map, if I
>>>> read patch 2 & 3 correctly.
>>>>
>>>> Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used
>>>> by one struct_ops map with autocreate == true.
>>>>
>>>> If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be
>>>> loaded even the autoload is set. If bpf prog is used in a struct_ops map and its
>>>> autoload is set to false, the struct_ops map will be in broken state. Thus,
>>>
>>> We can easily detect this condition and report meaningful error.
>>>
>>>> prog->autoload does not fit very well with struct_ops prog and may as well
>>>> depend on whether the struct_ops prog is used by a struct_ops map alone?
>>>
>>> I think it's probably fine from a usability standpoint to disable
>>> loading the BPF program if its struct_ops map was explicitly set to
>>> not auto-create. It's a bit of deviation from other program types, but
>>> in practice this logic will make it easier for users.
>>>
>>> One question I have, though, is whether we should report as an error a
>>> stand-alone struct_ops BPF program that is not used from any
>>> struct_ops map? Or should we load it nevertheless? Or should we
>>> silently not load it?
>>
>> I think the libbpf could report an error if the prog is not used in any
>> struct_ops map at the source code level, not sure if it is useful.
>>
>> However, it probably should not report error if that struct_ops map (where the
>> prog is resided) does not have autocreate set to true.
>>
>> If a BPF program is not used in any struct_ops map, it cannot be loaded anyway
>> because the prog->attach_btf_id is not set. If libbpf tries to load the prog,
>> the kernel will reject it also. I think it may be a question on whether it is
>> the user intention of not loading the prog if the prog is not used in any
>> struct_ops map. I tend to think it is the user intention of not loading it in
>> this case.
>>
>> SEC("struct_ops/test1")
>> int BPF_PROG(test1) { ... }
>>
>> SEC("struct_ops/test2")
>> int BPF_PROG(test2) { ... }
>>
>> SEC("?.struct_ops.link") struct some_ops___v1 a = { .test1 = test1 }
>> SEC("?.struct_ops.link") struct some_ops___v2 b = { .test1 = test1,
>>                                                     .test2 = test2, }
>>
>> In the above, the userspace may try to load with a newer some_ops___v2 first,
>> failed and then try a lower version some_ops___v1 and then succeeded. The test2
>> prog will not be used and not expected to be loaded.
>>
> 
> Yes, it's all sane in the above example. But imagine a stand-alone
> struct_ops program with no SEC(".struct_ops") at all:
> 
> 
> SEC("struct_ops/test1")
> int BPF_PROG(test1) { ... }
> 
> /* nothing else */
> 
> Currently this will fail, right?
> 
> And with your proposal it will succeed without actually even
> attempting to load the BPF program. Or am I misunderstanding?

Yep, currently it should fail.

Agree that we need to distinguish this case and prog->attach_btf_id is not 
enough. This probably can be tracked in collect_st_ops_relos at the open phase.