Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1585426 > unrolled thread

[PATCH 0/9] tools subsystem refcounter conversions

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/9] tools subsystem refcounter conversions Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
    [PATCH 9/9] tools: convert thread_map.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
    [PATCH 2/9] tools: convert cpu_map.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
      Re: [PATCH 2/9] tools: convert cpu_map.refcnt from atomic_t to  refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 21:40 +0100
    [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
      Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to  refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-21 16:50 +0100
        RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 15:30 +0100
          Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to  refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 16:50 +0100
            RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 17:20 +0100
              Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to  refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 21:40 +0100
                RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 14:20 +0100
    [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
      Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to  refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 21:40 +0100
        Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to  refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 23:30 +0100
          Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to  refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 23:40 +0100
            RE: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 10:20 +0100
              Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to  refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 14:20 +0100
    Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-21 16:50 +0100
      Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 00:30 +0100
        Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 00:30 +0100
          RE: [PATCH 0/9] tools subsystem refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 12:40 +0100
            Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 14:00 +0100
            Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 17:30 +0100
              RE: [PATCH 0/9] tools subsystem refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-24 08:40 +0100
                Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-24 14:40 +0100
    Re: [PATCH 0/9] tools subsystem refcounter conversions Peter Zijlstra <peterz@infradead.org> - 2017-02-21 16:50 +0100
      RE: [PATCH 0/9] tools subsystem refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-21 17:10 +0100

Page 1 of 2  [1] 2  Next page →

#1585426 — [PATCH 0/9] tools subsystem refcounter conversions

Now when new refcount_t type and API are finally merged
(see include/linux/refcount.h), the following
patches convert various refcounters in the tools susystem from atomic_t
to refcount_t. By doing this we prevent intentional or accidental
underflows or overflows that can led to use-after-free vulnerabilities.

The below patches are fully independent and can be cherry-picked separately.
Since we convert all kernel subsystems in the same fashion, resulting
in about 300 patches, we have to group them for sending at least in some
fashion to be manageable. Please excuse the long cc list.

Elena Reshetova (9):
  tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
  tools: convert cpu_map.refcnt from atomic_t to refcount_t
  tools: convert comm_str.refcnt from atomic_t to refcount_t
  tools: convert dso.refcnt from atomic_t to refcount_t
  tools: convert map.refcnt from atomic_t to refcount_t
  tools: convert map_groups.refcnt from atomic_t to refcount_t
  tools: convert perf_map.refcnt from atomic_t to refcount_t
  tools: convert thread.refcnt from atomic_t to refcount_t
  tools: convert thread_map.refcnt from atomic_t to refcount_t

 tools/perf/util/cgroup.c     |  6 +++---
 tools/perf/util/cgroup.h     |  4 ++--
 tools/perf/util/comm.c       | 13 +++++--------
 tools/perf/util/cpumap.c     | 16 ++++++++--------
 tools/perf/util/cpumap.h     |  4 ++--
 tools/perf/util/dso.c        |  6 +++---
 tools/perf/util/dso.h        |  4 ++--
 tools/perf/util/evlist.c     | 18 +++++++++---------
 tools/perf/util/evlist.h     |  4 ++--
 tools/perf/util/map.c        | 10 +++++-----
 tools/perf/util/map.h        | 10 +++++-----
 tools/perf/util/thread.c     |  6 +++---
 tools/perf/util/thread.h     |  4 ++--
 tools/perf/util/thread_map.c | 20 ++++++++++----------
 tools/perf/util/thread_map.h |  4 ++--
 15 files changed, 63 insertions(+), 66 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1585427 — [PATCH 9/9] tools: convert thread_map.refcnt from atomic_t to refcount_t

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 tools/perf/util/thread_map.c | 20 ++++++++++----------
 tools/perf/util/thread_map.h |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
index 7c3fcc5..9026408 100644
--- a/tools/perf/util/thread_map.c
+++ b/tools/perf/util/thread_map.c
@@ -66,7 +66,7 @@ struct thread_map *thread_map__new_by_pid(pid_t pid)
 		for (i = 0; i < items; i++)
 			thread_map__set_pid(threads, i, atoi(namelist[i]->d_name));
 		threads->nr = items;
-		atomic_set(&threads->refcnt, 1);
+		refcount_set(&threads->refcnt, 1);
 	}
 
 	for (i=0; i<items; i++)
@@ -83,7 +83,7 @@ struct thread_map *thread_map__new_by_tid(pid_t tid)
 	if (threads != NULL) {
 		thread_map__set_pid(threads, 0, tid);
 		threads->nr = 1;
-		atomic_set(&threads->refcnt, 1);
+		refcount_set(&threads->refcnt, 1);
 	}
 
 	return threads;
@@ -105,7 +105,7 @@ struct thread_map *thread_map__new_by_uid(uid_t uid)
 		goto out_free_threads;
 
 	threads->nr = 0;
-	atomic_set(&threads->refcnt, 1);
+	refcount_set(&threads->refcnt, 1);
 
 	while ((dirent = readdir(proc)) != NULL) {
 		char *end;
@@ -235,7 +235,7 @@ static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
 out:
 	strlist__delete(slist);
 	if (threads)
-		atomic_set(&threads->refcnt, 1);
+		refcount_set(&threads->refcnt, 1);
 	return threads;
 
 out_free_namelist:
@@ -255,7 +255,7 @@ struct thread_map *thread_map__new_dummy(void)
 	if (threads != NULL) {
 		thread_map__set_pid(threads, 0, -1);
 		threads->nr = 1;
-		atomic_set(&threads->refcnt, 1);
+		refcount_set(&threads->refcnt, 1);
 	}
 	return threads;
 }
@@ -300,7 +300,7 @@ struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
 	}
 out:
 	if (threads)
-		atomic_set(&threads->refcnt, 1);
+		refcount_set(&threads->refcnt, 1);
 	return threads;
 
 out_free_threads:
@@ -326,7 +326,7 @@ static void thread_map__delete(struct thread_map *threads)
 	if (threads) {
 		int i;
 
-		WARN_ONCE(atomic_read(&threads->refcnt) != 0,
+		WARN_ONCE(refcount_read(&threads->refcnt) != 0,
 			  "thread map refcnt unbalanced\n");
 		for (i = 0; i < threads->nr; i++)
 			free(thread_map__comm(threads, i));
@@ -337,13 +337,13 @@ static void thread_map__delete(struct thread_map *threads)
 struct thread_map *thread_map__get(struct thread_map *map)
 {
 	if (map)
-		atomic_inc(&map->refcnt);
+		refcount_inc(&map->refcnt);
 	return map;
 }
 
 void thread_map__put(struct thread_map *map)
 {
-	if (map && atomic_dec_and_test(&map->refcnt))
+	if (map && refcount_dec_and_test(&map->refcnt))
 		thread_map__delete(map);
 }
 
@@ -423,7 +423,7 @@ static void thread_map__copy_event(struct thread_map *threads,
 		threads->map[i].comm = strndup(event->entries[i].comm, 16);
 	}
 
-	atomic_set(&threads->refcnt, 1);
+	refcount_set(&threads->refcnt, 1);
 }
 
 struct thread_map *thread_map__new_event(struct thread_map_event *event)
diff --git a/tools/perf/util/thread_map.h b/tools/perf/util/thread_map.h
index ea0ef08..bd34d7a 100644
--- a/tools/perf/util/thread_map.h
+++ b/tools/perf/util/thread_map.h
@@ -3,7 +3,7 @@
 
 #include <sys/types.h>
 #include <stdio.h>
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 
 struct thread_map_data {
 	pid_t    pid;
@@ -11,7 +11,7 @@ struct thread_map_data {
 };
 
 struct thread_map {
-	atomic_t refcnt;
+	refcount_t refcnt;
 	int nr;
 	struct thread_map_data map[];
 };
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1585429 — [PATCH 2/9] tools: convert cpu_map.refcnt from atomic_t to refcount_t

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 tools/perf/util/cpumap.c | 16 ++++++++--------
 tools/perf/util/cpumap.h |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 2c0b522..0e21e28 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -28,7 +28,7 @@ static struct cpu_map *cpu_map__default_new(void)
 			cpus->map[i] = i;
 
 		cpus->nr = nr_cpus;
-		atomic_set(&cpus->refcnt, 1);
+		refcount_set(&cpus->refcnt, 1);
 	}
 
 	return cpus;
@@ -42,7 +42,7 @@ static struct cpu_map *cpu_map__trim_new(int nr_cpus, int *tmp_cpus)
 	if (cpus != NULL) {
 		cpus->nr = nr_cpus;
 		memcpy(cpus->map, tmp_cpus, payload_size);
-		atomic_set(&cpus->refcnt, 1);
+		refcount_set(&cpus->refcnt, 1);
 	}
 
 	return cpus;
@@ -251,7 +251,7 @@ struct cpu_map *cpu_map__dummy_new(void)
 	if (cpus != NULL) {
 		cpus->nr = 1;
 		cpus->map[0] = -1;
-		atomic_set(&cpus->refcnt, 1);
+		refcount_set(&cpus->refcnt, 1);
 	}
 
 	return cpus;
@@ -268,7 +268,7 @@ struct cpu_map *cpu_map__empty_new(int nr)
 		for (i = 0; i < nr; i++)
 			cpus->map[i] = -1;
 
-		atomic_set(&cpus->refcnt, 1);
+		refcount_set(&cpus->refcnt, 1);
 	}
 
 	return cpus;
