linux-kernel - Re: [PATCH] perf build: Restore {0} initializer since GCC-15

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fUp492RATRhc-OmikK7knO+PUWAt6Z-60Cax0QOJPrFgg@mail.gmail.com>
Date: Wed, 19 Mar 2025 08:26:10 -0700
From: Ian Rogers <irogers@...gle.com>
To: Leo Yan <leo.yan@....com>
Cc: James Clark <james.clark@...aro.org>, Arnaldo Carvalho de Melo <acme@...nel.org>, 
	Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, "Liang, Kan" <kan.liang@...ux.intel.com>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf build: Restore {0} initializer since GCC-15

On Wed, Mar 19, 2025 at 6:31 AM Leo Yan <leo.yan@....com> wrote:
>
> On Wed, Mar 19, 2025 at 11:19:38AM +0000, James Clark wrote:
> >
> > On 19/03/2025 11:04 am, Leo Yan wrote:
> > > GCC-15 release claims [1]:
> > >
> > >   {0} initializer in C or C++ for unions no longer guarantees clearing
> > >   of the whole union (except for static storage duration initialization),
> > >   it just initializes the first union member to zero. If initialization
> > >   of the whole union including padding bits is desirable, use {} (valid
> > >   in C23 or C++) or use -fzero-init-padding-bits=unions option to
> > >   restore old GCC behavior.
> > >
> > > This new behaviour might cause stale and unexpected data we defined in
> > > Perf.  Add the -fzero-init-padding-bits=unions option for entirely
> > > zeroing union structures.
> > >
> >
> > Do we need this? I don't see any unions initialized in that way. In fact
> > there is only one struct initialized with {0}, the other handful are char*s
> > but I don't think either are affected.
>
> Though I did not found a straightforward case in Perf for initializing
> union with "{0}", the result I got:
>
> $ git grep -E "\{ *0 *\}" tools/perf/
> tools/perf/arch/x86/tests/insn-x86.c:   {{0}, 0, 0, NULL, NULL, NULL},
> tools/perf/arch/x86/tests/insn-x86.c:   {{0}, 0, 0, NULL, NULL, NULL},
> tools/perf/arch/x86/tests/intel-pt-test.c:      {1, {0}, 0, {INTEL_PT_PAD, 0, 0}, 0, 1 },
> tools/perf/arch/x86/tests/intel-pt-test.c:      {0, {0}, 0, {0, 0, 0}, 0, 0 },
> tools/perf/arch/x86/util/perf_regs.c:   char new_reg[SDT_REG_NAME_SIZE] = {0};
> tools/perf/arch/x86/util/perf_regs.c:   char prefix[3] = {0};
> tools/perf/builtin-kwork.c:             .nr_skipped_events   = { 0 },
> tools/perf/builtin-record.c:    u8 pad[8] = {0};
> tools/perf/python/twatch.py:                    print("cpu: {0}, pid: {1}, tid: {2} {3}".format(event.sample_cpu,
> tools/perf/tests/code-reading.c:        unsigned char buf1[BUFSZ] = {0};
> tools/perf/tests/code-reading.c:        unsigned char buf2[BUFSZ] = {0};
> tools/perf/util/bpf_counter.c:  struct bpf_map_info map_info = {0};
> tools/perf/util/bpf_kwork.c:    char name[MAX_KWORKNAME] = { 0 };
> tools/perf/util/bpf_skel/bperf_follower.bpf.c:  struct bperf_filter_value child_fval = { 0 };
> tools/perf/util/lzma.c: char buf[6] = { 0 };
> tools/perf/util/mem-events.c:bool perf_mem_record[PERF_MEM_EVENTS__MAX] = { 0 };
> tools/perf/util/mem-events.c:   char hit_miss[5] = {0};
> tools/perf/util/trace-event-scripting.c:        char xs[16] = {0};
> tools/perf/util/zlib.c: char buf[2] = { 0 };
>
> We can see the bpf structures (bpf_map_info/bperf_filter_value) are
> initialized with {0}.  For a more complex case, {0} is used for
> initialize a specific field in a structure (see the results in
> insn-x86.c and intel-pt-test.c).
>
> > Adding options that allow people to add more non standard code doesn't feel
> > very portable or in the spirit of doing it the right way. Maybe there's an
> > argument that it guards against future mistakes, but it's not mentioned in
> > the commit message.
>
> I think Linux perf shares the same understanding with "we do expect
> initializers that always initialize the whole variable fully" (quote
> in [1]).  Furthermore, the reply mentioned:
>
>  The exact same problem happens with "{ 0 }" as happens with "{ }".
>  The bug is literally that some versions of clang seem to implement
>  BOTH of these as "initialize the first member of the union", which
>  then means that if the first member isn't the largest member, the
>  rest of the union is entirely undefined.
>
> So I think it is reasonable to imposes a compiler option to make
> compiler's behavouir consistent.

We have encountered this problem, here is a fix for a case:
https://lore.kernel.org/r/20241119230033.115369-1-irogers@google.com
It would be nice if rather than -fzero-init-padding-bits=unions there
were some kind of warning flag we could enable, or worse use a tool
like clang-tidy to identify these problems. In the linked change the
problem was identified with -fsanitize=undefined but IIRC perf didn't
quit with a sanitizer warning message, just things broke and needed
fixing.

Thanks,
Ian

> Thanks,
> Leo
>
> [1] https://www.spinics.net/lists/netdev/msg1007244.html