[GIT PULL 0/1] perf/urgent fix

[GIT PULL 0/1] perf/urgent fix

[Arnaldo Carvalho de Melo]

Hi Ingo,

	Please consider applying,

- Arnaldo

The following changes since commit 021f5f12f2ab44874193c68fb19eea154493f83a:

  Merge tag 'perf-urgent-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/urgent (2015-03-01 17:41:42 +0100)

are available in the git repository at:


  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-urgent-for-mingo

for you to fetch changes up to 3995614d9b0320e10ce202836c8477e1bcf1a2d4:

  perf annotate: Fix fallback to unparsed disassembler line (2015-03-05 15:27:28 -0300)

----------------------------------------------------------------
perf/urgent fix:

- Fix SEGFAULT when freeing unparsed lock prefixed instructions in 'perf
  annotate' (Arnaldo Carvalho de Melo)

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

----------------------------------------------------------------
Arnaldo Carvalho de Melo (1):
      perf annotate: Fix fallback to unparsed disassembler line

 tools/perf/util/annotate.c | 2 ++
 1 file changed, 2 insertions(+)

[PATCH 1/1] perf annotate: Fix fallback to unparsed disassembler line

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When annotating source/disasm lines the perf tools parse the output of
objdump, trying to provide augmented output that allows navigating
jumps, calls, etc.

But when a line output by objdump can't be parsed the annotation code
falls back to just presenting the unparsed line.

