[SRU][Artful][PATCH 0/2] Fixes for LP:1769027

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Artful][PATCH 0/2] Fixes for LP:1769027

Joseph Salisbury-3
== SRU Justification ==
This SRU request is for two commits, that are needed for two bug reports.  The
first bug(1767204) is marked as a duplicate of the bug used for this SRU
request.  The commit(3d8bba9535ac) required to fix the first bug introduced the second
bug.  The second bug is then fixed buy commit cd8dd032f61a.

The first issue is perf crashes due to swapped xyarray function signatures and is
fixed by commit 3d8bba9535ac.

The second issue is a crash due to "refcount_inc assertion failed".
This second bug is introduced by picking commit 3d8bba9535ac without
picking commit cd8dd032f61a first.


== Fixes ==
cd8dd032f61a ("perf cgroup: Fix refcount usage")
3d8bba9535ac ("perf xyarray: Fix wrong processing when closing evsel fd")

== Regression Potential ==
Low.  Limited to perf tool.

== Test Case ==
A test kernel was built with these patches and tested by the original bug reporter.
The bug reporter states the test kernel resolved the bug.

Arnaldo Carvalho de Melo (1):
  perf cgroup: Fix refcount usage

Jin Yao (1):
  perf xyarray: Fix wrong processing when closing evsel fd

 tools/perf/util/cgroup.c  | 8 +++++---
 tools/perf/util/xyarray.h | 4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Artful][PATCH 1/2] perf cgroup: Fix refcount usage

Joseph Salisbury-3
From: Arnaldo Carvalho de Melo <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1769027

When converting from atomic_t to refcount_t we didn't follow the usual
step of initializing it to one before taking any new reference, which
trips over checking if taking a reference for a freed refcount_t, fix
it.

Brendan's report:

 ---
It's 4.12-rc7, with node v4.4.1. I'm building 4.13-rc1 now, as I hit
what I think is another unrelated perf bug and I'm starting to wonder
what else is broken on that version:

(root) /mnt/src/linux-4.12-rc7/tools/perf # ./perf record -F 99 -a -e
cpu-clock --cgroup=docker/f9e9d5df065b14646e8a11edc837a13877fd90c171137b2ba3feb67a0201cb65
-g
perf: /mnt/src/linux-4.12-rc7/tools/include/linux/refcount.h:108:
refcount_inc: Assertion `!(!refcount_inc_not_zero(r))' failed.
Aborted

that used to work...
 ---

Testing it:

Before:

  # perf stat -e cycles -C 0 --cgroup /
  perf: /home/acme/git/linux/tools/include/linux/refcount.h:108: refcount_inc: Assertion `!(!refcount_inc_not_zero(r))' failed.
  Aborted (core dumped)
  #

After:

  # perf stat -e cycles -C 0 --cgroup /
^C
  Performance counter stats for 'CPU(s) 0':

       132,081,393      cycles                    /

       2.492942763 seconds time elapsed

  #

Reported-by: Brendan Gregg <[hidden email]>
Acked-by: Elena Reshetova <[hidden email]>
Cc: Alexander Shishkin <[hidden email]>
Cc: David Ahern <[hidden email]>
Cc: David Carrillo-Cisneros <[hidden email]>
Cc: Kees Kook <[hidden email]>
Cc: Krister Johansen <[hidden email]>
Cc: Paul Turner <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Stephane Eranian <[hidden email]>
Cc: Sudeep Holla <[hidden email]>
Cc: Thomas-Mich Richter <[hidden email]>
Cc: Wang Nan <[hidden email]>
Fixes: 79c5fe6db8c7 ("perf cgroup: Convert cgroup_sel.refcnt from atomic_t to refcount_t")
Link: http://lkml.kernel.org/n/tip-l7ovfblq14ip2i08m1g0fkhv@...
Signed-off-by: Arnaldo Carvalho de Melo <[hidden email]>
(cherry picked from commit cd8dd032f61abeb08d2c03bab4968a9de231a1be)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 tools/perf/util/cgroup.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index 0334774..0e77bc9e5 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -98,8 +98,10 @@ static int add_cgroup(struct perf_evlist *evlist, char *str)
  cgrp = counter->cgrp;
  if (!cgrp)
  continue;
