On Wed, Aug 21, 2024 at 9:39 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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...
compiler is not obligated to zero out padding in the struct/union, and
kernel is pretty strict about that, that's the issue. memset(0)
guarantees all the bytes are set to zero, not just those that belong
to fields
>
> 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 :)
>
ain't nobody got time for reading commit messages ;)
> > 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.
yep, follow up is good, thanks
>
> [...]
>
