[PATCH] perf util: Fix wrong processing when closing evsel fd

* [PATCH] perf util: Fix wrong processing when closing evsel fd
@ 2017-10-18 15:11 Jin Yao
  2017-10-18  8:12 ` Jiri Olsa
  2017-10-18 11:50 ` [PATCH] perf record: Fix tool crash with xyarray Ravi Bangoria
  0 siblings, 2 replies; 6+ messages in thread
From: Jin Yao @ 2017-10-18 15:11 UTC (permalink / raw)
  To: acme, jolsa, peterz, mingo, alexander.shishkin
  Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao

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)

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread