| 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
| ||
|
Message-ID: <aIfGwdcRZ_gwyhiC@google.com>
Date: Mon, 28 Jul 2025 11:51:45 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Kan Liang <kan.liang@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v4 8/9] perf annotate: Hide data-type for stack operation
and canary
On Fri, Jul 25, 2025 at 05:44:33PM -0700, Ian Rogers wrote:
> On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > It's mostly unnecessary to print when it has no actual type information
> > like in the stack operations and canary. Let's have them if -v option
> > is given.
> >
> > Before:
> > $ perf annotate --code-with-type
> > ...
> > : 0 0xd640 <_dl_relocate_object>:
> > 0.00 : 0: endbr64
> > 0.00 : 4: pushq %rbp # data-type: (stack operation)
> > 0.00 : 5: movq %rsp, %rbp
> > 0.00 : 8: pushq %r15 # data-type: (stack operation)
> > 0.00 : a: pushq %r14 # data-type: (stack operation)
> > 0.00 : c: pushq %r13 # data-type: (stack operation)
> > 0.00 : e: pushq %r12 # data-type: (stack operation)
> > 0.00 : 10: pushq %rbx # data-type: (stack operation)
>
> I believe the intent in the dwarf is to say where the caller's callee
> saves are, but the stack slots should just be saved and restored and
> won't be used for anything interesting, perhaps for exception
> handling. An annotation like:
> # caller's RBX at stack frame offset -48
> could perhaps be useful.
The main purpose of the annotation of the stack operation is to sync
with the output of data type profiling. As it can report lots of
accesses were from the stack operation, this can answer where they are.
>
> > 0.00 : 11: subq $0xf8, %rsp
> > ...
> > 0.00 : d4: testl %eax, %eax
> > 0.00 : d6: jne 0xf424
> > 0.00 : dc: movq 0xf0(%r14), %rbx # data-type: struct link_map +0xf0
> > 0.00 : e3: testq %rbx, %rbx
> > 0.00 : e6: jne 0xf2dd
> > 0.00 : ec: cmpq $0, 0xf8(%r14) # data-type: struct link_map +0xf8
> > ...
> >
> > After:
> > : 0 0xd640 <_dl_relocate_object>:
> > 0.00 : 0: endbr64
> > 0.00 : 4: pushq %rbp
> > 0.00 : 5: movq %rsp, %rbp
> > 0.00 : 8: pushq %r15
> > 0.00 : a: pushq %r14
> > 0.00 : c: pushq %r13
> > 0.00 : e: pushq %r12
> > 0.00 : 10: pushq %rbx
> > 0.00 : 11: subq $0xf8, %rsp
> > ...
> > 0.00 : d4: testl %eax, %eax
> > 0.00 : d6: jne 0xf424
> > 0.00 : dc: movq 0xf0(%r14), %rbx # data-type: struct link_map +0xf0
> > 0.00 : e3: testq %rbx, %rbx
> > 0.00 : e6: jne 0xf2dd
> > 0.00 : ec: cmpq $0, 0xf8(%r14) # data-type: struct link_map +0xf8
> > ...
> >
> > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> > ---
> > tools/perf/util/annotate.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 06ddc7a9f58722a4..6fc07971631ac8a3 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -765,6 +765,17 @@ __hist_entry__get_data_type(struct hist_entry *he, struct arch *arch,
> > struct debuginfo *dbg, struct disasm_line *dl,
> > int *type_offset);
> >
> > +static bool needs_type_info(struct annotated_data_type *data_type)
> > +{
> > + if (data_type == NULL || data_type == NO_TYPE)
> > + return false;
> > +
> > + if (verbose)
> > + return true;
>
> lgtm given the many overloaded meanings of verbose.
:-)
>
> Reviewed-by: Ian Rogers <irogers@...gle.com>
Thanks for your careful review!
Namhyung
> > +
> > + return (data_type != &stackop_type) && (data_type != &canary_type);
> > +}
> > +
> > static int
> > annotation_line__print(struct annotation_line *al, struct annotation_print_data *apd,
> > struct annotation_options *opts, int printed,
> > @@ -844,7 +855,7 @@ annotation_line__print(struct annotation_line *al, struct annotation_print_data
> >
> > data_type = __hist_entry__get_data_type(apd->he, apd->arch,
> > apd->dbg, dl, &offset);
> > - if (data_type && data_type != NO_TYPE) {
> > + if (needs_type_info(data_type)) {
> > char buf[4096];
> >
> > printf("\t\t# data-type: %s",
> > @@ -2138,7 +2149,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> > apd->dbg,
> > disasm_line(al),
> > &offset);
> > - if (data_type && data_type != NO_TYPE) {
> > + if (needs_type_info(data_type)) {
> > char member[256];
> >
> > printed = scnprintf(bf, sizeof(bf),
> > --
> > 2.50.1
> >
Powered by blists - more mailing lists