@@ -277,7 +277,7 @@ struct cpu_map *cpu_map__empty_new(int nr)
 static void cpu_map__delete(struct cpu_map *map)
 {
 	if (map) {
-		WARN_ONCE(atomic_read(&map->refcnt) != 0,
+		WARN_ONCE(refcount_read(&map->refcnt) != 0,
 			  "cpu_map refcnt unbalanced\n");
 		free(map);
 	}
@@ -286,13 +286,13 @@ static void cpu_map__delete(struct cpu_map *map)
 struct cpu_map *cpu_map__get(struct cpu_map *map)
 {
 	if (map)
-		atomic_inc(&map->refcnt);
+		refcount_inc(&map->refcnt);
 	return map;
 }
 
 void cpu_map__put(struct cpu_map *map)
 {
-	if (map && atomic_dec_and_test(&map->refcnt))
+	if (map && refcount_dec_and_test(&map->refcnt))
 		cpu_map__delete(map);
 }
 
@@ -356,7 +356,7 @@ int cpu_map__build_map(struct cpu_map *cpus, struct cpu_map **res,
 	/* ensure we process id in increasing order */
 	qsort(c->map, c->nr, sizeof(int), cmp_ids);
 
-	atomic_set(&c->refcnt, 1);
+	refcount_set(&c->refcnt, 1);
 	*res = c;
 	return 0;
 }
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 06bd689..4f12a01 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -3,13 +3,13 @@
 
 #include <stdio.h>
 #include <stdbool.h>
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 
 #include "perf.h"
 #include "util/debug.h"
 
 struct cpu_map {
-	atomic_t refcnt;
+	refcount_t refcnt;
 	int nr;
 	int map[];
 };
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1586460 — Re: [PATCH 2/9] tools: convert cpu_map.refcnt from atomic_t to refcount_t

Em Tue, Feb 21, 2017 at 05:34:56PM +0200, Elena Reshetova escreveu:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.

The following patch was needed for this one to build:

diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
index f168a85992d0..4478773cdb97 100644
--- a/tools/perf/tests/cpumap.c
+++ b/tools/perf/tests/cpumap.c
@@ -66,7 +66,7 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
 	TEST_ASSERT_VAL("wrong nr",  map->nr == 2);
 	TEST_ASSERT_VAL("wrong cpu", map->map[0] == 1);
 	TEST_ASSERT_VAL("wrong cpu", map->map[1] == 256);
-	TEST_ASSERT_VAL("wrong refcnt", atomic_read(&map->refcnt) == 1);
+	TEST_ASSERT_VAL("wrong refcnt", refcount_read(&map->refcnt) == 1);
 	cpu_map__put(map);
 	return 0;
 }
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index e84491636c1b..ab1aeed8cd5d 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -3,10 +3,10 @@
 
 #include <stdio.h>
 #include <stdbool.h>
-#include <linux/refcount.h>
 
 #include "perf.h"
 #include "util/debug.h"
+#include <linux/refcount.h>
 
 struct cpu_map {
 	refcount_t refcnt;
 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  tools/perf/util/cpumap.c | 16 ++++++++--------
>  tools/perf/util/cpumap.h |  4 ++--
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 2c0b522..0e21e28 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -28,7 +28,7 @@ static struct cpu_map *cpu_map__default_new(void)
>  			cpus->map[i] = i;
>  
>  		cpus->nr = nr_cpus;
> -		atomic_set(&cpus->refcnt, 1);
> +		refcount_set(&cpus->refcnt, 1);
>  	}
>  
>  	return cpus;
> @@ -42,7 +42,7 @@ static struct cpu_map *cpu_map__trim_new(int nr_cpus, int *tmp_cpus)
>  	if (cpus != NULL) {
>  		cpus->nr = nr_cpus;
>  		memcpy(cpus->map, tmp_cpus, payload_size);
> -		atomic_set(&cpus->refcnt, 1);
> +		refcount_set(&cpus->refcnt, 1);
>  	}
>  
>  	return cpus;
> @@ -251,7 +251,7 @@ struct cpu_map *cpu_map__dummy_new(void)
>  	if (cpus != NULL) {
>  		cpus->nr = 1;
>  		cpus->map[0] = -1;
> -		atomic_set(&cpus->refcnt, 1);
> +		refcount_set(&cpus->refcnt, 1);
>  	}
>  
>  	return cpus;
> @@ -268,7 +268,7 @@ struct cpu_map *cpu_map__empty_new(int nr)
>  		for (i = 0; i < nr; i++)
>  			cpus->map[i] = -1;
>  
> -		atomic_set(&cpus->refcnt, 1);
> +		refcount_set(&cpus->refcnt, 1);
>  	}
>  
>  	return cpus;
> @@ -277,7 +277,7 @@ struct cpu_map *cpu_map__empty_new(int nr)
>  static void cpu_map__delete(struct cpu_map *map)
>  {
>  	if (map) {
> -		WARN_ONCE(atomic_read(&map->refcnt) != 0,
> +		WARN_ONCE(refcount_read(&map->refcnt) != 0,
>  			  "cpu_map refcnt unbalanced\n");
>  		free(map);
>  	}
> @@ -286,13 +286,13 @@ static void cpu_map__delete(struct cpu_map *map)
>  struct cpu_map *cpu_map__get(struct cpu_map *map)
>  {
>  	if (map)
> -		atomic_inc(&map->refcnt);
> +		refcount_inc(&map->refcnt);
>  	return map;
>  }
>  
>  void cpu_map__put(struct cpu_map *map)
>  {
> -	if (map && atomic_dec_and_test(&map->refcnt))
> +	if (map && refcount_dec_and_test(&map->refcnt))
>  		cpu_map__delete(map);
>  }
>  
> @@ -356,7 +356,7 @@ int cpu_map__build_map(struct cpu_map *cpus, struct cpu_map **res,
>  	/* ensure we process id in increasing order */
>  	qsort(c->map, c->nr, sizeof(int), cmp_ids);
>  
> -	atomic_set(&c->refcnt, 1);
> +	refcount_set(&c->refcnt, 1);
>  	*res = c;
>  	return 0;
>  }
> diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> index 06bd689..4f12a01 100644
> --- a/tools/perf/util/cpumap.h
> +++ b/tools/perf/util/cpumap.h
> @@ -3,13 +3,13 @@
>  
>  #include <stdio.h>
>  #include <stdbool.h>
> -#include <linux/atomic.h>
> +#include <linux/refcount.h>
>  
>  #include "perf.h"
>  #include "util/debug.h"
>  
>  struct cpu_map {
> -	atomic_t refcnt;
> +	refcount_t refcnt;
>  	int nr;
>  	int map[];
>  };
> -- 
> 2.7.4

[toc] | [prev] | [next] | [standalone]

#1585432 — [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 tools/perf/util/cgroup.c | 6 +++---
 tools/perf/util/cgroup.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index eafbf114..86399ed 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -127,19 +127,19 @@ static int add_cgroup(struct perf_evlist *evlist, char *str)
 			goto found;
 		n++;
 	}
-	if (atomic_read(&cgrp->refcnt) == 0)
+	if (refcount_read(&cgrp->refcnt) == 0)
 		free(cgrp);
 
 	return -1;
 found:
-	atomic_inc(&cgrp->refcnt);
+	refcount_inc(&cgrp->refcnt);
 	counter->cgrp = cgrp;
 	return 0;
 }
 
 void close_cgroup(struct cgroup_sel *cgrp)
 {
-	if (cgrp && atomic_dec_and_test(&cgrp->refcnt)) {
+	if (cgrp && refcount_dec_and_test(&cgrp->refcnt)) {
 		close(cgrp->fd);
 		zfree(&cgrp->name);
 		free(cgrp);
diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
index 31f8dcd..d91966b 100644
--- a/tools/perf/util/cgroup.h
+++ b/tools/perf/util/cgroup.h
@@ -1,14 +1,14 @@
 #ifndef __CGROUP_H__
 #define __CGROUP_H__
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 
 struct option;
 
 struct cgroup_sel {
 	char *name;
 	int fd;
-	atomic_t refcnt;
+	refcount_t refcnt;
 };
 
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1585435 — Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t

Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
>  #define __CGROUP_H__
>  
> -#include <linux/atomic.h>
> +#include <linux/refcount.h>

So this is the first one, I was expecting the copy from
include/linux/refcount.h to be made to tools/include/linux/refcount.h,
as was done for tools/include/linux/atomic.h and all the other stuff in
tools/include/

See:

commit c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3f
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Mon Jul 11 10:28:48 2016 -0300

    tools: Add copy of perf_event.h to tools/include/linux/

--------------

For one of the reasons we've been doing this.

- Arnaldo
  
>  struct option;
>  
>  struct cgroup_sel {
>  	char *name;
>  	int fd;
> -	atomic_t refcnt;
> +	refcount_t refcnt;
>  };
>  
>  
> -- 
> 2.7.4

[toc] | [prev] | [next] | [standalone]

#1586194 — RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t

> Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >  #define __CGROUP_H__
> >
> > -#include <linux/atomic.h>
> > +#include <linux/refcount.h>
> 
> So this is the first one, I was expecting the copy from
> include/linux/refcount.h to be made to tools/include/linux/refcount.h,
> as was done for tools/include/linux/atomic.h and all the other stuff in
> tools/include/
> 
> See:
> 
> commit c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3f
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Mon Jul 11 10:28:48 2016 -0300
> 
>     tools: Add copy of perf_event.h to tools/include/linux/
> 
> --------------
> 
> For one of the reasons we've been doing this.

Hm.. I have taken a look on it and I am confused.
refcount.h is not exactly standalone header and seems to bring in quite some many dependencies to other headers (linux/bug.h, linux/mutex.h etc.), which are not present in tools headers dirs.
I tried to compile perf tool as a start, copied the refcount.h to tools/include/linux/ and somewhere after it wanted me to bring the 10th header I stopped, because this cannot be right, or?

Best Regards,
Elena.

[toc] | [prev] | [next] | [standalone]

#1586270 — Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t

Em Wed, Feb 22, 2017 at 02:29:18PM +0000, Reshetova, Elena escreveu:
> > Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > >  #define __CGROUP_H__
> > >
> > > -#include <linux/atomic.h>
> > > +#include <linux/refcount.h>
> > 
> > So this is the first one, I was expecting the copy from
> > include/linux/refcount.h to be made to tools/include/linux/refcount.h,
> > as was done for tools/include/linux/atomic.h and all the other stuff in
> > tools/include/
> > 
> > See:
> > 
> > commit c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3f
> > Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Date:   Mon Jul 11 10:28:48 2016 -0300
> > 
> >     tools: Add copy of perf_event.h to tools/include/linux/
> > 
> > --------------
> > 
> > For one of the reasons we've been doing this.
 
> Hm.. I have taken a look on it and I am confused.  refcount.h is not
> exactly standalone header and seems to bring in quite some many
> dependencies to other headers (linux/bug.h, linux/mutex.h etc.), which
> are not present in tools headers dirs.

> I tried to compile perf tool as a start, copied the refcount.h to
> tools/include/linux/ and somewhere after it wanted me to bring the
> 10th header I stopped, because this cannot be right, or?

So, it doesn't have to be a straight copy, and it just shows the problem
with using the kernel headers directly, i.e. tools/perf/ uses atomic.h,
and uses that for refcounting, but not all of include/linux/refcount.h
should be copied to tools/include/linux/refcount.h.

I'll try doing the work, that way I'll read about this new stuff, will
come back here with what I find, so you can continue on the kernel bits
for now, ok?

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1586291 — RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t

> Em Wed, Feb 22, 2017 at 02:29:18PM +0000, Reshetova, Elena escreveu:
> > > Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
> > > > refcount_t type and corresponding API should be
> > > > used instead of atomic_t when the variable is used as
> > > > a reference counter. This allows to avoid accidental
> > > > refcounter overflows that might lead to use-after-free
> > > > situations.
> > > >  #define __CGROUP_H__
> > > >
> > > > -#include <linux/atomic.h>
> > > > +#include <linux/refcount.h>
> > >
> > > So this is the first one, I was expecting the copy from
> > > include/linux/refcount.h to be made to tools/include/linux/refcount.h,
> > > as was done for tools/include/linux/atomic.h and all the other stuff in
> > > tools/include/
> > >
> > > See:
> > >
> > > commit c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3f
> > > Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > Date:   Mon Jul 11 10:28:48 2016 -0300
> > >
> > >     tools: Add copy of perf_event.h to tools/include/linux/
> > >
> > > --------------
> > >
> > > For one of the reasons we've been doing this.
> 
> > Hm.. I have taken a look on it and I am confused.  refcount.h is not
> > exactly standalone header and seems to bring in quite some many
> > dependencies to other headers (linux/bug.h, linux/mutex.h etc.), which
> > are not present in tools headers dirs.
> 
> > I tried to compile perf tool as a start, copied the refcount.h to
> > tools/include/linux/ and somewhere after it wanted me to bring the
> > 10th header I stopped, because this cannot be right, or?
> 
> So, it doesn't have to be a straight copy, and it just shows the problem
> with using the kernel headers directly, i.e. tools/perf/ uses atomic.h,
> and uses that for refcounting, but not all of include/linux/refcount.h
> should be copied to tools/include/linux/refcount.h.

Oh, this is a good hint. Actually when I drop the *_lock and *_mutex_lock functions
(which are not needed by tools anyway), indeed most of the issues with header inclusions are gone. 
However, there are still some additional atomic functions needed that are not present in current atomic headers of tools.

> 
> I'll try doing the work, that way I'll read about this new stuff, will
> come back here with what I find, so you can continue on the kernel bits
> for now, ok?

Sure, if you want to take it over, nobosy won't complain! We need many of such changes merged and not everyone is so nice to help :)
I think after the needed headers/functions from refcount/atomic are in place in tools, the current patches should compile with no or almost no changes, so hopefully it still makes your work easier!

Best Regards,
Elena.
> 
> - Arnaldo

[toc] | [prev] | [next] | [standalone]

#1586458 — Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t

Em Wed, Feb 22, 2017 at 04:10:59PM +0000, Reshetova, Elena escreveu:
> > Em Wed, Feb 22, 2017 at 02:29:18PM +0000, Reshetova, Elena escreveu:
> > > > Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
> > > > > refcount_t type and corresponding API should be
> > > > > used instead of atomic_t when the variable is used as
> > > > > a reference counter. This allows to avoid accidental
> > > > > refcounter overflows that might lead to use-after-free
> > > > > situations.
> > > > >  #define __CGROUP_H__
> > > > >
> > > > > -#include <linux/atomic.h>
> > > > > +#include <linux/refcount.h>
> > > >
> > > > So this is the first one, I was expecting the copy from
> > > > include/linux/refcount.h to be made to tools/include/linux/refcount.h,
> > > > as was done for tools/include/linux/atomic.h and all the other stuff in
> > > > tools/include/
> > > >
> > > > See:
> > > >
> > > > commit c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3f
> > > > Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > > Date:   Mon Jul 11 10:28:48 2016 -0300
> > > >
> > > >     tools: Add copy of perf_event.h to tools/include/linux/
> > > >
> > > > --------------
> > > >
> > > > For one of the reasons we've been doing this.
> > 
> > > Hm.. I have taken a look on it and I am confused.  refcount.h is not
> > > exactly standalone header and seems to bring in quite some many
> > > dependencies to other headers (linux/bug.h, linux/mutex.h etc.), which
> > > are not present in tools headers dirs.
> > 
> > > I tried to compile perf tool as a start, copied the refcount.h to
> > > tools/include/linux/ and somewhere after it wanted me to bring the
> > > 10th header I stopped, because this cannot be right, or?
> > 
> > So, it doesn't have to be a straight copy, and it just shows the problem
> > with using the kernel headers directly, i.e. tools/perf/ uses atomic.h,
> > and uses that for refcounting, but not all of include/linux/refcount.h
> > should be copied to tools/include/linux/refcount.h.
 
> Oh, this is a good hint. Actually when I drop the *_lock and
> *_mutex_lock functions (which are not needed by tools anyway), indeed
> most of the issues with header inclusions are gone.  However, there
> are still some additional atomic functions needed that are not present
> in current atomic headers of tools.

I did it, needed a good number of bits and pieces into
tools/{include,arch}/, now I am processing your patches and...
 
> > I'll try doing the work, that way I'll read about this new stuff, will
> > come back here with what I find, so you can continue on the kernel bits
> > for now, ok?
 
> Sure, if you want to take it over, nobosy won't complain! We need many
> of such changes merged and not everyone is so nice to help :) I think
> after the needed headers/functions from refcount/atomic are in place
> in tools, the current patches should compile with no or almost no
> changes, so hopefully it still makes your work easier!

You use things in refcount.h for which you are not adding the relevant
headers, like UINT_MAX, that is defined in linux/kernel.h, but that file
is not included in refcount.h, most of the time it is available by luck,
being something so commonly included, but some of your patches don't
build because of that, so I am moving the include <linux/refcount.h> to
after other headers to continue.

The right thing tho is to fix linux/refcount.h (and then its trimmed
down copy in tools/) to have everything it needs not to contribute to
the header messentropy. 8-)

Anyway, I'll put this in a branch later so that you can take a look.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1586876 — RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t

> Em Wed, Feb 22, 2017 at 04:10:59PM +0000, Reshetova, Elena escreveu:
> > > Em Wed, Feb 22, 2017 at 02:29:18PM +0000, Reshetova, Elena escreveu:
> > > > > Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
> > > > > > refcount_t type and corresponding API should be
> > > > > > used instead of atomic_t when the variable is used as
> > > > > > a reference counter. This allows to avoid accidental
> > > > > > refcounter overflows that might lead to use-after-free
> > > > > > situations.
> > > > > >  #define __CGROUP_H__
> > > > > >
> > > > > > -#include <linux/atomic.h>
> > > > > > +#include <linux/refcount.h>
> > > > >
> > > > > So this is the first one, I was expecting the copy from
> > > > > include/linux/refcount.h to be made to tools/include/linux/refcount.h,
> > > > > as was done for tools/include/linux/atomic.h and all the other stuff in
> > > > > tools/include/
> > > > >
> > > > > See:
> > > > >
> > > > > commit c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3f
> > > > > Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > > > Date:   Mon Jul 11 10:28:48 2016 -0300
> > > > >
> > > > >     tools: Add copy of perf_event.h to tools/include/linux/
> > > > >
> > > > > --------------
> > > > >
> > > > > For one of the reasons we've been doing this.
> > >
> > > > Hm.. I have taken a look on it and I am confused.  refcount.h is not
> > > > exactly standalone header and seems to bring in quite some many
> > > > dependencies to other headers (linux/bug.h, linux/mutex.h etc.), which
> > > > are not present in tools headers dirs.
> > >
> > > > I tried to compile perf tool as a start, copied the refcount.h to
> > > > tools/include/linux/ and somewhere after it wanted me to bring the
> > > > 10th header I stopped, because this cannot be right, or?
> > >
> > > So, it doesn't have to be a straight copy, and it just shows the problem
> > > with using the kernel headers directly, i.e. tools/perf/ uses atomic.h,
> > > and uses that for refcounting, but not all of include/linux/refcount.h
> > > should be copied to tools/include/linux/refcount.h.
> 
> > Oh, this is a good hint. Actually when I drop the *_lock and
> > *_mutex_lock functions (which are not needed by tools anyway), indeed
> > most of the issues with header inclusions are gone.  However, there
> > are still some additional atomic functions needed that are not present
> > in current atomic headers of tools.
> 
> I did it, needed a good number of bits and pieces into
> tools/{include,arch}/, now I am processing your patches and...
> 
> > > I'll try doing the work, that way I'll read about this new stuff, will
> > > come back here with what I find, so you can continue on the kernel bits
> > > for now, ok?
> 
> > Sure, if you want to take it over, nobosy won't complain! We need many
> > of such changes merged and not everyone is so nice to help :) I think
> > after the needed headers/functions from refcount/atomic are in place
> > in tools, the current patches should compile with no or almost no
> > changes, so hopefully it still makes your work easier!
> 
> You use things in refcount.h for which you are not adding the relevant
> headers, like UINT_MAX, that is defined in linux/kernel.h, but that file
> is not included in refcount.h, most of the time it is available by luck,
> being something so commonly included, but some of your patches don't
> build because of that, so I am moving the include <linux/refcount.h> to
> after other headers to continue.
> 
> The right thing tho is to fix linux/refcount.h (and then its trimmed
> down copy in tools/) to have everything it needs not to contribute to
> the header messentropy. 8-)

