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

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

On 21/10/2024 17:44, Jiri Olsa 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>
>>   
>>   #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;
>> +	struct bpf_insn insns[] = {
>> +		BPF_MOV64_IMM(BPF_REG_0, 0),
>> +		BPF_EXIT_INSN(),
>> +	};
>> +	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);
> 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?
>
> jirka
Yes, this allocates 1GB by default, I expect this is not a big of a 
problem if verifier
does not touch all that memory.
I tried doing gradual allocation initially, but that requires more 
significant rework
of the code.
Thanks for looking into this patch!
>
>> +
>> +	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
>> +	);
>> +	ret = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns, insn_cnt, &opts);
>> +	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
>>
>>