- if (!strcmp(cgrp->name, str))
+ if (!strcmp(cgrp->name, str)) {
+ refcount_inc(&cgrp->refcnt);
  break;
+ }
 
  cgrp = NULL;
  }
@@ -110,6 +112,7 @@ static int add_cgroup(struct perf_evlist *evlist, char *str)
  return -1;
 
  cgrp->name = str;
+ refcount_set(&cgrp->refcnt, 1);
 
  cgrp->fd = open_cgroup(str);
  if (cgrp->fd == -1) {
@@ -128,12 +131,11 @@ static int add_cgroup(struct perf_evlist *evlist, char *str)
  goto found;
  n++;
  }
- if (refcount_read(&cgrp->refcnt) == 0)
+ if (refcount_dec_and_test(&cgrp->refcnt))
  free(cgrp);
 
  return -1;
 found:
- refcount_inc(&cgrp->refcnt);
  counter->cgrp = cgrp;
  return 0;
 }
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Artful][PATCH 2/2] perf xyarray: Fix wrong processing when closing evsel fd

Joseph Salisbury-3
In reply to this post by Joseph Salisbury-3
From: Jin Yao <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1769027

In current xyarray code, xyarray__max_x() returns max_y, and xyarray__max_y()
returns max_x.

It's confusing and for code logic it looks not correct.

Error happens when closing evsel fd. Let's see this scenario:

1. Allocate an fd (pseudo-code)

  perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
  {
        evsel->fd = xyarray__new(ncpus, nthreads, sizeof(int));
  }

  xyarray__new(int xlen, int ylen, size_t entry_size)
  {
        size_t row_size = ylen * entry_size;
        struct xyarray *xy = zalloc(sizeof(*xy) + xlen * row_size);

        xy->entry_size = entry_size;
        xy->row_size   = row_size;
        xy->entries    = xlen * ylen;
        xy->max_x      = xlen;
        xy->max_y      = ylen;
        ......
  }

So max_x is ncpus, max_y is nthreads and row_size = nthreads * 4.