I agree that fixing refcount.h is the correct way to resolve this. 
I just send this one line patch to Peter directly. 

Best Regards,
Elena.

> 
> Anyway, I'll put this in a branch later so that you can take a look.
> 
> - Arnaldo

[toc] | [prev] | [next] | [standalone]

#1585433 — [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 tools/perf/util/comm.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index 21b7ff3..0fd3d70 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -2,12 +2,12 @@
 #include "util.h"
 #include <stdlib.h>
 #include <stdio.h>
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 
 struct comm_str {
 	char *str;
 	struct rb_node rb_node;
-	atomic_t refcnt;
+	refcount_t refcnt;
 };
 
 /* Should perhaps be moved to struct machine */
@@ -16,13 +16,13 @@ static struct rb_root comm_str_root;
 static struct comm_str *comm_str__get(struct comm_str *cs)
 {
 	if (cs)
-		atomic_inc(&cs->refcnt);
+		refcount_inc(&cs->refcnt);
 	return cs;
 }
 
 static void comm_str__put(struct comm_str *cs)
 {
-	if (cs && atomic_dec_and_test(&cs->refcnt)) {
+	if (cs && refcount_dec_and_test(&cs->refcnt)) {
 		rb_erase(&cs->rb_node, &comm_str_root);
 		zfree(&cs->str);
 		free(cs);
@@ -43,7 +43,7 @@ static struct comm_str *comm_str__alloc(const char *str)
 		return NULL;
 	}
 
-	atomic_set(&cs->refcnt, 0);
+	refcount_set(&cs->refcnt, 1);
 
 	return cs;
 }
@@ -95,8 +95,6 @@ struct comm *comm__new(const char *str, u64 timestamp, bool exec)
 		return NULL;
 	}
 
-	comm_str__get(comm->comm_str);
-
 	return comm;
 }
 
@@ -108,7 +106,6 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp, bool exec)
 	if (!new)
 		return -ENOMEM;
 
-	comm_str__get(new);
 	comm_str__put(old);
 	comm->comm_str = new;
 	comm->start = timestamp;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1586459 — Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t

Em Tue, Feb 21, 2017 at 05:34:57PM +0200, Elena Reshetova escreveu:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>

You are doing two things (well three) things here:

1. converting to refcnt.h

2. Initiationg the refcount to 1, which makes this take place:

[acme@jouet linux]$ m
make: Entering directory '/home/acme/git/linux/tools/perf'
  BUILD:   Doing 'make -j4' parallel build
Warning: arch/x86/include/asm/cpufeatures.h differs from kernel
  CC       /tmp/build/perf/util/comm.o
  INSTALL  trace_plugins
util/comm.c:16:25: error: ‘comm_str__get’ defined but not used [-Werror=unused-function]
 static struct comm_str *comm_str__get(struct comm_str *cs)
                         ^~~~~~~~~~~~~
cc1: all warnings being treated as errors
mv: cannot stat '/tmp/build/perf/util/.comm.o.tmp': No such file or directory
/home/acme/git/linux/tools/build/Makefile.build:101: recipe for target '/tmp/build/perf/util/comm.o' failed
make[4]: *** [/tmp/build/perf/util/comm.o] Error 1
/home/acme/git/linux/tools/build/Makefile.build:144: recipe for target 'util' failed
make[3]: *** [util] Error 2
Makefile.perf:523: recipe for target '/tmp/build/perf/libperf-in.o' failed
make[2]: *** [/tmp/build/perf/libperf-in.o] Error 2
Makefile.perf:204: recipe for target 'sub-make' failed
make[1]: *** [sub-make] Error 2
Makefile:108: recipe for target 'install-bin' failed
make: *** [install-bin] Error 2
make: Leaving directory '/home/acme/git/linux/tools/perf'
[acme@jouet linux]$

3) not test building your patches :-\

I'll let this pass this time, minor, I am removing the now unused
comm_str__get() function.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1586523 — Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t

Em Wed, Feb 22, 2017 at 05:33:50PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Feb 21, 2017 at 05:34:57PM +0200, Elena Reshetova escreveu:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> > 
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> 
> You are doing two things (well three) things here:
> 
> 1. converting to refcnt.h
> 
> 2. Initiationg the refcount to 1, which makes this take place:
> 
> [acme@jouet linux]$ m
> make: Entering directory '/home/acme/git/linux/tools/perf'
>   BUILD:   Doing 'make -j4' parallel build
> Warning: arch/x86/include/asm/cpufeatures.h differs from kernel
>   CC       /tmp/build/perf/util/comm.o
>   INSTALL  trace_plugins
> util/comm.c:16:25: error: ‘comm_str__get’ defined but not used [-Werror=unused-function]
>  static struct comm_str *comm_str__get(struct comm_str *cs)
>                          ^~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> mv: cannot stat '/tmp/build/perf/util/.comm.o.tmp': No such file or directory
> /home/acme/git/linux/tools/build/Makefile.build:101: recipe for target '/tmp/build/perf/util/comm.o' failed
> make[4]: *** [/tmp/build/perf/util/comm.o] Error 1
> /home/acme/git/linux/tools/build/Makefile.build:144: recipe for target 'util' failed
> make[3]: *** [util] Error 2
> Makefile.perf:523: recipe for target '/tmp/build/perf/libperf-in.o' failed
> make[2]: *** [/tmp/build/perf/libperf-in.o] Error 2
> Makefile.perf:204: recipe for target 'sub-make' failed
> make[1]: *** [sub-make] Error 2
> Makefile:108: recipe for target 'install-bin' failed
> make: *** [install-bin] Error 2
> make: Leaving directory '/home/acme/git/linux/tools/perf'
> [acme@jouet linux]$
> 
> 3) not test building your patches :-\
> 
> I'll let this pass this time, minor, I am removing the now unused
> comm_str__get() function.

But it can't get unused, because the comm_str__findnew() may return an
existing entry, that _needs_ to get its refcount bumped, that is the
reason for this refcount to be there... reinstating it:

