linux-kernel - Re: [PATCH v3 3/5] perf synthetic-events: Ensure features are aligned

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: <Z2OvM2YkFd1zoDot@google.com>
Date: Wed, 18 Dec 2024 21:29:23 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...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>,
	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 v3 3/5] perf synthetic-events: Ensure features are aligned

On Wed, Dec 18, 2024 at 05:29:01PM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 5:08 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > On Mon, Dec 16, 2024 at 03:24:57PM -0800, Ian Rogers wrote:
> > > Features like hostname have arbitrary size and break the assumed
> > > 8-byte alignment of perf events. Pad all feature events until 8-byte
> > > alignment is restored.
> >
> > But it seems write_hostname() and others use do_write_string() which
> > handles the padding already.
> 
> Well it does something :-) (I agree my description isn't 100%)
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.c?h=perf-tools-next#n188
> ```
> static int do_write_string(struct feat_fd *ff, const char *str)
> {
> u32 len, olen;
> int ret;
> 
> olen = strlen(str) + 1;
> len = PERF_ALIGN(olen, NAME_ALIGN);
> 
> /* write len, incl. \0 */
> ret = do_write(ff, &len, sizeof(len));
> if (ret < 0)
> return ret;
> 
> return write_padded(ff, str, olen, len);
> }
> ```
> but NAME_ALIGN is 64:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.h?h=perf-tools-next#n187
> whereas the data file is expecting things aligned to sizeof(u64) which
> is 8-bytes. We're potentially writing out 7 u64s worth of 0s for no
> alignment gain. It should be safe, given I don't see other use of this
> alignment value, to switch the string alignment to being sizeof(u64)
> but following this change we could also just get rid of the padding
> knowing it will be fixed in the caller.

Probably, but it seems simpler just to change NAME_ALIGN to 8. :)

Thanks,
Namhyung