When fixing a leak in the 0fb9f2aab738 commit ("perf annotate: Fix
memory leaks in LOCK handling") we failed to take that into account and
instead tried to free one of the data structures that should be freed
only when successfully allocated, oops, segfault.

There was a change in the way the objdump output for lock prefixed
instructions is formatted that lead the relevant parser to fail to grok
it.

At least RHEL7 works ok, but Fedora 20 segfaults.

Fix it by making the ins__delete() destructor work like the most basic
destructor: free().

Namely make it accept a NULL pointer and when handling it just do
nothing.

Further investigation is needed to figure out the nature of the objdump
output change so as to make the parser grok it.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Rabin Vincent <rabin@rab.in>
Link: http://lkml.kernel.org/n/tip-7wsy0zo292pif0yjoqpfryrz@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/annotate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 61bf9128e1f2..9d9db3b296dd 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -30,6 +30,8 @@ static int disasm_line__parse(char *line, char **namep, char **rawp);
 
 static void ins__delete(struct ins_operands *ops)
 {
+	if (ops == NULL)
+		return;
 	zfree(&ops->source.raw);
 	zfree(&ops->source.name);
 	zfree(&ops->target.raw);
-- 
1.9.3

Re: [PATCH 1/1] perf annotate: Fix fallback to unparsed disassembler line

[Rabin Vincent]

On Thu, Mar 05, 2015 at 04:03:20PM -0300, Arnaldo Carvalho de Melo wrote:
> When fixing a leak in the 0fb9f2aab738 commit ("perf annotate: Fix
> memory leaks in LOCK handling") we failed to take that into account and
> instead tried to free one of the data structures that should be freed
> only when successfully allocated, oops, segfault.
> 
> There was a change in the way the objdump output for lock prefixed
> instructions is formatted that lead the relevant parser to fail to grok
> it.
> 
> At least RHEL7 works ok, but Fedora 20 segfaults.
> 
> Fix it by making the ins__delete() destructor work like the most basic
> destructor: free().
> 
> Namely make it accept a NULL pointer and when handling it just do
> nothing.

While this patch is certainly sufficient both as a safety and as a quick
fix to the current problem (sorry about that), it seems that the real
issue is that lock__parse() returns success even on parsing failures?
The following patch fixes the issue for me without the above check.

(The removal of locked.ins == NULL in lock__scnprintf() is because it
 becomes unused after this.  On failure to parse the lock's inner
 instruction, the default printing in disasm__line__scnprintf() will be
 used, and that is identical to what this ins__raw_scnprintf() call
 does.)

8<------------
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 61bf912..51ab850 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -171,7 +171,7 @@ static int lock__parse(struct ins_operands *ops)
 
 	ops->locked.ops = zalloc(sizeof(*ops->locked.ops));
 	if (ops->locked.ops == NULL)
-		return 0;
+		return -1;
 
 	if (disasm_line__parse(ops->raw, &name, &ops->locked.ops->raw) < 0)
 		goto out_free_ops;
@@ -193,7 +193,7 @@ static int lock__parse(struct ins_operands *ops)
 
 out_free_ops:
 	zfree(&ops->locked.ops);
-	return 0;
+	return -1;
 }
 
 static int lock__scnprintf(struct ins *ins, char *bf, size_t size,
@@ -201,9 +201,6 @@ static int lock__scnprintf(struct ins *ins, char *bf, size_t size,
 {
 	int printed;
 
-	if (ops->locked.ins == NULL)
-		return ins__raw_scnprintf(ins, bf, size, ops);
-
 	printed = scnprintf(bf, size, "%-6.6s ", ins->name);
 	return printed + ins__scnprintf(ops->locked.ins, bf + printed,
 					size - printed, ops->locked.ops);

Re: [GIT PULL 0/1] perf/urgent fix

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Hi Ingo,
> 
> 	Please consider applying,
> 
> - Arnaldo
> 
> The following changes since commit 021f5f12f2ab44874193c68fb19eea154493f83a:
> 
>   Merge tag 'perf-urgent-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/urgent (2015-03-01 17:41:42 +0100)
> 
> are available in the git repository at:
> 
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-urgent-for-mingo
> 
> for you to fetch changes up to 3995614d9b0320e10ce202836c8477e1bcf1a2d4:
> 
>   perf annotate: Fix fallback to unparsed disassembler line (2015-03-05 15:27:28 -0300)
> 
> ----------------------------------------------------------------
> perf/urgent fix:
> 
> - Fix SEGFAULT when freeing unparsed lock prefixed instructions in 'perf
>   annotate' (Arnaldo Carvalho de Melo)
> 
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> ----------------------------------------------------------------
> Arnaldo Carvalho de Melo (1):
>       perf annotate: Fix fallback to unparsed disassembler line
> 
>  tools/perf/util/annotate.c | 2 ++
>  1 file changed, 2 insertions(+)

Pulled, thanks Arnaldo!

	Ingo

Re: [PATCH 1/1] perf annotate: Fix fallback to unparsed disassembler line

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> [...]
> 
> Further investigation is needed to figure out the nature of the 
> objdump output change so as to make the parser grok it.

Btw., maybe someone finds this interesting: we could also (re-)use the 
in-kernel disassembler (on x86 and any other architecture that might 
have one), which used by live patching facilities (kprobes et al).

See arch/x86/lib/insn.c.

The 'visualization' bit is missing entirely: but it does a lot of the 
hard work of knowing about the instruction format: it knows about 
essentially all x86 instructions and is able to determine instruction 
boundaries, and can decode immediate constants.

Using this in tools/perf/ would have the added advantage that we could 
then use the dissasembly in kernel oops output (nice feature!) - plus 
tooling folks would help us fix and extend the kernel's disassembler! 
;-)

Thanks,

	Ingo

Re: [PATCH 1/1] perf annotate: Fix fallback to unparsed disassembler line

[Arnaldo Carvalho de Melo]

Em Mon, Mar 09, 2015 at 12:09:58PM +0100, Ingo Molnar escreveu:
> 
> * Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > [...]
> > 
> > Further investigation is needed to figure out the nature of the 
> > objdump output change so as to make the parser grok it.
> 
> Btw., maybe someone finds this interesting: we could also (re-)use the 
> in-kernel disassembler (on x86 and any other architecture that might 
> have one), which used by live patching facilities (kprobes et al).
> 
> See arch/x86/lib/insn.c.
> 
> The 'visualization' bit is missing entirely: but it does a lot of the 
> hard work of knowing about the instruction format: it knows about 
> essentially all x86 instructions and is able to determine instruction 
> boundaries, and can decode immediate constants.
> 
> Using this in tools/perf/ would have the added advantage that we could 
> then use the dissasembly in kernel oops output (nice feature!) - plus 
> tooling folks would help us fix and extend the kernel's disassembler! 
> ;-)

I think it should provide a good synergy, yes, IIRC this was discussed
already at some point even.

- Arnaldo