#0  0x00007ffff522491f in raise () from /lib64/libc.so.6
#1  0x00007ffff522651a in abort () from /lib64/libc.so.6
#2  0x00007ffff5268200 in __libc_message () from /lib64/libc.so.6
#3  0x00007ffff527188a in _int_free () from /lib64/libc.so.6
#4  0x00007ffff52752bc in free () from /lib64/libc.so.6
#5  0x000000000051125f in comm_str__put (cs=0x35038e0) at util/comm.c:20
#6  0x00000000005115b3 in comm__free (comm=0x6f4ee90) at util/comm.c:113
#7  0x0000000000511e10 in thread__delete (thread=0x6f4ee10) at util/thread.c:81
#8  0x0000000000511f0e in thread__put (thread=0x6f4ee10) at util/thread.c:103
#9  0x0000000000504ea6 in machine__process_fork_event (machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at util/machine.c:1496
#10 0x0000000000505092 in machine__process_event (machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at util/machine.c:1544
#11 0x0000000000451ae9 in perf_top__mmap_read_idx (top=0x7fffffffa7c0, idx=3) at builtin-top.c:844
#12 0x0000000000451bb6 in perf_top__mmap_read (top=0x7fffffffa7c0) at builtin-top.c:857
#13 0x0000000000452229 in __cmd_top (top=0x7fffffffa7c0) at builtin-top.c:1002
#14 0x00000000004536a3 in cmd_top (argc=0, argv=0x7fffffffe150, prefix=0x0) at builtin-top.c:1332
#15 0x00000000004b82a8 in run_builtin (p=0xa17cd0 <commands+336>, argc=4, argv=0x7fffffffe150) at perf.c:359
#16 0x00000000004b8515 in handle_internal_command (argc=4, argv=0x7fffffffe150) at perf.c:421
#17 0x00000000004b865a in run_argv (argcp=0x7fffffffdf9c, argv=0x7fffffffdf90) at perf.c:467
#18 0x00000000004b8a5d in main (argc=4, argv=0x7fffffffe150) at perf.c:614 

And this brings us to my learning experience, i.e. this should've been caught
by this machinery, right? But that only if I leaked this object, right?

I need to read more on this, that is for sure ;-)

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1586526 — Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t

Em Wed, Feb 22, 2017 at 07:20:45PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Feb 22, 2017 at 05:33:50PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Feb 21, 2017 at 05:34:57PM +0200, Elena Reshetova escreveu:
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > > 
> > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: David Windsor <dwindsor@gmail.com>
> > 
> > You are doing two things (well three) things here:
> > 
> > 1. converting to refcnt.h
> > 
> > 2. Initiationg the refcount to 1, which makes this take place:
> > 
> > [acme@jouet linux]$ m
> > make: Entering directory '/home/acme/git/linux/tools/perf'
> >   BUILD:   Doing 'make -j4' parallel build
> > Warning: arch/x86/include/asm/cpufeatures.h differs from kernel
> >   CC       /tmp/build/perf/util/comm.o
> >   INSTALL  trace_plugins
> > util/comm.c:16:25: error: ‘comm_str__get’ defined but not used [-Werror=unused-function]
> >  static struct comm_str *comm_str__get(struct comm_str *cs)
> >                          ^~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> > mv: cannot stat '/tmp/build/perf/util/.comm.o.tmp': No such file or directory
> > /home/acme/git/linux/tools/build/Makefile.build:101: recipe for target '/tmp/build/perf/util/comm.o' failed
> > make[4]: *** [/tmp/build/perf/util/comm.o] Error 1
> > /home/acme/git/linux/tools/build/Makefile.build:144: recipe for target 'util' failed
> > make[3]: *** [util] Error 2
> > Makefile.perf:523: recipe for target '/tmp/build/perf/libperf-in.o' failed
> > make[2]: *** [/tmp/build/perf/libperf-in.o] Error 2
> > Makefile.perf:204: recipe for target 'sub-make' failed
> > make[1]: *** [sub-make] Error 2
> > Makefile:108: recipe for target 'install-bin' failed
> > make: *** [install-bin] Error 2
> > make: Leaving directory '/home/acme/git/linux/tools/perf'
> > [acme@jouet linux]$
> > 
> > 3) not test building your patches :-\
> > 
> > I'll let this pass this time, minor, I am removing the now unused
> > comm_str__get() function.
> 
> But it can't get unused, because the comm_str__findnew() may return an
> existing entry, that _needs_ to get its refcount bumped, that is the
> reason for this refcount to be there... reinstating it:
> 
> #0  0x00007ffff522491f in raise () from /lib64/libc.so.6
> #1  0x00007ffff522651a in abort () from /lib64/libc.so.6
> #2  0x00007ffff5268200 in __libc_message () from /lib64/libc.so.6
> #3  0x00007ffff527188a in _int_free () from /lib64/libc.so.6
> #4  0x00007ffff52752bc in free () from /lib64/libc.so.6
> #5  0x000000000051125f in comm_str__put (cs=0x35038e0) at util/comm.c:20
> #6  0x00000000005115b3 in comm__free (comm=0x6f4ee90) at util/comm.c:113
> #7  0x0000000000511e10 in thread__delete (thread=0x6f4ee10) at util/thread.c:81
> #8  0x0000000000511f0e in thread__put (thread=0x6f4ee10) at util/thread.c:103
> #9  0x0000000000504ea6 in machine__process_fork_event (machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at util/machine.c:1496
> #10 0x0000000000505092 in machine__process_event (machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at util/machine.c:1544
> #11 0x0000000000451ae9 in perf_top__mmap_read_idx (top=0x7fffffffa7c0, idx=3) at builtin-top.c:844
> #12 0x0000000000451bb6 in perf_top__mmap_read (top=0x7fffffffa7c0) at builtin-top.c:857
> #13 0x0000000000452229 in __cmd_top (top=0x7fffffffa7c0) at builtin-top.c:1002
> #14 0x00000000004536a3 in cmd_top (argc=0, argv=0x7fffffffe150, prefix=0x0) at builtin-top.c:1332
> #15 0x00000000004b82a8 in run_builtin (p=0xa17cd0 <commands+336>, argc=4, argv=0x7fffffffe150) at perf.c:359
> #16 0x00000000004b8515 in handle_internal_command (argc=4, argv=0x7fffffffe150) at perf.c:421
> #17 0x00000000004b865a in run_argv (argcp=0x7fffffffdf9c, argv=0x7fffffffdf90) at perf.c:467
> #18 0x00000000004b8a5d in main (argc=4, argv=0x7fffffffe150) at perf.c:614 
> 
> And this brings us to my learning experience, i.e. this should've been caught
> by this machinery, right? But that only if I leaked this object, right?
> 
> I need to read more on this, that is for sure ;-)

For reference, this is the patch on top of this:

