linux-kernel - Re: [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes

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] [day] [month] [year] [list]

Message-ID: <CAP-5=fVvzthsR4G=aT8kTjR6c-xT=HAB9QhVdLR_Lo0LfMcTug@mail.gmail.com>
Date: Thu, 15 Aug 2024 18:44:19 -0700
From: Ian Rogers <irogers@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	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>, 
	Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>, 
	Kan Liang <kan.liang@...ux.intel.com>, Sun Haiyong <sunhaiyong@...ngson.cn>, 
	Yanteng Si <siyanteng@...ngson.cn>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes

On Mon, Aug 12, 2024 at 9:34 PM Ian Rogers <irogers@...gle.com> wrote:
>
> Events are 8-byte aligned in perf.data files, NAME_ALIGN is 64-bytes
> which is excessive given the alignment needs. Move the addition the
> sizeof so that the alignment considers that the rest of the event is
> 4-byte aligned.
>
> ```
> struct perf_record_header_build_id {
>         struct perf_event_header   header;               /*     0     8 */
>         pid_t                      pid;                  /*     8     4 */
>         union {
>                 __u8               build_id[24];         /*    12    24 */
>                 struct {
>                         __u8       data[20];             /*    12    20 */
>                         __u8       size;                 /*    32     1 */
>                         __u8       reserved1__;          /*    33     1 */
>                         __u16      reserved2__;          /*    34     2 */
>                 };                                       /*    12    24 */
>         };                                               /*    12    24 */
>         char                       filename[];           /*    36     0 */
>
>         /* size: 36, cachelines: 1, members: 4 */
>         /* last cacheline: 36 bytes */
> };
> ```
>
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
>  tools/perf/util/build-id.c         | 6 +++---
>  tools/perf/util/synthetic-events.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 83a1581e8cf1..8fea17d04468 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -309,8 +309,8 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
>         struct perf_record_header_build_id b;
>         size_t len;
>
> -       len = name_len + 1;
> -       len = PERF_ALIGN(len, NAME_ALIGN);
> +       len = sizeof(b) + name_len + 1;
> +       len = PERF_ALIGN(len, sizeof(u64));
>
>         memset(&b, 0, sizeof(b));
>         memcpy(&b.data, bid->data, bid->size);
> @@ -318,7 +318,7 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
>         misc |= PERF_RECORD_MISC_BUILD_ID_SIZE;
>         b.pid = pid;
>         b.header.misc = misc;
> -       b.header.size = sizeof(b) + len;
> +       b.header.size = len;
>
>         err = do_write(fd, &b, sizeof(b));
>         if (err < 0)
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 5498048f56ea..a52b85bcb6b6 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -2236,14 +2236,14 @@ int perf_event__synthesize_build_id(struct perf_tool *tool, struct dso *pos, u16
>
>         memset(&ev, 0, sizeof(ev));
>
> -       len = dso__long_name_len(pos) + 1;
> -       len = PERF_ALIGN(len, NAME_ALIGN);
> +       len = sizeof(ev.build_id) + dso__long_name_len(pos) + 1;
> +       len = PERF_ALIGN(len, sizeof(u64));
>         ev.build_id.size = min(dso__bid(pos)->size, sizeof(dso__bid(pos)->data));
>         memcpy(&ev.build_id.build_id, dso__bid(pos)->data, ev.build_id.size);
>         ev.build_id.header.type = PERF_RECORD_HEADER_BUILD_ID;
>         ev.build_id.header.misc = misc | PERF_RECORD_MISC_BUILD_ID_SIZE;
>         ev.build_id.pid = machine->pid;
> -       ev.build_id.header.size = sizeof(ev.build_id) + len;
> +       ev.build_id.header.size = len;
>         memcpy(&ev.build_id.filename, dso__long_name(pos), dso__long_name_len(pos));

So I think there is a bigger issue here and I'm working on a better
fix. The issue is that the synthesized build_id event doesn't have a
sample id inserted afterward (note, no adding of id_hdr_size above).
By over aligning the filename it is quite likely the sample id reading
is just reading zeros in particular for the time stamp. Not having a
sample id means the build_id events are unordered, this will break if
the data file has two dsos with the same filename but different build
ids - as happens when a file is replaced. The better fix is to 8 byte
align the event and insert a sample id.

Thanks,
Ian

>         return process(tool, &ev, NULL, machine);
> --
> 2.46.0.76.ge559c4bf1a-goog
>