Re: [PATCH bpf-next] selftests/bpf: increase verifier log limit in veristat

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 21/10/2024 21:14, Andrii Nakryiko wrote:
> On Mon, Oct 21, 2024 at 9:44 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>> On Mon, Oct 21, 2024 at 03:16:16PM +0100, Mykyta Yatsenko wrote:
>>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>>
>>> The current default buffer size of 16MB allocated by veristat is no
>>> longer sufficient to hold the verifier logs of some production BPF
>>> programs. To address this issue, we need to increase the verifier log
>>> limit.
>>> Commit 7a9f5c65abcc ("bpf: increase verifier log limit") has already
>>> increased the supported buffer size by the kernel, but veristat users
>>> need to explicitly pass a log size argument to use the bigger log.
>>>
>>> This patch adds a function to detect the maximum verifier log size
>>> supported by the kernel and uses that by default in veristat.
>>> This ensures that veristat can handle larger verifier logs without
>>> requiring users to manually specify the log size.
>>>
>>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>>> ---
>>>   tools/testing/selftests/bpf/veristat.c | 40 +++++++++++++++++++++++++-
>>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
>>> index c8efd44590d9..1d0708839f4b 100644
>>> --- a/tools/testing/selftests/bpf/veristat.c
>>> +++ b/tools/testing/selftests/bpf/veristat.c
>>> @@ -16,10 +16,12 @@
>>>   #include <sys/stat.h>
>>>   #include <bpf/libbpf.h>
>>>   #include <bpf/btf.h>
>>> +#include <bpf/bpf.h>
>>>   #include <libelf.h>
>>>   #include <gelf.h>
>>>   #include <float.h>
>>>   #include <math.h>
>>> +#include <linux/filter.h>
> this is kernel-internal header, which will be a problem for Github mirror, so...
>
>>>   #ifndef ARRAY_SIZE
>>>   #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>>> @@ -1109,6 +1111,42 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch
>>>        return;
>>>   }
>>>
>>> +static int max_verifier_log_size(void)
>>> +{
>>> +     const int big_log_size = UINT_MAX >> 2;
>>> +     const int small_log_size = UINT_MAX >> 8;
> nit: MAKE_ALL_CAPS, given they are fixed constants
>
>>> +     struct bpf_insn insns[] = {
>>> +             BPF_MOV64_IMM(BPF_REG_0, 0),
>>> +             BPF_EXIT_INSN(),
> ... let's instead either define these macro locally or just hard-code
> bpf_insn structs as is (thankfully we need just two)
>
>>> +     };
>>> +     int ret, insn_cnt = ARRAY_SIZE(insns);
>>> +     char *log_buf;
>>> +     static int log_size;
>>> +
>>> +     if (log_size != 0)
>>> +             return log_size;
>>> +
>>> +     log_size = small_log_size;
>>> +     log_buf = malloc(big_log_size);
> we don't really need to allocate anything. We can pass (void*)-1 as
> log_buf (invalid pointer), set size to UINT_MAX >> 8, log_level = 4.
> If the kernel doesn't support big log_size, we'll get -EINVAL. If it
> does, we'll get -EFAULT when the verifier will try to write something
> to the buffer. No allocation.
>
> pw-bot: cr
>
>> IIUC this would try to use 1GB by default? seems to agresive.. could we perhaps
>> do that gradually and double the size on each failed load attempt?
> The idea is that verifier will only page in as many pages as there is
> an actual log content (which normally would be much smaller than a
> full 1GB). Doing gradual size increase is actually pretty annoying in
> terms of how the code and logic is structured. So I think this
> approach is fine, overall.
>
>> jirka
>>
>>
>>> +
>>> +     if (!log_buf)
>>> +             return log_size;
>>> +
>>> +     LIBBPF_OPTS(bpf_prog_load_opts, opts,
>>> +                 .log_buf = log_buf,
>>> +                 .log_size = big_log_size,
>>> +                 .log_level = 2
> no need for log_level = 2, just use 4, we don't need to fill out the
> buffer, we need a verifier to check parameters.
>
>>> +     );
> LIBBPF_OPTS() macro define a variable, so please move it to the
> variable declaration block above.
>
>>> +     ret = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns, insn_cnt, &opts);
> nit: let's use TRACEPOINT instead, we had some problems with
> SOCKET_FILTER on some old Red Hat distro due to how they did selective
> backport, so best to avoid it, if possible.
>
>>> +     free(log_buf);
>>> +
>>> +     if (ret > 0) {
>>> +             log_size = big_log_size;
>>> +             close(ret);
>>> +     }
>>> +     return log_size;
>>> +}
>>> +
>>>   static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog)
>>>   {
>>>        const char *base_filename = basename(strdupa(filename));
>>> @@ -1132,7 +1170,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>>>        memset(stats, 0, sizeof(*stats));
>>>
>>>        if (env.verbose || env.top_src_lines > 0) {
>>> -             buf_sz = env.log_size ? env.log_size : 16 * 1024 * 1024;
>>> +             buf_sz = env.log_size ? env.log_size : max_verifier_log_size();
>>>                buf = malloc(buf_sz);
>>>                if (!buf)
>>>                        return -ENOMEM;
>>> --
>>> 2.47.0
>>>
>>>
Thanks for taking a look, I'll apply your suggestions for v2.