Re: [PATCH bpf-next v2 2/2] selftests/bpf: test for malformed BPF_CORE_TYPE_ID_LOCAL relocation

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-08-21 at 21:29 -0700, Andrii Nakryiko wrote:

[...]

> > +       btf_fd = bpf_btf_load(&raw_btf, sizeof(raw_btf), &opts);
> > +       saved_errno = errno;
> > +       if (btf_fd < 0 || env.verbosity > VERBOSE_NORMAL) {
> > +               printf("-------- BTF load log start --------\n");
> > +               printf("%s", log);
> > +               printf("-------- BTF load log end ----------\n");
> > +       }
> > +       if (btf_fd < 0) {
> > +               PRINT_FAIL("bpf_btf_load() failed, errno=%d\n", saved_errno);
> > +               return;
> > +       }
> > +
> > +       memset(log, 0, sizeof(log));
> 
> generally speaking there is no need to memset log buffer (maybe just a
> first byte, to be safe)

Will change.

> on the other hand, just `union bpf_attr attr = {};` is breakage
> waiting to happen, I'd do memset(0) on that, we did run into problems
> with that before (I believe it was systemd)

Compilers optimize out 'smth = {}' where 'smth' escapes?
I mean, I will change it to memset(0), but the fact that you observed
such behaviour is disturbing beyond limit...

I already run into gcc vs clang behaviour differences for the first
iteration of this test where I had:

    union bpf_attr {
	.prog_type = ...
    };

clang did not zero out all members of the union, while gcc did.

> > +       attr.prog_btf_fd = btf_fd;
> > +       attr.prog_type = BPF_TRACE_RAW_TP;
> > +       attr.license = (__u64)"GPL";
> > +       attr.insns = (__u64)&insns;
> > +       attr.insn_cnt = sizeof(insns) / sizeof(*insns);
> > +       attr.log_buf = (__u64)log;
> > +       attr.log_size = sizeof(log);
> > +       attr.log_level = log_level;
> > +       attr.func_info = (__u64)funcs;
> > +       attr.func_info_cnt = sizeof(funcs) / sizeof(*funcs);
> > +       attr.func_info_rec_size = sizeof(*funcs);
> > +       attr.core_relos = (__u64)relos;
> > +       attr.core_relo_cnt = sizeof(relos) / sizeof(*relos);
> > +       attr.core_relo_rec_size = sizeof(*relos);
> 
> I was wondering for a bit why you didn't just use bpf_prog_load(), and
> it seems like it's due to core_relos fields?

Yes, it is in commit message :)

> I don't see why we can't extend the bpf_prog_load() API to allow to
> specify those. (would allow to avoid open-coding this whole bpf_attr
> business, but it's fine as is as well)

Maybe extend API as a followup?
The test won't change much, just options instead of bpf_attr.

[...]