[alsa-devel] [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(-)
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; };
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
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.
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
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
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
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
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[]; };
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");@@ -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);}
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
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;
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
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:
converting to refcnt.h
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]$
- 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
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:
converting to refcnt.h
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]$
- 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]$
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:
converting to refcnt.h
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]$
- 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.
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]$
- 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
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/dso.c | 6 +++--- tools/perf/util/dso.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c index 3abe337..f88aa44 100644 --- a/tools/perf/util/dso.c +++ b/tools/perf/util/dso.c @@ -1109,7 +1109,7 @@ struct dso *dso__new(const char *name) INIT_LIST_HEAD(&dso->node); INIT_LIST_HEAD(&dso->data.open_entry); pthread_mutex_init(&dso->lock, NULL); - atomic_set(&dso->refcnt, 1); + refcount_set(&dso->refcnt, 1); }
return dso; @@ -1147,13 +1147,13 @@ void dso__delete(struct dso *dso) struct dso *dso__get(struct dso *dso) { if (dso) - atomic_inc(&dso->refcnt); + refcount_inc(&dso->refcnt); return dso; }
void dso__put(struct dso *dso) { - if (dso && atomic_dec_and_test(&dso->refcnt)) + if (dso && refcount_dec_and_test(&dso->refcnt)) dso__delete(dso); }
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h index ecc4bbd..12350b1 100644 --- a/tools/perf/util/dso.h +++ b/tools/perf/util/dso.h @@ -1,7 +1,7 @@ #ifndef __PERF_DSO #define __PERF_DSO
-#include <linux/atomic.h> +#include <linux/refcount.h> #include <linux/types.h> #include <linux/rbtree.h> #include <sys/types.h> @@ -187,7 +187,7 @@ struct dso { void *priv; u64 db_id; }; - atomic_t refcnt; + refcount_t refcnt; char name[0]; };
Em Tue, Feb 21, 2017 at 05:34:58PM +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.
Fixed by moving the include refcnt.h to later in the includes:
In file included from /home/acme/git/linux/tools/perf/util/dso.h:4:0, from /home/acme/git/linux/tools/perf/util/machine.h:7, from tests/thread-mg-share.c:2: /home/acme/git/linux/tools/include/linux/refcount.h: In function ‘refcount_inc_not_zero’: /home/acme/git/linux/tools/include/linux/refcount.h:95:23: error: ‘UINT_MAX’ undeclared (first use in this function) REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n"); ^ /home/acme/git/linux/tools/include/linux/refcount.h:47:41: note: in definition of macro ‘REFCOUNT_WARN’ #define REFCOUNT_WARN(cond, str) (void)(cond) ^~~~ /home/acme/git/linux/tools/include/linux/refcount.h:95:23: note: each undeclared identifier is reported only once for each function it appears in REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n"); ^ /home/acme/git/linux/tools/include/linux/refcount.h:47:41: note: in definition of macro ‘REFCOUNT_WARN’ #define REFCOUNT_WARN(cond, str) (void)(cond)
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/dso.c | 6 +++--- tools/perf/util/dso.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c index 3abe337..f88aa44 100644 --- a/tools/perf/util/dso.c +++ b/tools/perf/util/dso.c @@ -1109,7 +1109,7 @@ struct dso *dso__new(const char *name) INIT_LIST_HEAD(&dso->node); INIT_LIST_HEAD(&dso->data.open_entry); pthread_mutex_init(&dso->lock, NULL);
atomic_set(&dso->refcnt, 1);
refcount_set(&dso->refcnt, 1);}
return dso;
@@ -1147,13 +1147,13 @@ void dso__delete(struct dso *dso) struct dso *dso__get(struct dso *dso) { if (dso)
atomic_inc(&dso->refcnt);
refcount_inc(&dso->refcnt);}
void dso__put(struct dso *dso) {
- if (dso && atomic_dec_and_test(&dso->refcnt))
- if (dso && refcount_dec_and_test(&dso->refcnt)) dso__delete(dso);
}
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h index ecc4bbd..12350b1 100644 --- a/tools/perf/util/dso.h +++ b/tools/perf/util/dso.h @@ -1,7 +1,7 @@ #ifndef __PERF_DSO #define __PERF_DSO
-#include <linux/atomic.h> +#include <linux/refcount.h> #include <linux/types.h> #include <linux/rbtree.h> #include <sys/types.h> @@ -187,7 +187,7 @@ struct dso { void *priv; u64 db_id; };
- atomic_t refcnt;
- refcount_t refcnt; char name[0];
};
-- 2.7.4
Em Wed, Feb 22, 2017 at 05:37:46PM -0300, Arnaldo Carvalho de Melo escreveu:
Em Tue, Feb 21, 2017 at 05:34:58PM +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.
Fixed by moving the include refcnt.h to later in the includes:
Nope, in this case this trick didn't work, I'm going back and adding #include <linux/kernel.h> to refcnt.h.
In file included from /home/acme/git/linux/tools/perf/util/dso.h:4:0, from /home/acme/git/linux/tools/perf/util/machine.h:7, from tests/thread-mg-share.c:2: /home/acme/git/linux/tools/include/linux/refcount.h: In function ‘refcount_inc_not_zero’: /home/acme/git/linux/tools/include/linux/refcount.h:95:23: error: ‘UINT_MAX’ undeclared (first use in this function) REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n"); ^ /home/acme/git/linux/tools/include/linux/refcount.h:47:41: note: in definition of macro ‘REFCOUNT_WARN’ #define REFCOUNT_WARN(cond, str) (void)(cond) ^~~~ /home/acme/git/linux/tools/include/linux/refcount.h:95:23: note: each undeclared identifier is reported only once for each function it appears in REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n"); ^ /home/acme/git/linux/tools/include/linux/refcount.h:47:41: note: in definition of macro ‘REFCOUNT_WARN’ #define REFCOUNT_WARN(cond, str) (void)(cond)
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/dso.c | 6 +++--- tools/perf/util/dso.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c index 3abe337..f88aa44 100644 --- a/tools/perf/util/dso.c +++ b/tools/perf/util/dso.c @@ -1109,7 +1109,7 @@ struct dso *dso__new(const char *name) INIT_LIST_HEAD(&dso->node); INIT_LIST_HEAD(&dso->data.open_entry); pthread_mutex_init(&dso->lock, NULL);
atomic_set(&dso->refcnt, 1);
refcount_set(&dso->refcnt, 1);}
return dso;
@@ -1147,13 +1147,13 @@ void dso__delete(struct dso *dso) struct dso *dso__get(struct dso *dso) { if (dso)
atomic_inc(&dso->refcnt);
refcount_inc(&dso->refcnt);}
void dso__put(struct dso *dso) {
- if (dso && atomic_dec_and_test(&dso->refcnt))
- if (dso && refcount_dec_and_test(&dso->refcnt)) dso__delete(dso);
}
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h index ecc4bbd..12350b1 100644 --- a/tools/perf/util/dso.h +++ b/tools/perf/util/dso.h @@ -1,7 +1,7 @@ #ifndef __PERF_DSO #define __PERF_DSO
-#include <linux/atomic.h> +#include <linux/refcount.h> #include <linux/types.h> #include <linux/rbtree.h> #include <sys/types.h> @@ -187,7 +187,7 @@ struct dso { void *priv; u64 db_id; };
- atomic_t refcnt;
- refcount_t refcnt; char name[0];
};
-- 2.7.4
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/map.c | 6 +++--- tools/perf/util/map.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c index 0a943e7..f0e2428 100644 --- a/tools/perf/util/map.c +++ b/tools/perf/util/map.c @@ -141,7 +141,7 @@ void map__init(struct map *map, enum map_type type, RB_CLEAR_NODE(&map->rb_node); map->groups = NULL; map->erange_warned = false; - atomic_set(&map->refcnt, 1); + refcount_set(&map->refcnt, 1); }
struct map *map__new(struct machine *machine, u64 start, u64 len, @@ -255,7 +255,7 @@ void map__delete(struct map *map)
void map__put(struct map *map) { - if (map && atomic_dec_and_test(&map->refcnt)) + if (map && refcount_dec_and_test(&map->refcnt)) map__delete(map); }
@@ -354,7 +354,7 @@ struct map *map__clone(struct map *from) struct map *map = memdup(from, sizeof(*map));
if (map != NULL) { - atomic_set(&map->refcnt, 1); + refcount_set(&map->refcnt, 1); RB_CLEAR_NODE(&map->rb_node); dso__get(map->dso); map->groups = NULL; diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h index abdacf8..9545ff3 100644 --- a/tools/perf/util/map.h +++ b/tools/perf/util/map.h @@ -1,7 +1,7 @@ #ifndef __PERF_MAP_H #define __PERF_MAP_H
-#include <linux/atomic.h> +#include <linux/refcount.h> #include <linux/compiler.h> #include <linux/list.h> #include <linux/rbtree.h> @@ -51,7 +51,7 @@ struct map {
struct dso *dso; struct map_groups *groups; - atomic_t refcnt; + refcount_t refcnt; };
struct kmap { @@ -150,7 +150,7 @@ struct map *map__clone(struct map *map); static inline struct map *map__get(struct map *map) { if (map) - atomic_inc(&map->refcnt); + refcount_inc(&map->refcnt); return map; }
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/map.c | 4 ++-- tools/perf/util/map.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c index f0e2428..1d9ebcf 100644 --- a/tools/perf/util/map.c +++ b/tools/perf/util/map.c @@ -485,7 +485,7 @@ void map_groups__init(struct map_groups *mg, struct machine *machine) maps__init(&mg->maps[i]); } mg->machine = machine; - atomic_set(&mg->refcnt, 1); + refcount_set(&mg->refcnt, 1); }
static void __maps__purge(struct maps *maps) @@ -547,7 +547,7 @@ void map_groups__delete(struct map_groups *mg)
void map_groups__put(struct map_groups *mg) { - if (mg && atomic_dec_and_test(&mg->refcnt)) + if (mg && refcount_dec_and_test(&mg->refcnt)) map_groups__delete(mg); }
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h index 9545ff3..c8a5a64 100644 --- a/tools/perf/util/map.h +++ b/tools/perf/util/map.h @@ -67,7 +67,7 @@ struct maps { struct map_groups { struct maps maps[MAP__NR_TYPES]; struct machine *machine; - atomic_t refcnt; + refcount_t refcnt; };
struct map_groups *map_groups__new(struct machine *machine); @@ -77,7 +77,7 @@ bool map_groups__empty(struct map_groups *mg); static inline struct map_groups *map_groups__get(struct map_groups *mg) { if (mg) - atomic_inc(&mg->refcnt); + refcount_inc(&mg->refcnt); return mg; }
Em Tue, Feb 21, 2017 at 05:35:00PM +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.
You missed tools/perf/tests/thread-mg-share.c
I fixed it up.
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/map.c | 4 ++-- tools/perf/util/map.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c index f0e2428..1d9ebcf 100644 --- a/tools/perf/util/map.c +++ b/tools/perf/util/map.c @@ -485,7 +485,7 @@ void map_groups__init(struct map_groups *mg, struct machine *machine) maps__init(&mg->maps[i]); } mg->machine = machine;
- atomic_set(&mg->refcnt, 1);
- refcount_set(&mg->refcnt, 1);
}
static void __maps__purge(struct maps *maps) @@ -547,7 +547,7 @@ void map_groups__delete(struct map_groups *mg)
void map_groups__put(struct map_groups *mg) {
- if (mg && atomic_dec_and_test(&mg->refcnt))
- if (mg && refcount_dec_and_test(&mg->refcnt)) map_groups__delete(mg);
}
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h index 9545ff3..c8a5a64 100644 --- a/tools/perf/util/map.h +++ b/tools/perf/util/map.h @@ -67,7 +67,7 @@ struct maps { struct map_groups { struct maps maps[MAP__NR_TYPES]; struct machine *machine;
- atomic_t refcnt;
- refcount_t refcnt;
};
struct map_groups *map_groups__new(struct machine *machine); @@ -77,7 +77,7 @@ bool map_groups__empty(struct map_groups *mg); static inline struct map_groups *map_groups__get(struct map_groups *mg) { if (mg)
atomic_inc(&mg->refcnt);
refcount_inc(&mg->refcnt);}
-- 2.7.4
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/evlist.c | 18 +++++++++--------- tools/perf/util/evlist.h | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index b601f28..564b924 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -777,7 +777,7 @@ union perf_event *perf_mmap__read_forward(struct perf_mmap *md, bool check_messu /* * Check if event was unmapped due to a POLLHUP/POLLERR. */ - if (!atomic_read(&md->refcnt)) + if (!refcount_read(&md->refcnt)) return NULL;
head = perf_mmap__read_head(md); @@ -794,7 +794,7 @@ perf_mmap__read_backward(struct perf_mmap *md) /* * Check if event was unmapped due to a POLLHUP/POLLERR. */ - if (!atomic_read(&md->refcnt)) + if (!refcount_read(&md->refcnt)) return NULL;
head = perf_mmap__read_head(md); @@ -856,7 +856,7 @@ void perf_mmap__read_catchup(struct perf_mmap *md) { u64 head;
- if (!atomic_read(&md->refcnt)) + if (!refcount_read(&md->refcnt)) return;
head = perf_mmap__read_head(md); @@ -875,14 +875,14 @@ static bool perf_mmap__empty(struct perf_mmap *md)
static void perf_mmap__get(struct perf_mmap *map) { - atomic_inc(&map->refcnt); + refcount_inc(&map->refcnt); }
static void perf_mmap__put(struct perf_mmap *md) { - BUG_ON(md->base && atomic_read(&md->refcnt) == 0); + BUG_ON(md->base && refcount_read(&md->refcnt) == 0);
- if (atomic_dec_and_test(&md->refcnt)) + if (refcount_dec_and_test(&md->refcnt)) perf_mmap__munmap(md); }
@@ -894,7 +894,7 @@ void perf_mmap__consume(struct perf_mmap *md, bool overwrite) perf_mmap__write_tail(md, old); }
- if (atomic_read(&md->refcnt) == 1 && perf_mmap__empty(md)) + if (refcount_read(&md->refcnt) == 1 && perf_mmap__empty(md)) perf_mmap__put(md); }
@@ -937,7 +937,7 @@ static void perf_mmap__munmap(struct perf_mmap *map) munmap(map->base, perf_mmap__mmap_len(map)); map->base = NULL; map->fd = -1; - atomic_set(&map->refcnt, 0); + refcount_set(&map->refcnt, 0); } auxtrace_mmap__munmap(&map->auxtrace_mmap); } @@ -1001,7 +1001,7 @@ static int perf_mmap__mmap(struct perf_mmap *map, * evlist layer can't just drop it when filtering events in * perf_evlist__filter_pollfd(). */ - atomic_set(&map->refcnt, 2); + refcount_set(&map->refcnt, 2); map->prev = 0; map->mask = mp->mask; map->base = mmap(NULL, perf_mmap__mmap_len(map), mp->prot, diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h index 389b9cc..3994299 100644 --- a/tools/perf/util/evlist.h +++ b/tools/perf/util/evlist.h @@ -1,7 +1,7 @@ #ifndef __PERF_EVLIST_H #define __PERF_EVLIST_H 1
-#include <linux/atomic.h> +#include <linux/refcount.h> #include <linux/list.h> #include <api/fd/array.h> #include <stdio.h> @@ -29,7 +29,7 @@ struct perf_mmap { void *base; int mask; int fd; - atomic_t refcnt; + refcount_t refcnt; u64 prev; struct auxtrace_mmap auxtrace_mmap; char event_copy[PERF_SAMPLE_MAX_SIZE] __attribute__((aligned(8)));
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.c | 6 +++--- tools/perf/util/thread.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c index f5af87f..74e79d2 100644 --- a/tools/perf/util/thread.c +++ b/tools/perf/util/thread.c @@ -53,7 +53,7 @@ struct thread *thread__new(pid_t pid, pid_t tid) goto err_thread;
list_add(&comm->list, &thread->comm_list); - atomic_set(&thread->refcnt, 1); + refcount_set(&thread->refcnt, 1); RB_CLEAR_NODE(&thread->rb_node); }
@@ -88,13 +88,13 @@ void thread__delete(struct thread *thread) struct thread *thread__get(struct thread *thread) { if (thread) - atomic_inc(&thread->refcnt); + refcount_inc(&thread->refcnt); return thread; }
void thread__put(struct thread *thread) { - if (thread && atomic_dec_and_test(&thread->refcnt)) { + if (thread && refcount_dec_and_test(&thread->refcnt)) { /* * Remove it from the dead_threads list, as last reference * is gone. diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h index 99263cb..e571885 100644 --- a/tools/perf/util/thread.h +++ b/tools/perf/util/thread.h @@ -1,7 +1,7 @@ #ifndef __PERF_THREAD_H #define __PERF_THREAD_H
-#include <linux/atomic.h> +#include <linux/refcount.h> #include <linux/rbtree.h> #include <linux/list.h> #include <unistd.h> @@ -23,7 +23,7 @@ struct thread { pid_t tid; pid_t ppid; int cpu; - atomic_t refcnt; + refcount_t refcnt; char shortname[3]; bool comm_set; int comm_len;
Em Tue, Feb 21, 2017 at 05:35:02PM +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.
You forgot this one, fixing it...
@@ -1439,7 +1439,7 @@ static void __machine__remove_thread(struct machine *machine, struct thread *th, if (machine->last_match == th) machine->last_match = NULL;
- BUG_ON(atomic_read(&th->refcnt) == 0); + BUG_ON(refcount_read(&th->refcnt) == 0); if (lock) pthread_rwlock_wrlock(&machine->threads_lock); rb_erase_init(&th->rb_node, &machine->threads); [acme@jouet linux]$
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.c | 6 +++--- tools/perf/util/thread.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c index f5af87f..74e79d2 100644 --- a/tools/perf/util/thread.c +++ b/tools/perf/util/thread.c @@ -53,7 +53,7 @@ struct thread *thread__new(pid_t pid, pid_t tid) goto err_thread;
list_add(&comm->list, &thread->comm_list);
atomic_set(&thread->refcnt, 1);
refcount_set(&thread->refcnt, 1);@@ -88,13 +88,13 @@ void thread__delete(struct thread *thread) struct thread *thread__get(struct thread *thread) { if (thread)
atomic_inc(&thread->refcnt);
refcount_inc(&thread->refcnt);}
void thread__put(struct thread *thread) {
- if (thread && atomic_dec_and_test(&thread->refcnt)) {
- if (thread && refcount_dec_and_test(&thread->refcnt)) { /*
- Remove it from the dead_threads list, as last reference
- is gone.
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h index 99263cb..e571885 100644 --- a/tools/perf/util/thread.h +++ b/tools/perf/util/thread.h @@ -1,7 +1,7 @@ #ifndef __PERF_THREAD_H #define __PERF_THREAD_H
-#include <linux/atomic.h> +#include <linux/refcount.h> #include <linux/rbtree.h> #include <linux/list.h> #include <unistd.h> @@ -23,7 +23,7 @@ struct thread { pid_t tid; pid_t ppid; int cpu;
- atomic_t refcnt;
- refcount_t refcnt; char shortname[3]; bool comm_set; int comm_len;
-- 2.7.4
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[]; };
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
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
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
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
I took a look on it and it looks good. Just one thing I want to double check with regards to this commit: https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d561...
And more specifically to this chunk:
@@ -937,7 +937,7 @@ munmap(map->base, perf_mmap__mmap_len(map)); map->base = NULL; map->fd = -1; - atomic_set(&map->refcnt, 0); + refcount_set(&map->refcnt, 0); } auxtrace_mmap__munmap(&map->auxtrace_mmap); }
So, when the refcount set to zero in this place, what exactly happens to the perf_map object after? I just want to double check that we don't have another hiding reusage case here when refcounter later on is simply incremented vs. set to "2."
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.
Thank you!
Best Regards, Elena.
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
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
I took a look on it and it looks good. Just one thing I want to double check with regards to this commit: https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d561...
And more specifically to this chunk:
@@ -937,7 +937,7 @@ munmap(map->base, perf_mmap__mmap_len(map)); map->base = NULL; map->fd = -1;
atomic_set(&map->refcnt, 0);
refcount_set(&map->refcnt, 0);}
So, when the refcount set to zero in this place, what exactly happens to the perf_map object after? I just want to double check that we don't have another hiding reusage case here when refcounter later on is simply incremented vs. set to "2."
So, this looks fishy and I don't recall why that was done that way, this conversion to refcnt_t and the debug associated with it provides a good opportunity for us to try to get that to a more familiar reference counting workflow, I'll take a look at it.
- Arnaldo
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.
Thank you!
Best Regards, Elena.
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
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
I took a look on it and it looks good. Just one thing I want to double check with regards to this commit: https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d561...
And more specifically to this chunk:
@@ -937,7 +937,7 @@ munmap(map->base, perf_mmap__mmap_len(map)); map->base = NULL; map->fd = -1;
atomic_set(&map->refcnt, 0);
refcount_set(&map->refcnt, 0);}
So, when the refcount set to zero in this place, what exactly happens to the perf_map object after? I just want to double check that we don't have another hiding reusage case here when refcounter later on is simply incremented vs. set to "2."
So, this is an odd use of a reference count, the patch below should help understand it?
Those perf_mmap objects are created in a batch fashion, it being zero just means it isn't yet mmaped at all, and we check for that before using it.
So, it remains a bug to do a dec for a zeroed refcount, and the refcount_t infrastructure will catch it, which helps tools/.
- Arnaldo
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 564b924fb48a..5a70f08d2518 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -974,8 +974,19 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist) if (!map) return NULL;
- for (i = 0; i < evlist->nr_mmaps; i++) + for (i = 0; i < evlist->nr_mmaps; i++) { map[i].fd = -1; + /* + * When the perf_mmap() call is made we grab one refcount, plus + * one extra to let perf_evlist__mmap_consume() get the last + * events after all real references (perf_mmap__get()) are + * dropped. + * + * Each PERF_EVENT_IOC_SET_OUTPUT points to this mmap and + * thus does perf_mmap__get() on it. + */ + refcount_set(&map[i].refcnt, 0); + } return map; }
@@ -988,6 +999,7 @@ struct mmap_params { static int perf_mmap__mmap(struct perf_mmap *map, struct mmap_params *mp, int fd) { + perf_mmap__get(map); /* * The last one will be done at perf_evlist__mmap_consume(), so that we * make sure we don't prevent tools from consuming every last event in @@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap *map, * evlist layer can't just drop it when filtering events in * perf_evlist__filter_pollfd(). */ - refcount_set(&map->refcnt, 2); + perf_mmap__get(map); /* This is not a dup, see the comment above! */ map->prev = 0; map->mask = mp->mask; map->base = mmap(NULL, perf_mmap__mmap_len(map), mp->prot,
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.
Thank you!
Best Regards, Elena.
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
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
I took a look on it and it looks good. Just one thing I want to double check
with regards to this commit:
https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
And more specifically to this chunk:
@@ -937,7 +937,7 @@ munmap(map->base,
perf_mmap__mmap_len(map));
map->base = NULL; map->fd = -1;
atomic_set(&map->refcnt, 0);
refcount_set(&map->refcnt, 0);}
So, when the refcount set to zero in this place, what exactly happens to the
perf_map object after?
I just want to double check that we don't have another hiding reusage case
here when refcounter later on is simply incremented vs. set to "2."
So, this is an odd use of a reference count, the patch below should help understand it?
Yes, it helps, indeed, but I think we have an issue here. See below inline in patch.
Those perf_mmap objects are created in a batch fashion, it being zero just means it isn't yet mmaped at all, and we check for that before using it.
So, it remains a bug to do a dec for a zeroed refcount, and the refcount_t infrastructure will catch it, which helps tools/.
- Arnaldo
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 564b924fb48a..5a70f08d2518 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -974,8 +974,19 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist) if (!map) return NULL;
- for (i = 0; i < evlist->nr_mmaps; i++)
- for (i = 0; i < evlist->nr_mmaps; i++) { map[i].fd = -1;
/** When the perf_mmap() call is made we grabone refcount, plus
* one extra to let perf_evlist__mmap_consume()get the last
* events after all real references(perf_mmap__get()) are
* dropped.** Each PERF_EVENT_IOC_SET_OUTPUT points tothis mmap and
* thus does perf_mmap__get() on it.*/refcount_set(&map[i].refcnt, 0);- } return map;
}
@@ -988,6 +999,7 @@ struct mmap_params { static int perf_mmap__mmap(struct perf_mmap *map, struct mmap_params *mp, int fd) {
- perf_mmap__get(map); /*
- The last one will be done at perf_evlist__mmap_consume(),
so that we * make sure we don't prevent tools from consuming every last event in @@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap *map, * evlist layer can't just drop it when filtering events in * perf_evlist__filter_pollfd(). */
- refcount_set(&map->refcnt, 2);
- perf_mmap__get(map); /* This is not a dup, see the comment
above! */
This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero. So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)
When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above.
Best Regards, Elena.
map->prev = 0; map->mask = mp->mask; map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
prot,
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.
Thank you!
Best Regards, Elena.
Em Fri, Feb 24, 2017 at 07:27:32AM +0000, Reshetova, Elena escreveu:
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
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
I took a look on it and it looks good. Just one thing I want to double check
with regards to this commit:
https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
And more specifically to this chunk:
@@ -937,7 +937,7 @@ munmap(map->base,
perf_mmap__mmap_len(map));
map->base = NULL; map->fd = -1;
atomic_set(&map->refcnt, 0);
refcount_set(&map->refcnt, 0);}
So, when the refcount set to zero in this place, what exactly happens to the
perf_map object after?
I just want to double check that we don't have another hiding reusage case
here when refcounter later on is simply incremented vs. set to "2."
So, this is an odd use of a reference count, the patch below should help understand it?
Yes, it helps, indeed, but I think we have an issue here. See below inline in patch.
Those perf_mmap objects are created in a batch fashion, it being zero just means it isn't yet mmaped at all, and we check for that before using it.
So, it remains a bug to do a dec for a zeroed refcount, and the refcount_t infrastructure will catch it, which helps tools/.
- Arnaldo
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 564b924fb48a..5a70f08d2518 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -974,8 +974,19 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist) if (!map) return NULL;
- for (i = 0; i < evlist->nr_mmaps; i++)
- for (i = 0; i < evlist->nr_mmaps; i++) { map[i].fd = -1;
/** When the perf_mmap() call is made we grabone refcount, plus
* one extra to let perf_evlist__mmap_consume()get the last
* events after all real references(perf_mmap__get()) are
* dropped.** Each PERF_EVENT_IOC_SET_OUTPUT points tothis mmap and
* thus does perf_mmap__get() on it.*/refcount_set(&map[i].refcnt, 0);- } return map;
}
@@ -988,6 +999,7 @@ struct mmap_params { static int perf_mmap__mmap(struct perf_mmap *map, struct mmap_params *mp, int fd) {
- perf_mmap__get(map); /*
- The last one will be done at perf_evlist__mmap_consume(),
so that we * make sure we don't prevent tools from consuming every last event in @@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap *map, * evlist layer can't just drop it when filtering events in * perf_evlist__filter_pollfd(). */
- refcount_set(&map->refcnt, 2);
- perf_mmap__get(map); /* This is not a dup, see the comment
above! */
This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero. So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)
Duh, thanks for pointint it out, will leave the comments, remove the pair of perf_mmap__get()
When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above.
Best Regards, Elena.
map->prev = 0; map->mask = mp->mask; map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
prot,
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.
Thank you!
Best Regards, Elena.
On Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova wrote:
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 ++--
This is userspace code; did you build this? I see a distinct lack of adding refcount.h to the userspace headers.
On Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova wrote:
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 ++--
This is userspace code; did you build this? I see a distinct lack of adding refcount.h to the userspace headers.
Oh, damn, indeed... We were approaching this in the whole kernel tree pile in the same way. I will fix, rebuild and resend. Sorry about this!
participants (4)
-
Arnaldo Carvalho de Melo -
Elena Reshetova -
Peter Zijlstra -
Reshetova, Elena