+++ b/tools/perf/util/comm.c
@@ -13,6 +13,13 @@ struct comm_str {
 /* Should perhaps be moved to struct machine */
 static struct rb_root comm_str_root;
 
+static struct comm_str *comm_str__get(struct comm_str *cs)
+{
+       if (cs)
+               refcount_inc(&cs->refcnt);
+       return cs;
+}
+
 static void comm_str__put(struct comm_str *cs)
 {
        if (cs && refcount_dec_and_test(&cs->refcnt)) {
@@ -54,7 +61,7 @@ static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root)
 
                cmp = strcmp(str, iter->str);
                if (!cmp)
-                       return iter;
+                       return comm_str__get(iter);
 
                if (cmp < 0)
                        p = &(*p)->rb_left;
[acme@jouet linux]$

[toc] | [prev] | [next] | [standalone]

#1586770 — RE: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t

> Em Wed, Feb 22, 2017 at 07:20:45PM -0300, Arnaldo Carvalho de Melo
> escreveu:
> > Em Wed, Feb 22, 2017 at 05:33:50PM -0300, Arnaldo Carvalho de Melo
> escreveu:
> > > Em Tue, Feb 21, 2017 at 05:34:57PM +0200, Elena Reshetova escreveu:
> > > > refcount_t type and corresponding API should be
> > > > used instead of atomic_t when the variable is used as
> > > > a reference counter. This allows to avoid accidental
> > > > refcounter overflows that might lead to use-after-free
> > > > situations.
> > > >
> > > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > Signed-off-by: David Windsor <dwindsor@gmail.com>
> > >
> > > You are doing two things (well three) things here:
> > >
> > > 1. converting to refcnt.h
> > >
> > > 2. Initiationg the refcount to 1, which makes this take place:
> > >
> > > [acme@jouet linux]$ m
> > > make: Entering directory '/home/acme/git/linux/tools/perf'
> > >   BUILD:   Doing 'make -j4' parallel build
> > > Warning: arch/x86/include/asm/cpufeatures.h differs from kernel
> > >   CC       /tmp/build/perf/util/comm.o
> > >   INSTALL  trace_plugins
> > > util/comm.c:16:25: error: ‘comm_str__get’ defined but not used [-
> Werror=unused-function]
> > >  static struct comm_str *comm_str__get(struct comm_str *cs)
> > >                          ^~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > > mv: cannot stat '/tmp/build/perf/util/.comm.o.tmp': No such file or
> directory
> > > /home/acme/git/linux/tools/build/Makefile.build:101: recipe for target
> '/tmp/build/perf/util/comm.o' failed
> > > make[4]: *** [/tmp/build/perf/util/comm.o] Error 1
> > > /home/acme/git/linux/tools/build/Makefile.build:144: recipe for target
> 'util' failed
> > > make[3]: *** [util] Error 2
> > > Makefile.perf:523: recipe for target '/tmp/build/perf/libperf-in.o' failed
> > > make[2]: *** [/tmp/build/perf/libperf-in.o] Error 2
> > > Makefile.perf:204: recipe for target 'sub-make' failed
> > > make[1]: *** [sub-make] Error 2
> > > Makefile:108: recipe for target 'install-bin' failed
> > > make: *** [install-bin] Error 2
> > > make: Leaving directory '/home/acme/git/linux/tools/perf'
> > > [acme@jouet linux]$
> > >
> > > 3) not test building your patches :-\

Sorry about compilation errors: I totally forgot that tools is not getting compiled automatically when you build the whole tree with all configs on, so these patches 
really slipped through untested.

> > >
> > > I'll let this pass this time, minor, I am removing the now unused
> > > comm_str__get() function.
> >
> > But it can't get unused, because the comm_str__findnew() may return an
> > existing entry, that _needs_ to get its refcount bumped, that is the
> > reason for this refcount to be there... reinstating it:

True, we missed that it was reused behind the scenes. Your fix below does it correctly. 
The object resuse seems to be one of the main issues of this atomic_t to refcount_t conversions through the kernel. 
We have sooo many places where this happens (obvious and not so obvious ones) and every single of them would fail in run-time, unless we can modify the code not to do increments on zero. 

> >
> > #0  0x00007ffff522491f in raise () from /lib64/libc.so.6
> > #1  0x00007ffff522651a in abort () from /lib64/libc.so.6
> > #2  0x00007ffff5268200 in __libc_message () from /lib64/libc.so.6
> > #3  0x00007ffff527188a in _int_free () from /lib64/libc.so.6
> > #4  0x00007ffff52752bc in free () from /lib64/libc.so.6
> > #5  0x000000000051125f in comm_str__put (cs=0x35038e0) at util/comm.c:20
> > #6  0x00000000005115b3 in comm__free (comm=0x6f4ee90) at
> util/comm.c:113
> > #7  0x0000000000511e10 in thread__delete (thread=0x6f4ee10) at
> util/thread.c:81
> > #8  0x0000000000511f0e in thread__put (thread=0x6f4ee10) at
> util/thread.c:103
> > #9  0x0000000000504ea6 in machine__process_fork_event
> (machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at
> util/machine.c:1496
> > #10 0x0000000000505092 in machine__process_event (machine=0x21f4bf8,
> event=0x7fffed6b54a0, sample=0x7fffffff8420) at util/machine.c:1544
> > #11 0x0000000000451ae9 in perf_top__mmap_read_idx (top=0x7fffffffa7c0,
> idx=3) at builtin-top.c:844
> > #12 0x0000000000451bb6 in perf_top__mmap_read (top=0x7fffffffa7c0) at
> builtin-top.c:857
> > #13 0x0000000000452229 in __cmd_top (top=0x7fffffffa7c0) at builtin-
> top.c:1002
> > #14 0x00000000004536a3 in cmd_top (argc=0, argv=0x7fffffffe150,
> prefix=0x0) at builtin-top.c:1332
> > #15 0x00000000004b82a8 in run_builtin (p=0xa17cd0 <commands+336>,
> argc=4, argv=0x7fffffffe150) at perf.c:359
> > #16 0x00000000004b8515 in handle_internal_command (argc=4,
> argv=0x7fffffffe150) at perf.c:421
> > #17 0x00000000004b865a in run_argv (argcp=0x7fffffffdf9c,
> argv=0x7fffffffdf90) at perf.c:467
> > #18 0x00000000004b8a5d in main (argc=4, argv=0x7fffffffe150) at perf.c:614
> >
> > And this brings us to my learning experience, i.e. this should've been caught
> > by this machinery, right? But that only if I leaked this object, right?
> >
> > I need to read more on this, that is for sure ;-)

The way how current refcount_t implemented it would refuse to do any increments/decrements on zero, or increments/decrements on max values. 
Also, it should WARN about this cases so that people can trace the issue.

