tools subsystem refcounter conversions
cover 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(-)
Signature · 1 line
2.7.4
PATCH 1/9 tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
refcount_t type and corresponding API should be used instead of atomic_t when the variable is used as a reference counter. This allows to avoid accidental refcounter overflows that might lead to use-after-free situations.
--- tools/perf/util/cgroup.c | 6 +++--- tools/perf/util/cgroup.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
tools/perf/util/cgroup.c +3 -3
@@ -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);
tools/perf/util/cgroup.h +2 -2
@@ -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; };
Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
tools/perf/util/cgroup.h line 4
Quoted 9 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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__tools/perf/util/cgroup.h +1
-1-#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 <[email protected]> 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
Quoted 12 lines
struct option;
struct
cgroup_sel{tools/perf/util/cgroup.h +1
-1char *name;int fd; -atomic_t refcnt; +refcount_t refcnt; };Signature · 2 lines
-- 2.7.4
RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
Quoted 19 lines
Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
tools/perf/util/cgroup.h line 4
Quoted 9 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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__tools/perf/util/cgroup.h +1
-1-#include <linux/atomic.h> +#include <linux/refcount.h> So this is the first one, I was expecting the copy from
include/linux/refcount.hto be made totools/include/linux/refcount.h, as was done fortools/include/linux/atomic.hand all the other stuff intools/include/See:
commit
c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3f
Quoted 8 lines
Author: Arnaldo Carvalho de Melo <[email protected]> Date: Mon Jul 11 10:28:48 2016 -0300tools: Add copy of
perf_event.htotools/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.
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
Em Wed, Feb 22, 2017 at 02:29:18PM +0000, Reshetova, Elena escreveu:
Quoted 28 lines
Quoted 19 lines
Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
tools/perf/util/cgroup.h line 4
Quoted 9 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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__tools/perf/util/cgroup.h +1
-1-#include <linux/atomic.h> +#include <linux/refcount.h> So this is the first one, I was expecting the copy from
include/linux/refcount.hto be made totools/include/linux/refcount.h, as was done fortools/include/linux/atomic.hand all the other stuff intools/include/See:
commit
c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3fQuoted 8 lines
Author: Arnaldo Carvalho de Melo <[email protected]> Date: Mon Jul 11 10:28:48 2016 -0300tools: Add copy of
perf_event.htotools/include/linux/--------------
For one of the reasons we've been doing this.
Quoted 8 lines
Hm.. I have taken a look on it and I am confused.
refcount.his not exactly standalone header and seems to bring in quite some many dependencies to other headers (linux/bug.h,linux/mutex.hetc.), which are not present in tools headers dirs.I tried to compile perf tool as a start, copied the
refcount.htotools/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
RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
Quoted 34 lines
Em Wed, Feb 22, 2017 at 02:29:18PM +0000, Reshetova, Elena escreveu:
Quoted 32 lines
Quoted 19 lines
Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
tools/perf/util/cgroup.h line 4
Quoted 9 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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__tools/perf/util/cgroup.h +1
-1-#include <linux/atomic.h> +#include <linux/refcount.h> So this is the first one, I was expecting the copy from
include/linux/refcount.hto be made totools/include/linux/refcount.h, as was done fortools/include/linux/atomic.hand all the other stuff intools/include/See:
commit
c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3fQuoted 8 lines
Author: Arnaldo Carvalho de Melo <[email protected]> Date: Mon Jul 11 10:28:48 2016 -0300tools: Add copy of
perf_event.htotools/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.his not exactly standalone header and seems to bring in quite some many dependencies to other headers (linux/bug.h,linux/mutex.hetc.), which are not present in tools headers dirs.
Quoted 8 lines
Quoted 3 lines
I tried to compile perf tool as a start, copied the
refcount.htotools/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/usesatomic.h, and uses that for refcounting, but not all ofinclude/linux/refcount.hshould be copied totools/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.
Quoted 4 lines
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.
Quoted 2 lines
- Arnaldo
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
Em Wed, Feb 22, 2017 at 04:10:59PM +0000, Reshetova, Elena escreveu:
Quoted 43 lines
Quoted 34 lines
Em Wed, Feb 22, 2017 at 02:29:18PM +0000, Reshetova, Elena escreveu:
Quoted 32 lines
Quoted 19 lines
Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
tools/perf/util/cgroup.h line 4
Quoted 9 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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__tools/perf/util/cgroup.h +1
-1-#include <linux/atomic.h> +#include <linux/refcount.h> So this is the first one, I was expecting the copy from
include/linux/refcount.hto be made totools/include/linux/refcount.h, as was done fortools/include/linux/atomic.hand all the other stuff intools/include/See:
commit
c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3fQuoted 8 lines
Author: Arnaldo Carvalho de Melo <[email protected]> Date: Mon Jul 11 10:28:48 2016 -0300tools: Add copy of
perf_event.htotools/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.his not exactly standalone header and seems to bring in quite some many dependencies to other headers (linux/bug.h,linux/mutex.hetc.), which are not present in tools headers dirs.Quoted 8 lines
Quoted 3 lines
I tried to compile perf tool as a start, copied the
refcount.htotools/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/usesatomic.h, and uses that for refcounting, but not all ofinclude/linux/refcount.hshould be copied totools/include/linux/refcount.h.
Quoted 5 lines
Oh, this is a good hint. Actually when I drop the
*_lockand*_mutex_lockfunctions (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...
Quoted 9 lines
Quoted 3 lines
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
RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
Quoted 70 lines
Em Wed, Feb 22, 2017 at 04:10:59PM +0000, Reshetova, Elena escreveu:
Quoted 43 lines
Quoted 34 lines
Em Wed, Feb 22, 2017 at 02:29:18PM +0000, Reshetova, Elena escreveu:
Quoted 32 lines
Quoted 19 lines
Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu:
tools/perf/util/cgroup.h line 4
Quoted 9 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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__tools/perf/util/cgroup.h +1
-1-#include <linux/atomic.h> +#include <linux/refcount.h> So this is the first one, I was expecting the copy from
include/linux/refcount.hto be made totools/include/linux/refcount.h, as was done fortools/include/linux/atomic.hand all the other stuff intools/include/See:
commit
c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3fQuoted 8 lines
Author: Arnaldo Carvalho de Melo <[email protected]> Date: Mon Jul 11 10:28:48 2016 -0300tools: Add copy of
perf_event.htotools/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.his not exactly standalone header and seems to bring in quite some many dependencies to other headers (linux/bug.h,linux/mutex.hetc.), which are not present in tools headers dirs.Quoted 8 lines
Quoted 3 lines
I tried to compile perf tool as a start, copied the
refcount.htotools/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/usesatomic.h, and uses that for refcounting, but not all ofinclude/linux/refcount.hshould be copied totools/include/linux/refcount.h.Quoted 5 lines
Oh, this is a good hint. Actually when I drop the
*_lockand*_mutex_lockfunctions (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...Quoted 9 lines
Quoted 3 lines
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.hfor which you are not adding the relevant headers, likeUINT_MAX, that is defined inlinux/kernel.h, but that file is not included inrefcount.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.
Quoted 3 lines
The right thing tho is to fix
linux/refcount.h(and then its trimmed down copy intools/) 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.
Quoted 4 lines
Anyway, I'll put this in a branch later so that you can take a look.
- Arnaldo
tip-bot for Elena Reshetova
merge bot[tip:perf/core] perf cgroup: Convert cgroup_sel.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovaperf cgroup: Convert cgroup_sel.refcnt from atomic_t to refcount_t
Commit-ID: 79c5fe6db8c70558d3a64959f55596d137ccc6e6 Gitweb: http://git.kernel.org/tip/79c5fe6db8c70558d3a64959f55596d137ccc6e6 Author: Elena Reshetova <[email protected]> AuthorDate: Tue, 21 Feb 2017 17:34:55 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Fri, 3 Mar 2017 19:07:14 -0300
perf cgroup: Convert cgroup_sel.refcnt from atomic_t to refcount_t
The 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.
--- tools/perf/util/cgroup.c | 6 +++--- tools/perf/util/cgroup.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
tools/perf/util/cgroup.c +3 -3
@@ -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);
tools/perf/util/cgroup.h +2 -2
@@ -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; };
PATCH 2/9 tools: convert cpu_map.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovarefcount_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.
--- tools/perf/util/cpumap.c | 16 ++++++++-------- tools/perf/util/cpumap.h | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-)
tools/perf/util/cpumap.c +8 -8
@@ -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 ; }
tools/perf/util/cpumap.h +2 -2
@@ -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[]; };
Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 2/9] tools: convert cpu_map.refcnt from atomic_t to refcount_t
Em Tue, Feb 21, 2017 at 05:34:56PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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:
tools/perf/tests/cpumap.c +1 -1
@@ -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 ; }
tools/perf/util/cpumap.h +1 -1
@@ -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;
Quoted 105 lines
Signed-off-by Elena Reshetova <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>--- tools/perf/util/cpumap.c | 16 ++++++++-------- tools/perf/util/cpumap.h | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-)tools/perf/util/cpumap.c +8
-8@@ -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 ; }tools/perf/util/cpumap.h +2
-2@@ -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[]; };Signature · 1 line
2.7.4
PATCH 3/9 tools: convert comm_str.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovarefcount_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.
--- tools/perf/util/comm.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
tools/perf/util/comm.c +5 -8
@@ -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;
Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t
Em Tue, Feb 21, 2017 at 05:34:57PM +0200, Elena Reshetova escreveu:
Quoted 10 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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 <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>
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_pluginsutil/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
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t
Em Wed, Feb 22, 2017 at 05:33:50PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 42 lines
Em Tue, Feb 21, 2017 at 05:34:57PM +0200, Elena Reshetova escreveu:
Quoted 10 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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 <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>You are doing two things (well three) things here:
1. converting to
refcnt.h2. 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.hdiffers from kernel
CC/tmp/build/perf/util/comm.o
INSTALLtrace_pluginsutil/comm.c:16:25: error: ‘comm_str__get’ defined but not used [-Werror=unused-function]
static structcomm_str*comm_str__get(structcomm_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]$
Quoted 4 lines
3) not test building your patches :-\
I'll let this pass this time, minor, I am removing the now unused
comm_str__get()function.
But it can't get unused, because the comm_str__findnew() may return an existing entry, that _needs_ to get its refcount bumped, that is the reason for this refcount to be there... reinstating it:
#0 0x00007ffff522491f in raise () from /lib64/libc.so.6 #1 0x00007ffff522651a in abort () from /lib64/libc.so.6 #2 0x00007ffff5268200 in __libc_message () from /lib64/libc.so.6 #3 0x00007ffff527188a in _int_free () from /lib64/libc.so.6 #4 0x00007ffff52752bc in free () from /lib64/libc.so.6 #5 0x000000000051125f in comm_str__put (cs=0x35038e0) at util/comm.c:20 #6 0x00000000005115b3 in comm__free (comm=0x6f4ee90) at util/comm.c:113 #7 0x0000000000511e10 in thread__delete (thread=0x6f4ee10) at util/thread.c:81 #8 0x0000000000511f0e in thread__put (thread=0x6f4ee10) at util/thread.c:103 #9 0x0000000000504ea6 in machine__process_fork_event (machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at util/machine.c:1496
#10 0x0000000000505092 in machine__process_event (machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at util/machine.c:1544 #11 0x0000000000451ae9 in perf_top__mmap_read_idx (top=0x7fffffffa7c0, idx=3) at builtin-top.c:844
#12 0x0000000000451bb6 in perf_top__mmap_read (top=0x7fffffffa7c0) at builtin-top.c:857
#13 0x0000000000452229 in __cmd_top (top=0x7fffffffa7c0) at builtin-top.c:1002
#14 0x00000000004536a3 in cmd_top (argc=0, argv=0x7fffffffe150, prefix=0x0) at builtin-top.c:1332
#15 0x00000000004b82a8 in run_builtin (p=0xa17cd0 <commands+336>, argc=4, argv=0x7fffffffe150) at perf.c:359
#16 0x00000000004b8515 in handle_internal_command (argc=4, argv=0x7fffffffe150) at perf.c:421
#17 0x00000000004b865a in run_argv (argcp=0x7fffffffdf9c, argv=0x7fffffffdf90) at perf.c:467
#18 0x00000000004b8a5d in main (argc=4, argv=0x7fffffffe150) at perf.c:614
And this brings us to my learning experience, i.e. this should've been caught by this machinery, right? But that only if I leaked this object, right?
I need to read more on this, that is for sure ;-)
- Arnaldo
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t
Em Wed, Feb 22, 2017 at 07:20:45PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 72 lines
Em Wed, Feb 22, 2017 at 05:33:50PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 42 lines
Em Tue, Feb 21, 2017 at 05:34:57PM +0200, Elena Reshetova escreveu:
Quoted 10 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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 <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>You are doing two things (well three) things here:
1. converting to
refcnt.h2. 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.hdiffers from kernel
CC/tmp/build/perf/util/comm.o
INSTALLtrace_pluginsutil/comm.c:16:25: error: ‘comm_str__get’ defined but not used [-Werror=unused-function]
static structcomm_str*comm_str__get(structcomm_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]$Quoted 4 lines
3) not test building your patches :-\
I'll let this pass this time, minor, I am removing the now unused
comm_str__get()function.But it can't get unused, because the
comm_str__findnew()may return an existing entry, that _needs_ to get its refcount bumped, that is the reason for this refcount to be there... reinstating it:#0 0x00007ffff522491f in raise () from /lib64/libc.so.6 #1 0x00007ffff522651a in abort () from /lib64/libc.so.6 #2 0x00007ffff5268200 in __libc_message () from /lib64/libc.so.6 #3 0x00007ffff527188a in _int_free () from /lib64/libc.so.6 #4 0x00007ffff52752bc in free () from /lib64/libc.so.6 #5 0x000000000051125f in comm_str__put (cs=0x35038e0) at util/comm.c:20 #6 0x00000000005115b3 in comm__free (comm=0x6f4ee90) at util/comm.c:113 #7 0x0000000000511e10 in thread__delete (thread=0x6f4ee10) at util/thread.c:81 #8 0x0000000000511f0e in thread__put (thread=0x6f4ee10) at util/thread.c:103 #9 0x0000000000504ea6 in machine__process_fork_event (machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at util/machine.c:1496#10
0x0000000000505092inmachine__process_event(machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) atutil/machine.c:1544#110x0000000000451ae9inperf_top__mmap_read_idx(top=0x7fffffffa7c0, idx=3) at builtin-top.c:844
#120x0000000000451bb6inperf_top__mmap_read(top=0x7fffffffa7c0) at builtin-top.c:857
#130x0000000000452229in__cmd_top(top=0x7fffffffa7c0) at builtin-top.c:1002
#140x00000000004536a3incmd_top(argc=0, argv=0x7fffffffe150, prefix=0x0) at builtin-top.c:1332
#150x00000000004b82a8inrun_builtin(p=0xa17cd0 <commands+336>, argc=4, argv=0x7fffffffe150) at perf.c:359
#160x00000000004b8515inhandle_internal_command(argc=4, argv=0x7fffffffe150) at perf.c:421
#170x00000000004b865ainrun_argv(argcp=0x7fffffffdf9c, argv=0x7fffffffdf90) at perf.c:467
#180x00000000004b8a5din main (argc=4, argv=0x7fffffffe150) at perf.c:614
Quoted 4 lines
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]$
RE: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t
Quoted 34 lines
Em Wed, Feb 22, 2017 at 07:20:45PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 1 line
Em Wed, Feb 22, 2017 at 05:33:50PM -0300, Arnaldo Carvalho de Melo
escreveu:
Quoted 18 lines
Quoted 18 lines
Em Tue, Feb 21, 2017 at 05:34:57PM +0200, Elena Reshetova escreveu:
Quoted 10 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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 <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>You are doing two things (well three) things here:
1. converting to
refcnt.h2. Initiationg the refcount to 1, which makes this take place:
Quoted 7 lines
Quoted 7 lines
[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.hdiffers from kernel
CC/tmp/build/perf/util/comm.o
INSTALLtrace_pluginsutil/comm.c:16:25: error: ‘comm_str__get’ defined but not used [-Werror=unused-function]
Quoted 4 lines
Quoted 4 lines
static struct
comm_str*comm_str__get(structcomm_str*cs)^~~~~~~~~~~~~cc1: all warnings being treated as errors
mv: cannot stat '/tmp/build/perf/util/.comm.o.tmp': No such file or
Quoted 17 lines
directory
Quoted 1 line
Quoted 1 line
/home/acme/git/linux/tools/build/Makefile.build:101: recipe for target'
/tmp/build/perf/util/comm.o' failedQuoted 2 lines
Quoted 2 lines
make[4]: *** [
/tmp/build/perf/util/comm.o] Error 1/home/acme/git/linux/tools/build/Makefile.build:144: recipe for target'util' failed
Quoted 11 lines
Quoted 11 lines
make[3]: *** [util] Error 2
Makefile.perf:523: recipe for target '/tmp/build/perf/libperf-in.o' failed make[2]: *** [/tmp/build/perf/libperf-in.o] Error 2 Makefile.perf:204: recipe for target 'sub-make' failed make[1]: *** [sub-make] Error 2
Makefile:108: recipe for target 'install-bin' failed
make: *** [install-bin] Error 2
make: Leaving directory '/home/acme/git/linux/tools/perf' [acme@jouet linux]$3) not test building your patches :-\
Sorry about compilation errors: I totally forgot that tools is not getting compiled automatically when you build the whole tree with all configs on, so these patches really slipped through untested.
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
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.
Quoted 24 lines
Quoted 8 lines
#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:113Quoted 1 line
#7
0x0000000000511e10inthread__delete(thread=0x6f4ee10) at
util/thread.c:81Quoted 1 line
#8
0x0000000000511f0einthread__put(thread=0x6f4ee10) at
util/thread.c:103Quoted 1 line
#9
0x0000000000504ea6inmachine__process_fork_event(machine=0x21f4bf8, event=0x7fffed6b54a0, sample=0x7fffffff8420) at
util/machine.c:1496Quoted 1 line
#10
0x0000000000505092inmachine__process_event(machine=0x21f4bf8,event=0x7fffed6b54a0, sample=0x7fffffff8420) at
util/machine.c:1544Quoted 1 line
#11
0x0000000000451ae9inperf_top__mmap_read_idx(top=0x7fffffffa7c0,idx=3) at builtin-top.c:844
Quoted 1 line
#12
0x0000000000451bb6inperf_top__mmap_read(top=0x7fffffffa7c0) atbuiltin-top.c:857
Quoted 1 line
#13
0x0000000000452229in__cmd_top(top=0x7fffffffa7c0) at builtin-top.c:1002
Quoted 14 lines
Quoted 1 line
#14
0x00000000004536a3incmd_top(argc=0, argv=0x7fffffffe150,prefix=0x0) at builtin-top.c:1332
Quoted 1 line
#15
0x00000000004b82a8inrun_builtin(p=0xa17cd0 <commands+336>,argc=4, argv=0x7fffffffe150) at perf.c:359
Quoted 1 line
#16
0x00000000004b8515inhandle_internal_command(argc=4,argv=0x7fffffffe150) at perf.c:421
Quoted 1 line
#17
0x00000000004b865ainrun_argv(argcp=0x7fffffffdf9c,argv=0x7fffffffdf90) at perf.c:467
Quoted 6 lines
#18
0x00000000004b8a5din main (argc=4, argv=0x7fffffffe150) at perf.c:614And 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.
Quoted 28 lines
For reference, this is the patch on top of this:
+++
b/tools/perf/util/comm.c(quoted diff) +8
-1@@ -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;
Quoted 1 line
[acme@jouet linux]$
This looks correct now.
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t
Em Thu, Feb 23, 2017 at 09:16:07AM +0000, Reshetova, Elena escreveu:
Quoted 10 lines
Quoted 6 lines
Em Wed, Feb 22, 2017 at 07:20:45PM -0300, Arnaldo Carvalho de Melo
Quoted 5 lines
Quoted 5 lines
make: *** [install-bin] Error 2
make: Leaving directory '/home/acme/git/linux/tools/perf' [acme@jouet linux]$3) not test building your patches :-\
Sorry about compilation errors: I totally forgot that tools is not getting compiled automatically when you build the whole tree with all configs on, so these patches really slipped through untested.
np, each component in the tree has its own idiosyncrasies, its really difficult to test something so sweeping like this change, thanks again for doing this work, use-after-free is evil, we should do more things like this :-)
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
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:
Quoted 6 lines
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_ttorefcount_tconversions 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.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
#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) atAnd 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?
Quoted 6 lines
Quoted 1 line
Quoted 1 line
I need to read more on this, that is for sure ;-)
The way how current
refcount_timplemented 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...
Quoted 4 lines
Quoted 4 lines
For reference, this is the patch on top of this:
+++
b/tools/perf/util/comm.c(quoted diff) +0
-0@@ -13,6 +13,13 @@ struct comm_str {
<SNIP>
Quoted 1 line
This looks correct now.
Thanks for checking,
- Arnaldo
PATCH 4/9 tools: convert dso.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovarefcount_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.
--- tools/perf/util/dso.c | 6 +++--- tools/perf/util/dso.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
tools/perf/util/dso.c +3 -3
@@ -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); }
tools/perf/util/dso.h +2 -2
@@ -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 ]; };
Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 4/9] tools: convert dso.refcnt from atomic_t to refcount_t
Em Tue, Feb 21, 2017 at 05:34:58PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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)
Quoted 62 lines
Signed-off-by Elena Reshetova <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>--- tools/perf/util/dso.c | 6 +++--- tools/perf/util/dso.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)tools/perf/util/dso.c +3
-3@@ -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); }tools/perf/util/dso.h +2
-2@@ -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 ]; };Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 4/9] tools: convert dso.refcnt from atomic_t to refcount_t
Em Wed, Feb 22, 2017 at 05:37:46PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 8 lines
Em Tue, Feb 21, 2017 at 05:34:58PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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.hto 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.
Quoted 79 lines
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,
fromtests/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’
#defineREFCOUNT_WARN(cond, str)(void)(cond)Quoted 62 lines
Signed-off-by Elena Reshetova <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>--- tools/perf/util/dso.c | 6 +++--- tools/perf/util/dso.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)tools/perf/util/dso.c +3
-3@@ -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); }tools/perf/util/dso.h +2
-2@@ -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 ]; };Signature · 1 line
2.7.4
tip-bot for Elena Reshetova
merge bot[tip:perf/core] perf dso: Convert dso.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovaperf dso: Convert dso.refcnt from atomic_t to refcount_t
Commit-ID: 7100810a75b9854f1b05550b54500497c5914d4b Gitweb: http://git.kernel.org/tip/7100810a75b9854f1b05550b54500497c5914d4b Author: Elena Reshetova <[email protected]> AuthorDate: Tue, 21 Feb 2017 17:34:58 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Fri, 3 Mar 2017 19:07:15 -0300
perf dso: Convert dso.refcnt from atomic_t to refcount_t
The 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.
--- tools/perf/util/dso.c | 6 +++--- tools/perf/util/dso.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
tools/perf/util/dso.c +3 -3
@@ -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); }
tools/perf/util/dso.h +2 -2
@@ -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 ]; };
PATCH 5/9 tools: convert map.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovarefcount_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.
--- tools/perf/util/map.c | 6 +++--- tools/perf/util/map.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)
tools/perf/util/map.c +3 -3
@@ -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 ;
tools/perf/util/map.h +3 -3
@@ -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; }
Signature · 1 line
2.7.4
tip-bot for Elena Reshetova
merge bot[tip:perf/core] perf map: Convert map.refcnt from atomic_t to refcount_t
perf map: Convert map.refcnt from atomic_t to refcount_t
Commit-ID: e3a42cdd3e35d6c2181d5acfa191eb448aea6ace Gitweb: http://git.kernel.org/tip/e3a42cdd3e35d6c2181d5acfa191eb448aea6ace Author: Elena Reshetova <[email protected]> AuthorDate: Tue, 21 Feb 2017 17:34:59 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Fri, 3 Mar 2017 19:07:15 -0300
perf map: Convert map.refcnt from atomic_t to refcount_t
The 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.
--- tools/perf/util/map.c | 6 +++--- tools/perf/util/map.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)
tools/perf/util/map.c +3 -3
@@ -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 ;
tools/perf/util/map.h +3 -3
@@ -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; }
PATCH 6/9 tools: convert map_groups.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovarefcount_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.
--- tools/perf/util/map.c | 4 ++-- tools/perf/util/map.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
tools/perf/util/map.c +2 -2
@@ -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); }
tools/perf/util/map.h +2 -2
@@ -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; }
Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 6/9] tools: convert map_groups.refcnt from atomic_t to refcount_t
Em Tue, Feb 21, 2017 at 05:35:00PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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.
Quoted 55 lines
Signed-off-by Elena Reshetova <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>--- tools/perf/util/map.c | 4 ++-- tools/perf/util/map.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)tools/perf/util/map.c +2
-2@@ -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); }tools/perf/util/map.h +2
-2@@ -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; }Signature · 1 line
2.7.4
PATCH 7/9 tools: convert perf_map.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovarefcount_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.
--- tools/perf/util/evlist.c | 18 +++++++++--------- tools/perf/util/evlist.h | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-)
tools/perf/util/evlist.c +9 -9
@@ -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,
tools/perf/util/evlist.h +2 -2
@@ -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)));
Signature · 1 line
2.7.4
PATCH 8/9 tools: convert thread.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovarefcount_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.
--- tools/perf/util/thread.c | 6 +++--- tools/perf/util/thread.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
tools/perf/util/thread.c +3 -3
@@ -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.
tools/perf/util/thread.h +2 -2
@@ -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;
Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 8/9] tools: convert thread.refcnt from atomic_t to refcount_t
Em Tue, Feb 21, 2017 at 05:35:02PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
refcount_ttype and corresponding API should be used instead ofatomic_twhen 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]$
Quoted 62 lines
Signed-off-by Elena Reshetova <[email protected]>Signed-off-by Hans Liljestrand <[email protected]>Signed-off-by Kees Cook <[email protected]>Signed-off-by David Windsor <[email protected]>--- tools/perf/util/thread.c | 6 +++--- tools/perf/util/thread.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)tools/perf/util/thread.c +3
-3@@ -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. tools/perf/util/thread.h +2
-2@@ -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;Signature · 1 line
2.7.4
tip-bot for Elena Reshetova
merge bot[tip:perf/core] perf thread: convert thread.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovaperf thread: convert thread.refcnt from atomic_t to refcount_t
Commit-ID: e34f5b11cd51fbe723e481c1db03a77260be6f4c Gitweb: http://git.kernel.org/tip/e34f5b11cd51fbe723e481c1db03a77260be6f4c Author: Elena Reshetova <[email protected]> AuthorDate: Tue, 21 Feb 2017 17:35:02 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Fri, 3 Mar 2017 19:07:16 -0300
perf thread: convert thread.refcnt from atomic_t to refcount_t
The 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.
--- tools/perf/util/machine.c | 2 +- tools/perf/util/thread.c | 6 +++--- tools/perf/util/thread.h | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)
tools/perf/util/machine.c +1 -1
@@ -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);
tools/perf/util/thread.c +3 -3
@@ -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.
tools/perf/util/thread.h +2 -2
@@ -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;
PATCH 9/9 tools: convert thread_map.refcnt from atomic_t to refcount_t
In reply to Elena Reshetovarefcount_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.
--- tools/perf/util/thread_map.c | 20 ++++++++++---------- tools/perf/util/thread_map.h | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-)
tools/perf/util/thread_map.c +10 -10
@@ -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)
tools/perf/util/thread_map.h +2 -2
@@ -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[]; };
Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerEm Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
Now when new
refcount_ttype and API are finally merged
(seeinclude/linux/refcount.h), the following
patches convert various refcounters in the tools susystem fromatomic_ttorefcount_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
Quoted 35 lines
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 fromatomic_ttorefcount_t
tools: convert cpu_map.refcnt fromatomic_ttorefcount_t
tools: convert comm_str.refcnt fromatomic_ttorefcount_t
tools: convert dso.refcnt fromatomic_ttorefcount_t
tools: convert map.refcnt fromatomic_ttorefcount_t
tools: convert map_groups.refcnt fromatomic_ttorefcount_t
tools: convert perf_map.refcnt fromatomic_ttorefcount_t
tools: convert thread.refcnt fromatomic_ttorefcount_t
tools: convert thread_map.refcnt fromatomic_ttorefcount_ttools/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(-)Signature · 1 line
2.7.4
Arnaldo Carvalho de Melo
maintainerEm Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 11 lines
Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
Now when new
refcount_ttype and API are finally merged
(seeinclude/linux/refcount.h), the following
patches convert various refcounters in the tools susystem fromatomic_ttorefcount_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
Arnaldo Carvalho de Melo
maintainerEm Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 20 lines
Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 11 lines
Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
Now when new
refcount_ttype and API are finally merged
(seeinclude/linux/refcount.h), the following
patches convert various refcounters in the tools susystem fromatomic_ttorefcount_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.gitThere are multiple fixes in it to get it to build and test it, so far, with:
Quoted 7 lines
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
Quoted 19 lines
Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 1 line
Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
escreveu:
Quoted 15 lines
Quoted 11 lines
Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
Now when new
refcount_ttype and API are finally merged
(seeinclude/linux/refcount.h), the following
patches convert various refcounters in the tools susystem fromatomic_ttorefcount_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/+/58d561002587bf2572f9e6f4d222659e4068fadf%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);
} 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."
Quoted 6 lines
Quoted 6 lines
There are multiple fixes in it to get it to build and test it, so far, with:
perf top -F 15000 -d 0
Quoted 7 lines
Quoted 5 lines
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.
Arnaldo Carvalho de Melo
maintainerEm Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
Quoted 35 lines
Quoted 19 lines
Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 1 line
Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
escreveu:
Quoted 15 lines
Quoted 11 lines
Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
Now when new
refcount_ttype and API are finally merged
(seeinclude/linux/refcount.h), the following
patches convert various refcounters in the tools susystem fromatomic_ttorefcount_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.gitI 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/+/58d561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0And more specifically to this chunk:
(quoted diff) +1
-1@@ -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); }
Quoted 4 lines
So, when the refcount set to zero in this place, what exactly happens to the
perf_mapobject 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
Quoted 18 lines
Quoted 5 lines
Quoted 5 lines
There are multiple fixes in it to get it to build and test it, so far, with:
perf top -F 15000 -d 0
Quoted 7 lines
Quoted 5 lines
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.
Arnaldo Carvalho de Melo
maintainerEm Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
Quoted 35 lines
Quoted 19 lines
Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 1 line
Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
escreveu:
Quoted 15 lines
Quoted 11 lines
Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
Quoted 5 lines
Now when new
refcount_ttype and API are finally merged
(seeinclude/linux/refcount.h), the following
patches convert various refcounters in the tools susystem fromatomic_ttorefcount_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.gitI 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/+/58d561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0And more specifically to this chunk:
(quoted diff) +1
-1@@ -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); }
Quoted 2 lines
So, when the refcount set to zero in this place, what exactly happens to the
perf_mapobject 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
tools/perf/util/evlist.c +14 -2
@@ -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,
Quoted 18 lines
Quoted 5 lines
Quoted 5 lines
There are multiple fixes in it to get it to build and test it, so far, with:
perf top -F 15000 -d 0
Quoted 7 lines
Quoted 5 lines
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.
Quoted 41 lines
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
Quoted 8 lines
Quoted 8 lines
Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 1 line
Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
escreveu:
Quoted 4 lines
Quoted 4 lines
Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
Quoted 3 lines
Now when new
refcount_ttype and API are finally merged (seeinclude/linux/refcount.h), the following patches convert various refcounters in the tools susystem from
atomic_tQuoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
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!
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
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.gitI took a look on it and it looks good. Just one thing I want to double check
with regards to this commit:
Quoted 1 line
https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0Review line 937
Quoted 5 lines
And more specifically to this chunk:
(quoted diff) +0
-0@@ -937,7 +937,7 @@ munmap(map->base,
perf_mmap__mmap_len(map));Quoted 8 lines
(quoted diff) +1
-1map->base = NULL; map->fd = -1; - atomic_set(&map->refcnt, 0); + refcount_set(&map->refcnt, 0); } auxtrace_mmap__munmap(&map->auxtrace_mmap); }
Quoted 7 lines
Quoted 1 line
So, when the refcount set to zero in this place, what exactly happens to the
perf_mapobject after?Quoted 1 line
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.
Quoted 57 lines
Those
perf_mmapobjects 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_tinfrastructure will catch it, which helpstools/.- Arnaldo
tools/perf/util/evlist.c +3
-1@@ -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
tools/perf/util/evlist.c +1
-0+* one extra to let perf_evlist__mmap_consume() get the last
tools/perf/util/evlist.c +1
-0+* events after all real references (
perf_mmap__get()) aretools/perf/util/evlist.c +3
-0+* dropped. +* +* Each PERF_EVENT_IOC_SET_OUTPUT points to this mmap and
tools/perf/util/evlist.c +5
-0+* 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 intools/perf/util/evlist.c +1
-1@@ -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
Quoted 1 line
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.
Quoted 23 lines
tools/perf/util/evlist.c +0
-0map->prev =0 ; map->mask = mp->mask; map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-Quoted 20 lines
prot,
Quoted 5 lines
Quoted 5 lines
There are multiple fixes in it to get it to build and test it, so far, with:
perf top -F 15000 -d 0
Quoted 7 lines
Quoted 5 lines
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.
Arnaldo Carvalho de Melo
maintainerEm Fri, Feb 24, 2017 at 07:27:32AM +0000, Reshetova, Elena escreveu:
Quoted 105 lines
Quoted 41 lines
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
Quoted 8 lines
Quoted 8 lines
Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 1 line
Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
escreveu:
Quoted 4 lines
Quoted 4 lines
Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
Quoted 3 lines
Now when new
refcount_ttype and API are finally merged (seeinclude/linux/refcount.h), the following patches convert various refcounters in the tools susystem from
atomic_tQuoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
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!
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
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.gitI took a look on it and it looks good. Just one thing I want to double check
with regards to this commit:
Quoted 1 line
https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0Review line 937
Quoted 5 lines
And more specifically to this chunk:
(quoted diff) +0
-0@@ -937,7 +937,7 @@ munmap(map->base,
perf_mmap__mmap_len(map));Quoted 8 lines
(quoted diff) +1
-1map->base = NULL; map->fd = -1; - atomic_set(&map->refcnt, 0); + refcount_set(&map->refcnt, 0); } auxtrace_mmap__munmap(&map->auxtrace_mmap); }Quoted 7 lines
Quoted 1 line
So, when the refcount set to zero in this place, what exactly happens to the
perf_mapobject after?Quoted 1 line
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.
Quoted 54 lines
Those
perf_mmapobjects 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_tinfrastructure will catch it, which helpstools/.- Arnaldo
tools/perf/util/evlist.c +3
-1@@ -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
tools/perf/util/evlist.c +1
-0+* one extra to let perf_evlist__mmap_consume() get the last
tools/perf/util/evlist.c +1
-0+* events after all real references (
perf_mmap__get()) aretools/perf/util/evlist.c +3
-0+* dropped. +* +* Each PERF_EVENT_IOC_SET_OUTPUT points to this mmap and
tools/perf/util/evlist.c +5
-0+* 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 intools/perf/util/evlist.c +0
-0@@ -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().
Quoted 8 lines
Quoted 4 lines
tools/perf/util/evlist.c +1
-1*/ -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_setto 2 when refcount value is "0", you are doing tworefcount_inc()viaperf_mmap__get(), which is not going to do any increments when refcount value is zero. So, in that sense having just onerefcount_setto 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()
Quoted 28 lines
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.Quoted 23 lines
tools/perf/util/evlist.c +0
-0map->prev =0 ; map->mask = mp->mask; map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-Quoted 20 lines
prot,
Quoted 5 lines
Quoted 5 lines
There are multiple fixes in it to get it to build and test it, so far, with:
perf top -F 15000 -d 0
Quoted 7 lines
Quoted 5 lines
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.
tip-bot for Arnaldo Carvalho de Melo
merge bot[tip:perf/core] perf evlist: Clarify a bit the use of perf_mmap->refcnt
In reply to Arnaldo Carvalho de Meloperf evlist: Clarify a bit the use of perf_mmap->refcnt
Commit-ID: 4738ca30b4a7a113084d7863846175094f95c62f Gitweb: http://git.kernel.org/tip/4738ca30b4a7a113084d7863846175094f95c62f Author: Arnaldo Carvalho de Melo <[email protected]> AuthorDate: Thu, 23 Feb 2017 13:24:34 -0300 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Fri, 3 Mar 2017 19:07:16 -0300
perf evlist: Clarify a bit the use of perf_mmap->refcnt
This is an odd refcount use case, so add some more comments to help understand that when it hits zero it really means that the mmap()ed area (on a perf_event_open() returned fd) has been munmap()ed.
--- tools/perf/util/evlist.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
tools/perf/util/evlist.c +12 -1
@@ -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; }
Peter Zijlstra
maintainerQuoted 22 lines · Elena Reshetova
Now when new
refcount_ttype and API are finally merged
(seeinclude/linux/refcount.h), the following
patches convert various refcounters in the tools susystem fromatomic_ttorefcount_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 fromatomic_ttorefcount_t
tools: convert cpu_map.refcnt fromatomic_ttorefcount_t
tools: convert comm_str.refcnt fromatomic_ttorefcount_t
tools: convert dso.refcnt fromatomic_ttorefcount_t
tools: convert map.refcnt fromatomic_ttorefcount_t
tools: convert map_groups.refcnt fromatomic_ttorefcount_t
tools: convert perf_map.refcnt fromatomic_ttorefcount_t
tools: convert thread.refcnt fromatomic_ttorefcount_t
tools: convert thread_map.refcnt fromatomic_ttorefcount_t
Quoted 15 lines
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.
Quoted 41 lines · Elena Reshetova
Quoted 7 lines
Now when new
refcount_ttype and API are finally merged
(seeinclude/linux/refcount.h), the following
patches convert various refcounters in the tools susystem fromatomic_ttorefcount_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.
Quoted 15 lines
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 fromatomic_ttorefcount_t
tools: convert cpu_map.refcnt fromatomic_ttorefcount_t
tools: convert comm_str.refcnt fromatomic_ttorefcount_t
tools: convert dso.refcnt fromatomic_ttorefcount_t
tools: convert map.refcnt fromatomic_ttorefcount_t
tools: convert map_groups.refcnt fromatomic_ttorefcount_t
tools: convert perf_map.refcnt fromatomic_ttorefcount_t
tools: convert thread.refcnt fromatomic_ttorefcount_t
tools: convert thread_map.refcnt fromatomic_ttorefcount_tQuoted 15 lines
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 ++--
Quoted 2 lines
This is userspace code; did you build this? I see a distinct lack of adding
refcount.hto 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!