2. Use perf syscall and get the fd

  int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
                     struct thread_map *threads)
  {
        for (cpu = 0; cpu < cpus->nr; cpu++) {

                for (thread = 0; thread < nthreads; thread++) {
                        int fd, group_fd;

                        fd = sys_perf_event_open(&evsel->attr, pid, cpus->map[cpu],
                                                 group_fd, flags);

                        FD(evsel, cpu, thread) = fd;
        }
  }

  static inline void *xyarray__entry(struct xyarray *xy, int x, int y)
  {
        return &xy->contents[x * xy->row_size + y * xy->entry_size];
  }

These codes don't have issues. The issue happens in the closing of fd.

3. Close fd.

  void perf_evsel__close_fd(struct perf_evsel *evsel)
  {
        int cpu, thread;

        for (cpu = 0; cpu < xyarray__max_x(evsel->fd); cpu++)
                for (thread = 0; thread < xyarray__max_y(evsel->fd); ++thread) {
                        close(FD(evsel, cpu, thread));
                        FD(evsel, cpu, thread) = -1;
                }
  }

  Since xyarray__max_x() returns max_y (nthreads) and xyarry__max_y()
  returns max_x (ncpus), so above code is actually to be:

        for (cpu = 0; cpu < nthreads; cpu++)
                for (thread = 0; thread < ncpus; ++thread) {
                        close(FD(evsel, cpu, thread));
                        FD(evsel, cpu, thread) = -1;
                }

  It's not correct!

This change is introduced by "475fb533fb7d" ("perf evsel: Fix buffer overflow
while freeing events")

This fix is to let xyarray__max_x() return max_x (ncpus) and
let xyarry__max_y() return max_y (nthreads)

Committer note:

This was also fixed by Ravi Bangoria, who provided the same patch,
noticing the problem with 'perf record':

<quote Ravi>
I see 'perf record -p <pid>' crashes with following log:

   *** Error in `./perf': free(): invalid next size (normal): 0x000000000298b340 ***
   ======= Backtrace: =========
   /lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7f7fd85c87e5]
   /lib/x86_64-linux-gnu/libc.so.6(+0x8037a)[0x7f7fd85d137a]
   /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f7fd85d553c]
   ./perf(perf_evsel__close+0xb4)[0x4b7614]
   ./perf(perf_evlist__delete+0x100)[0x4ab180]
   ./perf(cmd_record+0x1d9)[0x43a5a9]
   ./perf[0x49aa2f]
   ./perf(main+0x631)[0x427841]
   /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f7fd8571830]
   ./perf(_start+0x29)[0x427a59]
</>

Signed-off-by: Jin Yao <[hidden email]>
Acked-by: Jiri Olsa <[hidden email]>
Cc: Alexander Shishkin <[hidden email]>
Cc: Andi Kleen <[hidden email]>
Cc: Kan Liang <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Ravi Bangoria <[hidden email]>
Fixes: d74be4767367 ("perf xyarray: Save max_x, max_y")
Link: http://lkml.kernel.org/r/1508339478-26674-1-git-send-email-yao.jin@...
Link: http://lkml.kernel.org/r/1508327446-15302-1-git-send-email-ravi.bangoria@...
Signed-off-by: Arnaldo Carvalho de Melo <[hidden email]>
(cherry picked from commit 3d8bba9535ac6e79453c769dd0c8ea852a51ad60)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 tools/perf/util/xyarray.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/xyarray.h b/tools/perf/util/xyarray.h
index 4ba726c..54af6046 100644
--- a/tools/perf/util/xyarray.h
+++ b/tools/perf/util/xyarray.h
@@ -23,12 +23,12 @@ static inline void *xyarray__entry(struct xyarray *xy, int x, int y)
 
 static inline int xyarray__max_y(struct xyarray *xy)
 {
- return xy->max_x;
+ return xy->max_y;
 }
 
 static inline int xyarray__max_x(struct xyarray *xy)
 {
- return xy->max_y;
+ return xy->max_x;
 }
 
 #endif /* _PERF_XYARRAY_H_ */
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Artful][PATCH 0/2] Fixes for LP:1769027

Kleber Souza
In reply to this post by Joseph Salisbury-3
On 05/11/18 20:20, Joseph Salisbury wrote:

> == SRU Justification ==
> This SRU request is for two commits, that are needed for two bug reports.  The
> first bug(1767204) is marked as a duplicate of the bug used for this SRU
> request.  The commit(3d8bba9535ac) required to fix the first bug introduced the second
> bug.  The second bug is then fixed buy commit cd8dd032f61a.
>
> The first issue is perf crashes due to swapped xyarray function signatures and is
> fixed by commit 3d8bba9535ac.
>
> The second issue is a crash due to "refcount_inc assertion failed".
> This second bug is introduced by picking commit 3d8bba9535ac without
> picking commit cd8dd032f61a first.
>
>
> == Fixes ==
> cd8dd032f61a ("perf cgroup: Fix refcount usage")
> 3d8bba9535ac ("perf xyarray: Fix wrong processing when closing evsel fd")
>
> == Regression Potential ==
> Low.  Limited to perf tool.
>
> == Test Case ==
> A test kernel was built with these patches and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
> Arnaldo Carvalho de Melo (1):
>   perf cgroup: Fix refcount usage
>
> Jin Yao (1):
>   perf xyarray: Fix wrong processing when closing evsel fd
>
>  tools/perf/util/cgroup.c  | 8 +++++---
>  tools/perf/util/xyarray.h | 4 ++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
>

Scope limited to perftools, tested by the bug reporter.

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Artful][PATCH 0/2] Fixes for LP:1769027

Khalid Elmously
In reply to this post by Joseph Salisbury-3
On 2018-05-11 14:20:50 , Joseph Salisbury wrote:

> == SRU Justification ==
> This SRU request is for two commits, that are needed for two bug reports.  The
> first bug(1767204) is marked as a duplicate of the bug used for this SRU
> request.  The commit(3d8bba9535ac) required to fix the first bug introduced the second
> bug.  The second bug is then fixed buy commit cd8dd032f61a.
>
> The first issue is perf crashes due to swapped xyarray function signatures and is
> fixed by commit 3d8bba9535ac.
>
> The second issue is a crash due to "refcount_inc assertion failed".
> This second bug is introduced by picking commit 3d8bba9535ac without
> picking commit cd8dd032f61a first.
>
>
> == Fixes ==
> cd8dd032f61a ("perf cgroup: Fix refcount usage")
> 3d8bba9535ac ("perf xyarray: Fix wrong processing when closing evsel fd")
>
> == Regression Potential ==
> Low.  Limited to perf tool.
>
> == Test Case ==
> A test kernel was built with these patches and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
> Arnaldo Carvalho de Melo (1):
>   perf cgroup: Fix refcount usage
>
> Jin Yao (1):
>   perf xyarray: Fix wrong processing when closing evsel fd
>
>  tools/perf/util/cgroup.c  | 8 +++++---
>  tools/perf/util/xyarray.h | 4 ++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
Acked-by: Khalid Elmously <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [SRU][Artful][PATCH 0/2] Fixes for LP:1769027

Khalid Elmously
In reply to this post by Joseph Salisbury-3
Applied to A

On 2018-05-11 14:20:50 , Joseph Salisbury wrote:

> == SRU Justification ==
> This SRU request is for two commits, that are needed for two bug reports.  The
> first bug(1767204) is marked as a duplicate of the bug used for this SRU
> request.  The commit(3d8bba9535ac) required to fix the first bug introduced the second
> bug.  The second bug is then fixed buy commit cd8dd032f61a.
>
> The first issue is perf crashes due to swapped xyarray function signatures and is
> fixed by commit 3d8bba9535ac.
>
> The second issue is a crash due to "refcount_inc assertion failed".
> This second bug is introduced by picking commit 3d8bba9535ac without
> picking commit cd8dd032f61a first.
>
>
> == Fixes ==
> cd8dd032f61a ("perf cgroup: Fix refcount usage")
> 3d8bba9535ac ("perf xyarray: Fix wrong processing when closing evsel fd")
>
> == Regression Potential ==
> Low.  Limited to perf tool.
>
> == Test Case ==
> A test kernel was built with these patches and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
> Arnaldo Carvalho de Melo (1):
>   perf cgroup: Fix refcount usage
>
> Jin Yao (1):
>   perf xyarray: Fix wrong processing when closing evsel fd
>
>  tools/perf/util/cgroup.c  | 8 +++++---
>  tools/perf/util/xyarray.h | 4 ++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> --
> 2.7.4
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU][Artful][PATCH 0/2] Fixes for LP:1769027

Khalid Elmously
In reply to this post by Joseph Salisbury-3
Adding "APPLIED" to the subject


On 2018-05-11 14:20:50 , Joseph Salisbury wrote:

> == SRU Justification ==
> This SRU request is for two commits, that are needed for two bug reports.  The
> first bug(1767204) is marked as a duplicate of the bug used for this SRU
> request.  The commit(3d8bba9535ac) required to fix the first bug introduced the second
> bug.  The second bug is then fixed buy commit cd8dd032f61a.
>
> The first issue is perf crashes due to swapped xyarray function signatures and is
> fixed by commit 3d8bba9535ac.
>
> The second issue is a crash due to "refcount_inc assertion failed".
> This second bug is introduced by picking commit 3d8bba9535ac without
> picking commit cd8dd032f61a first.
>
>
> == Fixes ==
> cd8dd032f61a ("perf cgroup: Fix refcount usage")
> 3d8bba9535ac ("perf xyarray: Fix wrong processing when closing evsel fd")
>
> == Regression Potential ==
> Low.  Limited to perf tool.
>
> == Test Case ==
> A test kernel was built with these patches and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
> Arnaldo Carvalho de Melo (1):
>   perf cgroup: Fix refcount usage
>
> Jin Yao (1):
>   perf xyarray: Fix wrong processing when closing evsel fd
>
>  tools/perf/util/cgroup.c  | 8 +++++---
>  tools/perf/util/xyarray.h | 4 ++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> --
> 2.7.4
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team