> 
> For reference, this is the patch on top of this:
> 
> +++ b/tools/perf/util/comm.c
> @@ -13,6 +13,13 @@ struct comm_str {
>  /* Should perhaps be moved to struct machine */
>  static struct rb_root comm_str_root;
> 
> +static struct comm_str *comm_str__get(struct comm_str *cs)
> +{
> +       if (cs)
> +               refcount_inc(&cs->refcnt);
> +       return cs;
> +}
> +
>  static void comm_str__put(struct comm_str *cs)
>  {
>         if (cs && refcount_dec_and_test(&cs->refcnt)) {
> @@ -54,7 +61,7 @@ static struct comm_str *comm_str__findnew(const char
> *str, struct rb_root *root)
> 
>                 cmp = strcmp(str, iter->str);
>                 if (!cmp)
> -                       return iter;
> +                       return comm_str__get(iter);
> 
>                 if (cmp < 0)
>                         p = &(*p)->rb_left;
> [acme@jouet linux]$

This looks correct now.
	

[toc] | [prev] | [next] | [standalone]

#1586877 — Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t

Em Thu, Feb 23, 2017 at 09:16:07AM +0000, Reshetova, Elena escreveu:
> > Em Wed, Feb 22, 2017 at 07:20:45PM -0300, Arnaldo Carvalho de Melo
> > > > make: *** [install-bin] Error 2
> > > > make: Leaving directory '/home/acme/git/linux/tools/perf'
> > > > [acme@jouet linux]$

> > > > 3) not test building your patches :-\
 
> Sorry about compilation errors: I totally forgot that tools is not
> getting compiled automatically when you build the whole tree with all
> configs on, so these patches really slipped through untested.

np, each component in the tree has its own idiosyncrasies, its really
difficult to test something so sweeping like this change, thanks again
for doing this work, use-after-free is evil, we should do more things
like this :-)

> > > > I'll let this pass this time, minor, I am removing the now unused
> > > > comm_str__get() function.

> > > But it can't get unused, because the comm_str__findnew() may return an
> > > existing entry, that _needs_ to get its refcount bumped, that is the
> > > reason for this refcount to be there... reinstating it:
 
> True, we missed that it was reused behind the scenes. Your fix below
> does it correctly.  The object resuse seems to be one of the main
> issues of this atomic_t to refcount_t conversions through the kernel.
> We have sooo many places where this happens (obvious and not so
> obvious ones) and every single of them would fail in run-time, unless
> we can modify the code not to do increments on zero. 

Yeah, but this code wasn't using the right idiom, which is to, in a
"__findnew()" method to lock it and before dropping the lock to bump the
refcount of whatever is going to be returned, so that after the lock is
dropped we don't open a window where that object can get removed from
the rbtree and hit the bit bucket.
 
> > > #0  0x00007ffff522491f in raise () from /lib64/libc.so.6
> > > #1  0x00007ffff522651a in abort () from /lib64/libc.so.6
> > > #2  0x00007ffff5268200 in __libc_message () from /lib64/libc.so.6
> > > #3  0x00007ffff527188a in _int_free () from /lib64/libc.so.6
> > > #4  0x00007ffff52752bc in free () from /lib64/libc.so.6
> > > #5  0x000000000051125f in comm_str__put (cs=0x35038e0) at util/comm.c:20
> > > #6  0x00000000005115b3 in comm__free (comm=0x6f4ee90) at

> > > And this brings us to my learning experience, i.e. this should've been caught
> > > by this machinery, right? But that only if I leaked this object, right?
> > >
> > > I need to read more on this, that is for sure ;-)
 
> The way how current refcount_t implemented it would refuse to do any
> increments/decrements on zero, or increments/decrements on max values.
> Also, it should WARN about this cases so that people can trace the
> issue.

Humm, but the sequence is:

1. refcount_set(1)

2. recount_dec_and_test(1) -> 0
      delete object, _free_ it

3. if there is any reference to it yet, without holding a refcount_inc()
   obtained reference (a __get() method for the protected object) it may
   well be pointing to something that was reused for another unrelated
   object, so it can get a value != 0 and then the next
   refcount_dec_and_test() will not catch it being zero as set in step #2.

   To really catch this we would have to just not delete it when it
   refcunt_dec_and_test(1) sets it to zero, i.e. _leak_ it, right?

I'll check some other conversion done in the kernel to see where I am
missing something...

> > For reference, this is the patch on top of this:
> > 
> > +++ b/tools/perf/util/comm.c
> > @@ -13,6 +13,13 @@ struct comm_str {

<SNIP>
 
> This looks correct now.

Thanks for checking,

- Arnaldo 

[toc] | [prev] | [next] | [standalone]

#1585436

Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> Now when new refcount_t type and API are finally merged
> (see include/linux/refcount.h), the following
> patches convert various refcounters in the tools susystem from atomic_t
> to refcount_t. By doing this we prevent intentional or accidental
> underflows or overflows that can led to use-after-free vulnerabilities.

Thanks for working on this! I was almost going to jump on doing this
myself!

I'll try and get this merged ASAP.

- Arnaldo
 
> The below patches are fully independent and can be cherry-picked separately.
> Since we convert all kernel subsystems in the same fashion, resulting
> in about 300 patches, we have to group them for sending at least in some
> fashion to be manageable. Please excuse the long cc list.
> 
> Elena Reshetova (9):
>   tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
>   tools: convert cpu_map.refcnt from atomic_t to refcount_t
>   tools: convert comm_str.refcnt from atomic_t to refcount_t
>   tools: convert dso.refcnt from atomic_t to refcount_t
>   tools: convert map.refcnt from atomic_t to refcount_t
>   tools: convert map_groups.refcnt from atomic_t to refcount_t
>   tools: convert perf_map.refcnt from atomic_t to refcount_t
>   tools: convert thread.refcnt from atomic_t to refcount_t
>   tools: convert thread_map.refcnt from atomic_t to refcount_t
> 
>  tools/perf/util/cgroup.c     |  6 +++---
>  tools/perf/util/cgroup.h     |  4 ++--
>  tools/perf/util/comm.c       | 13 +++++--------
>  tools/perf/util/cpumap.c     | 16 ++++++++--------
>  tools/perf/util/cpumap.h     |  4 ++--
>  tools/perf/util/dso.c        |  6 +++---
>  tools/perf/util/dso.h        |  4 ++--
>  tools/perf/util/evlist.c     | 18 +++++++++---------
>  tools/perf/util/evlist.h     |  4 ++--
>  tools/perf/util/map.c        | 10 +++++-----
>  tools/perf/util/map.h        | 10 +++++-----
>  tools/perf/util/thread.c     |  6 +++---
>  tools/perf/util/thread.h     |  4 ++--
>  tools/perf/util/thread_map.c | 20 ++++++++++----------
>  tools/perf/util/thread_map.h |  4 ++--
>  15 files changed, 63 insertions(+), 66 deletions(-)
> 
> -- 
> 2.7.4

[toc] | [prev] | [next] | [standalone]

#1586548

Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > Now when new refcount_t type and API are finally merged
> > (see include/linux/refcount.h), the following
> > patches convert various refcounters in the tools susystem from atomic_t
> > to refcount_t. By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> 
> Thanks for working on this! I was almost going to jump on doing this
> myself!
> 
> I'll try and get this merged ASAP.

So, please take a look at my tmp.perf/refcount branch at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git

There are multiple fixes in it to get it to build and test it, so far,
with:

  perf top -F 15000 -d 0

while doing kernel builds and tight usleep 1 loops to create lots of
short lived threads with its map_groups, maps, dsos, etc.

Now running some build tests in some 36 containers with assorted distros
and cross compilers.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1586549

Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > Now when new refcount_t type and API are finally merged
> > > (see include/linux/refcount.h), the following
> > > patches convert various refcounters in the tools susystem from atomic_t
> > > to refcount_t. By doing this we prevent intentional or accidental
> > > underflows or overflows that can led to use-after-free vulnerabilities.
> > 
> > Thanks for working on this! I was almost going to jump on doing this
> > myself!
> > 
> > I'll try and get this merged ASAP.
> 
> So, please take a look at my tmp.perf/refcount branch at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> 
> There are multiple fixes in it to get it to build and test it, so far,
> with:
> 
>   perf top -F 15000 -d 0
> 
> while doing kernel builds and tight usleep 1 loops to create lots of
> short lived threads with its map_groups, maps, dsos, etc.
> 
> Now running some build tests in some 36 containers with assorted distros
> and cross compilers.

Tomorrow I'll inject some refcount errors to test this all.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web