[PATCH 0/9] tools subsystem refcounter conversions

[PATCH 0/9] tools subsystem refcounter conversions

[Elena Reshetova]

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

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

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

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

-- 
2.7.4

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

[Elena Reshetova]

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

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

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

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

[Arnaldo Carvalho de Melo]

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

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

See:

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

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

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

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

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

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

[Reshetova, Elena]

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

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

Best Regards,
Elena.

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

[Reshetova, Elena]

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

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

Best Regards,
Elena.

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

[Arnaldo Carvalho de Melo]

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

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

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

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

- Arnaldo

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

[Arnaldo Carvalho de Melo]

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

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

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

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

- Arnaldo

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

[Reshetova, Elena]

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

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

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

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

Best Regards,
Elena.
> 
> - Arnaldo

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

[Reshetova, Elena]

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

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

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

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

Best Regards,
Elena.
> 
> - Arnaldo

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

[Arnaldo Carvalho de Melo]

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

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

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

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

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

- Arnaldo

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

[Arnaldo Carvalho de Melo]

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

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

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

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

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

- Arnaldo

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

[Reshetova, Elena]

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

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

Best Regards,
Elena.

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

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

[Reshetova, Elena]

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

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

Best Regards,
Elena.

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

[tip:perf/core] perf cgroup: Convert cgroup_sel.refcnt from atomic_t to refcount_t

[tip-bot for Elena Reshetova]

Commit-ID:  79c5fe6db8c70558d3a64959f55596d137ccc6e6
Gitweb:     http://git.kernel.org/tip/79c5fe6db8c70558d3a64959f55596d137ccc6e6
Author:     Elena Reshetova <elena.reshetova@intel.com>
AuthorDate: Tue, 21 Feb 2017 17:34:55 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: David Windsor <dwindsor@gmail.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Kook <keescook@chromium.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: alsa-devel@alsa-project.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Windsor <dwindsor@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Hans Liljestrand <ishkamiel@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kees Kook <keescook@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1487691303-31858-2-git-send-email-elena.reshetova@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/cgroup.c | 6 +++---
 tools/perf/util/cgroup.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

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

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

[Elena Reshetova]

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

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

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

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

[Arnaldo Carvalho de Melo]

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

The following patch was needed for this one to build:

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

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

[Elena Reshetova]

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

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

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

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

[Arnaldo Carvalho de Melo]

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

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

1. converting to refcnt.h

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

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

3) not test building your patches :-\

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

- Arnaldo

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

[Arnaldo Carvalho de Melo]

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

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

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

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

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

- Arnaldo

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

[Arnaldo Carvalho de Melo]

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

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

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

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

[Reshetova, Elena]

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

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

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

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

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

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

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

This looks correct now.
	

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

[Reshetova, Elena]

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

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

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

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

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

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

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

This looks correct now.
	

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

[Arnaldo Carvalho de Melo]

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

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

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

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

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

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

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

Humm, but the sequence is:

1. refcount_set(1)

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

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

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

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

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

<SNIP>
 
> This looks correct now.

Thanks for checking,

- Arnaldo 

[PATCH 4/9] tools: convert dso.refcnt from atomic_t to refcount_t

[Elena Reshetova]

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

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

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 3abe337..f88aa44 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1109,7 +1109,7 @@ struct dso *dso__new(const char *name)
 		INIT_LIST_HEAD(&dso->node);
 		INIT_LIST_HEAD(&dso->data.open_entry);
 		pthread_mutex_init(&dso->lock, NULL);
-		atomic_set(&dso->refcnt, 1);
+		refcount_set(&dso->refcnt, 1);
 	}
 
 	return dso;
@@ -1147,13 +1147,13 @@ void dso__delete(struct dso *dso)
 struct dso *dso__get(struct dso *dso)
 {
 	if (dso)
-		atomic_inc(&dso->refcnt);
+		refcount_inc(&dso->refcnt);
 	return dso;
 }
 
 void dso__put(struct dso *dso)
 {
-	if (dso && atomic_dec_and_test(&dso->refcnt))
+	if (dso && refcount_dec_and_test(&dso->refcnt))
 		dso__delete(dso);
 }
 
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index ecc4bbd..12350b1 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -1,7 +1,7 @@
 #ifndef __PERF_DSO
 #define __PERF_DSO
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/types.h>
 #include <linux/rbtree.h>
 #include <sys/types.h>
@@ -187,7 +187,7 @@ struct dso {
 		void	 *priv;
 		u64	 db_id;
 	};
-	atomic_t	 refcnt;
+	refcount_t	 refcnt;
 	char		 name[0];
 };
 
-- 
2.7.4

Re: [PATCH 4/9] tools: convert dso.refcnt from atomic_t to refcount_t

[Arnaldo Carvalho de Melo]

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

Fixed by moving the include refcnt.h to later in the includes:

In file included from /home/acme/git/linux/tools/perf/util/dso.h:4:0,
                 from /home/acme/git/linux/tools/perf/util/machine.h:7,
                 from tests/thread-mg-share.c:2:
/home/acme/git/linux/tools/include/linux/refcount.h: In function ‘refcount_inc_not_zero’:
/home/acme/git/linux/tools/include/linux/refcount.h:95:23: error: ‘UINT_MAX’ undeclared (first use in this function)
  REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
                       ^
/home/acme/git/linux/tools/include/linux/refcount.h:47:41: note: in definition of macro ‘REFCOUNT_WARN’
 #define REFCOUNT_WARN(cond, str) (void)(cond)
                                         ^~~~
/home/acme/git/linux/tools/include/linux/refcount.h:95:23: note: each undeclared identifier is reported only once for each function it appears in
  REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
                       ^
/home/acme/git/linux/tools/include/linux/refcount.h:47:41: note: in definition of macro ‘REFCOUNT_WARN’
 #define REFCOUNT_WARN(cond, str) (void)(cond)

 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  tools/perf/util/dso.c | 6 +++---
>  tools/perf/util/dso.h | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 3abe337..f88aa44 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1109,7 +1109,7 @@ struct dso *dso__new(const char *name)
>  		INIT_LIST_HEAD(&dso->node);
>  		INIT_LIST_HEAD(&dso->data.open_entry);
>  		pthread_mutex_init(&dso->lock, NULL);
> -		atomic_set(&dso->refcnt, 1);
> +		refcount_set(&dso->refcnt, 1);
>  	}
>  
>  	return dso;
> @@ -1147,13 +1147,13 @@ void dso__delete(struct dso *dso)
>  struct dso *dso__get(struct dso *dso)
>  {
>  	if (dso)
> -		atomic_inc(&dso->refcnt);
> +		refcount_inc(&dso->refcnt);
>  	return dso;
>  }
>  
>  void dso__put(struct dso *dso)
>  {
> -	if (dso && atomic_dec_and_test(&dso->refcnt))
> +	if (dso && refcount_dec_and_test(&dso->refcnt))
>  		dso__delete(dso);
>  }
>  
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index ecc4bbd..12350b1 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -1,7 +1,7 @@
>  #ifndef __PERF_DSO
>  #define __PERF_DSO
>  
> -#include <linux/atomic.h>
> +#include <linux/refcount.h>
>  #include <linux/types.h>
>  #include <linux/rbtree.h>
>  #include <sys/types.h>
> @@ -187,7 +187,7 @@ struct dso {
>  		void	 *priv;
>  		u64	 db_id;
>  	};
> -	atomic_t	 refcnt;
> +	refcount_t	 refcnt;
>  	char		 name[0];
>  };
>  
> -- 
> 2.7.4

Re: [PATCH 4/9] tools: convert dso.refcnt from atomic_t to refcount_t

[Arnaldo Carvalho de Melo]

Em Wed, Feb 22, 2017 at 05:37:46PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Feb 21, 2017 at 05:34:58PM +0200, Elena Reshetova escreveu:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> Fixed by moving the include refcnt.h to later in the includes:

Nope, in this case this trick didn't work, I'm going back and adding
#include <linux/kernel.h> to refcnt.h.
 
> In file included from /home/acme/git/linux/tools/perf/util/dso.h:4:0,
>                  from /home/acme/git/linux/tools/perf/util/machine.h:7,
>                  from tests/thread-mg-share.c:2:
> /home/acme/git/linux/tools/include/linux/refcount.h: In function ‘refcount_inc_not_zero’:
> /home/acme/git/linux/tools/include/linux/refcount.h:95:23: error: ‘UINT_MAX’ undeclared (first use in this function)
>   REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
>                        ^
> /home/acme/git/linux/tools/include/linux/refcount.h:47:41: note: in definition of macro ‘REFCOUNT_WARN’
>  #define REFCOUNT_WARN(cond, str) (void)(cond)
>                                          ^~~~
> /home/acme/git/linux/tools/include/linux/refcount.h:95:23: note: each undeclared identifier is reported only once for each function it appears in
>   REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
>                        ^
> /home/acme/git/linux/tools/include/linux/refcount.h:47:41: note: in definition of macro ‘REFCOUNT_WARN’
>  #define REFCOUNT_WARN(cond, str) (void)(cond)
> 
>  
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> > ---
> >  tools/perf/util/dso.c | 6 +++---
> >  tools/perf/util/dso.h | 4 ++--
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 3abe337..f88aa44 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -1109,7 +1109,7 @@ struct dso *dso__new(const char *name)
> >  		INIT_LIST_HEAD(&dso->node);
> >  		INIT_LIST_HEAD(&dso->data.open_entry);
> >  		pthread_mutex_init(&dso->lock, NULL);
> > -		atomic_set(&dso->refcnt, 1);
> > +		refcount_set(&dso->refcnt, 1);
> >  	}
> >  
> >  	return dso;
> > @@ -1147,13 +1147,13 @@ void dso__delete(struct dso *dso)
> >  struct dso *dso__get(struct dso *dso)
> >  {
> >  	if (dso)
> > -		atomic_inc(&dso->refcnt);
> > +		refcount_inc(&dso->refcnt);
> >  	return dso;
> >  }
> >  
> >  void dso__put(struct dso *dso)
> >  {
> > -	if (dso && atomic_dec_and_test(&dso->refcnt))
> > +	if (dso && refcount_dec_and_test(&dso->refcnt))
> >  		dso__delete(dso);
> >  }
> >  
> > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> > index ecc4bbd..12350b1 100644
> > --- a/tools/perf/util/dso.h
> > +++ b/tools/perf/util/dso.h
> > @@ -1,7 +1,7 @@
> >  #ifndef __PERF_DSO
> >  #define __PERF_DSO
> >  
> > -#include <linux/atomic.h>
> > +#include <linux/refcount.h>
> >  #include <linux/types.h>
> >  #include <linux/rbtree.h>
> >  #include <sys/types.h>
> > @@ -187,7 +187,7 @@ struct dso {
> >  		void	 *priv;
> >  		u64	 db_id;
> >  	};
> > -	atomic_t	 refcnt;
> > +	refcount_t	 refcnt;
> >  	char		 name[0];
> >  };
> >  
> > -- 
> > 2.7.4

[tip:perf/core] perf dso: Convert dso.refcnt from atomic_t to refcount_t

[tip-bot for Elena Reshetova]

Commit-ID:  7100810a75b9854f1b05550b54500497c5914d4b
Gitweb:     http://git.kernel.org/tip/7100810a75b9854f1b05550b54500497c5914d4b
Author:     Elena Reshetova <elena.reshetova@intel.com>
AuthorDate: Tue, 21 Feb 2017 17:34:58 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: David Windsor <dwindsor@gmail.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Kook <keescook@chromium.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Windsor <dwindsor@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Hans Liljestrand <ishkamiel@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kees Kook <keescook@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: alsa-devel@alsa-project.org
Link: http://lkml.kernel.org/r/1487691303-31858-5-git-send-email-elena.reshetova@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c | 6 +++---
 tools/perf/util/dso.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index d38b62a..42db00d 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1109,7 +1109,7 @@ struct dso *dso__new(const char *name)
 		INIT_LIST_HEAD(&dso->node);
 		INIT_LIST_HEAD(&dso->data.open_entry);
 		pthread_mutex_init(&dso->lock, NULL);
-		atomic_set(&dso->refcnt, 1);
+		refcount_set(&dso->refcnt, 1);
 	}
 
 	return dso;
@@ -1147,13 +1147,13 @@ void dso__delete(struct dso *dso)
 struct dso *dso__get(struct dso *dso)
 {
 	if (dso)
-		atomic_inc(&dso->refcnt);
+		refcount_inc(&dso->refcnt);
 	return dso;
 }
 
 void dso__put(struct dso *dso)
 {
-	if (dso && atomic_dec_and_test(&dso->refcnt))
+	if (dso && refcount_dec_and_test(&dso->refcnt))
 		dso__delete(dso);
 }
 
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index ecc4bbd..12350b1 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -1,7 +1,7 @@
 #ifndef __PERF_DSO
 #define __PERF_DSO
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/types.h>
 #include <linux/rbtree.h>
 #include <sys/types.h>
@@ -187,7 +187,7 @@ struct dso {
 		void	 *priv;
 		u64	 db_id;
 	};
-	atomic_t	 refcnt;
+	refcount_t	 refcnt;
 	char		 name[0];
 };
 

[PATCH 5/9] tools: convert map.refcnt from atomic_t to refcount_t

[Elena Reshetova]

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

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

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 0a943e7..f0e2428 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -141,7 +141,7 @@ void map__init(struct map *map, enum map_type type,
 	RB_CLEAR_NODE(&map->rb_node);
 	map->groups   = NULL;
 	map->erange_warned = false;
-	atomic_set(&map->refcnt, 1);
+	refcount_set(&map->refcnt, 1);
 }
 
 struct map *map__new(struct machine *machine, u64 start, u64 len,
@@ -255,7 +255,7 @@ void map__delete(struct map *map)
 
 void map__put(struct map *map)
 {
-	if (map && atomic_dec_and_test(&map->refcnt))
+	if (map && refcount_dec_and_test(&map->refcnt))
 		map__delete(map);
 }
 
@@ -354,7 +354,7 @@ struct map *map__clone(struct map *from)
 	struct map *map = memdup(from, sizeof(*map));
 
 	if (map != NULL) {
-		atomic_set(&map->refcnt, 1);
+		refcount_set(&map->refcnt, 1);
 		RB_CLEAR_NODE(&map->rb_node);
 		dso__get(map->dso);
 		map->groups = NULL;
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index abdacf8..9545ff3 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -1,7 +1,7 @@
 #ifndef __PERF_MAP_H
 #define __PERF_MAP_H
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/compiler.h>
 #include <linux/list.h>
 #include <linux/rbtree.h>
@@ -51,7 +51,7 @@ struct map {
 
 	struct dso		*dso;
 	struct map_groups	*groups;
-	atomic_t		refcnt;
+	refcount_t		refcnt;
 };
 
 struct kmap {
@@ -150,7 +150,7 @@ struct map *map__clone(struct map *map);
 static inline struct map *map__get(struct map *map)
 {
 	if (map)
-		atomic_inc(&map->refcnt);
+		refcount_inc(&map->refcnt);
 	return map;
 }
 
-- 
2.7.4

[tip:perf/core] perf map: Convert map.refcnt from atomic_t to refcount_t

[tip-bot for Elena Reshetova]

Commit-ID:  e3a42cdd3e35d6c2181d5acfa191eb448aea6ace
Gitweb:     http://git.kernel.org/tip/e3a42cdd3e35d6c2181d5acfa191eb448aea6ace
Author:     Elena Reshetova <elena.reshetova@intel.com>
AuthorDate: Tue, 21 Feb 2017 17:34:59 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: David Windsor <dwindsor@gmail.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Kook <keescook@chromium.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Windsor <dwindsor@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Hans Liljestrand <ishkamiel@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kees Kook <keescook@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: alsa-devel@alsa-project.org
Link: http://lkml.kernel.org/r/1487691303-31858-6-git-send-email-elena.reshetova@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c | 6 +++---
 tools/perf/util/map.h | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 0a943e7..f0e2428 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -141,7 +141,7 @@ void map__init(struct map *map, enum map_type type,
 	RB_CLEAR_NODE(&map->rb_node);
 	map->groups   = NULL;
 	map->erange_warned = false;
-	atomic_set(&map->refcnt, 1);
+	refcount_set(&map->refcnt, 1);
 }
 
 struct map *map__new(struct machine *machine, u64 start, u64 len,
@@ -255,7 +255,7 @@ void map__delete(struct map *map)
 
 void map__put(struct map *map)
 {
-	if (map && atomic_dec_and_test(&map->refcnt))
+	if (map && refcount_dec_and_test(&map->refcnt))
 		map__delete(map);
 }
 
@@ -354,7 +354,7 @@ struct map *map__clone(struct map *from)
 	struct map *map = memdup(from, sizeof(*map));
 
 	if (map != NULL) {
-		atomic_set(&map->refcnt, 1);
+		refcount_set(&map->refcnt, 1);
 		RB_CLEAR_NODE(&map->rb_node);
 		dso__get(map->dso);
 		map->groups = NULL;
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index abdacf8..9545ff3 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -1,7 +1,7 @@
 #ifndef __PERF_MAP_H
 #define __PERF_MAP_H
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/compiler.h>
 #include <linux/list.h>
 #include <linux/rbtree.h>
@@ -51,7 +51,7 @@ struct map {
 
 	struct dso		*dso;
 	struct map_groups	*groups;
-	atomic_t		refcnt;
+	refcount_t		refcnt;
 };
 
 struct kmap {
@@ -150,7 +150,7 @@ struct map *map__clone(struct map *map);
 static inline struct map *map__get(struct map *map)
 {
 	if (map)
-		atomic_inc(&map->refcnt);
+		refcount_inc(&map->refcnt);
 	return map;
 }
 

[PATCH 6/9] tools: convert map_groups.refcnt from atomic_t to refcount_t

[Elena Reshetova]

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

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

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index f0e2428..1d9ebcf 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -485,7 +485,7 @@ void map_groups__init(struct map_groups *mg, struct machine *machine)
 		maps__init(&mg->maps[i]);
 	}
 	mg->machine = machine;
-	atomic_set(&mg->refcnt, 1);
+	refcount_set(&mg->refcnt, 1);
 }
 
 static void __maps__purge(struct maps *maps)
@@ -547,7 +547,7 @@ void map_groups__delete(struct map_groups *mg)
 
 void map_groups__put(struct map_groups *mg)
 {
-	if (mg && atomic_dec_and_test(&mg->refcnt))
+	if (mg && refcount_dec_and_test(&mg->refcnt))
 		map_groups__delete(mg);
 }
 
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 9545ff3..c8a5a64 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -67,7 +67,7 @@ struct maps {
 struct map_groups {
 	struct maps	 maps[MAP__NR_TYPES];
 	struct machine	 *machine;
-	atomic_t	 refcnt;
+	refcount_t	 refcnt;
 };
 
 struct map_groups *map_groups__new(struct machine *machine);
@@ -77,7 +77,7 @@ bool map_groups__empty(struct map_groups *mg);
 static inline struct map_groups *map_groups__get(struct map_groups *mg)
 {
 	if (mg)
-		atomic_inc(&mg->refcnt);
+		refcount_inc(&mg->refcnt);
 	return mg;
 }
 
-- 
2.7.4

Re: [PATCH 6/9] tools: convert map_groups.refcnt from atomic_t to refcount_t

[Arnaldo Carvalho de Melo]

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

You missed tools/perf/tests/thread-mg-share.c

I fixed it up.
 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  tools/perf/util/map.c | 4 ++--
>  tools/perf/util/map.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index f0e2428..1d9ebcf 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -485,7 +485,7 @@ void map_groups__init(struct map_groups *mg, struct machine *machine)
>  		maps__init(&mg->maps[i]);
>  	}
>  	mg->machine = machine;
> -	atomic_set(&mg->refcnt, 1);
> +	refcount_set(&mg->refcnt, 1);
>  }
>  
>  static void __maps__purge(struct maps *maps)
> @@ -547,7 +547,7 @@ void map_groups__delete(struct map_groups *mg)
>  
>  void map_groups__put(struct map_groups *mg)
>  {
> -	if (mg && atomic_dec_and_test(&mg->refcnt))
> +	if (mg && refcount_dec_and_test(&mg->refcnt))
>  		map_groups__delete(mg);
>  }
>  
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index 9545ff3..c8a5a64 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -67,7 +67,7 @@ struct maps {
>  struct map_groups {
>  	struct maps	 maps[MAP__NR_TYPES];
>  	struct machine	 *machine;
> -	atomic_t	 refcnt;
> +	refcount_t	 refcnt;
>  };
>  
>  struct map_groups *map_groups__new(struct machine *machine);
> @@ -77,7 +77,7 @@ bool map_groups__empty(struct map_groups *mg);
>  static inline struct map_groups *map_groups__get(struct map_groups *mg)
>  {
>  	if (mg)
> -		atomic_inc(&mg->refcnt);
> +		refcount_inc(&mg->refcnt);
>  	return mg;
>  }
>  
> -- 
> 2.7.4

[PATCH 7/9] tools: convert perf_map.refcnt from atomic_t to refcount_t

[Elena Reshetova]

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

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

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index b601f28..564b924 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -777,7 +777,7 @@ union perf_event *perf_mmap__read_forward(struct perf_mmap *md, bool check_messu
 	/*
 	 * Check if event was unmapped due to a POLLHUP/POLLERR.
 	 */
-	if (!atomic_read(&md->refcnt))
+	if (!refcount_read(&md->refcnt))
 		return NULL;
 
 	head = perf_mmap__read_head(md);
@@ -794,7 +794,7 @@ perf_mmap__read_backward(struct perf_mmap *md)
 	/*
 	 * Check if event was unmapped due to a POLLHUP/POLLERR.
 	 */
-	if (!atomic_read(&md->refcnt))
+	if (!refcount_read(&md->refcnt))
 		return NULL;
 
 	head = perf_mmap__read_head(md);
@@ -856,7 +856,7 @@ void perf_mmap__read_catchup(struct perf_mmap *md)
 {
 	u64 head;
 
-	if (!atomic_read(&md->refcnt))
+	if (!refcount_read(&md->refcnt))
 		return;
 
 	head = perf_mmap__read_head(md);
@@ -875,14 +875,14 @@ static bool perf_mmap__empty(struct perf_mmap *md)
 
 static void perf_mmap__get(struct perf_mmap *map)
 {
-	atomic_inc(&map->refcnt);
+	refcount_inc(&map->refcnt);
 }
 
 static void perf_mmap__put(struct perf_mmap *md)
 {
-	BUG_ON(md->base && atomic_read(&md->refcnt) == 0);
+	BUG_ON(md->base && refcount_read(&md->refcnt) == 0);
 
-	if (atomic_dec_and_test(&md->refcnt))
+	if (refcount_dec_and_test(&md->refcnt))
 		perf_mmap__munmap(md);
 }
 
@@ -894,7 +894,7 @@ void perf_mmap__consume(struct perf_mmap *md, bool overwrite)
 		perf_mmap__write_tail(md, old);
 	}
 
-	if (atomic_read(&md->refcnt) == 1 && perf_mmap__empty(md))
+	if (refcount_read(&md->refcnt) == 1 && perf_mmap__empty(md))
 		perf_mmap__put(md);
 }
 
@@ -937,7 +937,7 @@ static void perf_mmap__munmap(struct perf_mmap *map)
 		munmap(map->base, perf_mmap__mmap_len(map));
 		map->base = NULL;
 		map->fd = -1;
-		atomic_set(&map->refcnt, 0);
+		refcount_set(&map->refcnt, 0);
 	}
 	auxtrace_mmap__munmap(&map->auxtrace_mmap);
 }
@@ -1001,7 +1001,7 @@ static int perf_mmap__mmap(struct perf_mmap *map,
 	 * evlist layer can't just drop it when filtering events in
 	 * perf_evlist__filter_pollfd().
 	 */
-	atomic_set(&map->refcnt, 2);
+	refcount_set(&map->refcnt, 2);
 	map->prev = 0;
 	map->mask = mp->mask;
 	map->base = mmap(NULL, perf_mmap__mmap_len(map), mp->prot,
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 389b9cc..3994299 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -1,7 +1,7 @@
 #ifndef __PERF_EVLIST_H
 #define __PERF_EVLIST_H 1
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/list.h>
 #include <api/fd/array.h>
 #include <stdio.h>
@@ -29,7 +29,7 @@ struct perf_mmap {
 	void		 *base;
 	int		 mask;
 	int		 fd;
-	atomic_t	 refcnt;
+	refcount_t	 refcnt;
 	u64		 prev;
 	struct auxtrace_mmap auxtrace_mmap;
 	char		 event_copy[PERF_SAMPLE_MAX_SIZE] __attribute__((aligned(8)));
-- 
2.7.4

[PATCH 8/9] tools: convert thread.refcnt from atomic_t to refcount_t

[Elena Reshetova]

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

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

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index f5af87f..74e79d2 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -53,7 +53,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
 			goto err_thread;
 
 		list_add(&comm->list, &thread->comm_list);
-		atomic_set(&thread->refcnt, 1);
+		refcount_set(&thread->refcnt, 1);
 		RB_CLEAR_NODE(&thread->rb_node);
 	}
 
@@ -88,13 +88,13 @@ void thread__delete(struct thread *thread)
 struct thread *thread__get(struct thread *thread)
 {
 	if (thread)
-		atomic_inc(&thread->refcnt);
+		refcount_inc(&thread->refcnt);
 	return thread;
 }
 
 void thread__put(struct thread *thread)
 {
-	if (thread && atomic_dec_and_test(&thread->refcnt)) {
+	if (thread && refcount_dec_and_test(&thread->refcnt)) {
 		/*
 		 * Remove it from the dead_threads list, as last reference
 		 * is gone.
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 99263cb..e571885 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -1,7 +1,7 @@
 #ifndef __PERF_THREAD_H
 #define __PERF_THREAD_H
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/rbtree.h>
 #include <linux/list.h>
 #include <unistd.h>
@@ -23,7 +23,7 @@ struct thread {
 	pid_t			tid;
 	pid_t			ppid;
 	int			cpu;
-	atomic_t		refcnt;
+	refcount_t		refcnt;
 	char			shortname[3];
 	bool			comm_set;
 	int			comm_len;
-- 
2.7.4

Re: [PATCH 8/9] tools: convert thread.refcnt from atomic_t to refcount_t

[Arnaldo Carvalho de Melo]

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

You forgot this one, fixing it...

@@ -1439,7 +1439,7 @@ static void __machine__remove_thread(struct machine *machine, struct thread *th,
        if (machine->last_match == th)
                machine->last_match = NULL;
 
-       BUG_ON(atomic_read(&th->refcnt) == 0);
+       BUG_ON(refcount_read(&th->refcnt) == 0);
        if (lock)
                pthread_rwlock_wrlock(&machine->threads_lock);
        rb_erase_init(&th->rb_node, &machine->threads);
[acme@jouet linux]$
 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  tools/perf/util/thread.c | 6 +++---
>  tools/perf/util/thread.h | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index f5af87f..74e79d2 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -53,7 +53,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
>  			goto err_thread;
>  
>  		list_add(&comm->list, &thread->comm_list);
> -		atomic_set(&thread->refcnt, 1);
> +		refcount_set(&thread->refcnt, 1);
>  		RB_CLEAR_NODE(&thread->rb_node);
>  	}
>  
> @@ -88,13 +88,13 @@ void thread__delete(struct thread *thread)
>  struct thread *thread__get(struct thread *thread)
>  {
>  	if (thread)
> -		atomic_inc(&thread->refcnt);
> +		refcount_inc(&thread->refcnt);
>  	return thread;
>  }
>  
>  void thread__put(struct thread *thread)
>  {
> -	if (thread && atomic_dec_and_test(&thread->refcnt)) {
> +	if (thread && refcount_dec_and_test(&thread->refcnt)) {
>  		/*
>  		 * Remove it from the dead_threads list, as last reference
>  		 * is gone.
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 99263cb..e571885 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -1,7 +1,7 @@
>  #ifndef __PERF_THREAD_H
>  #define __PERF_THREAD_H
>  
> -#include <linux/atomic.h>
> +#include <linux/refcount.h>
>  #include <linux/rbtree.h>
>  #include <linux/list.h>
>  #include <unistd.h>
> @@ -23,7 +23,7 @@ struct thread {
>  	pid_t			tid;
>  	pid_t			ppid;
>  	int			cpu;
> -	atomic_t		refcnt;
> +	refcount_t		refcnt;
>  	char			shortname[3];
>  	bool			comm_set;
>  	int			comm_len;
> -- 
> 2.7.4

[tip:perf/core] perf thread: convert thread.refcnt from atomic_t to refcount_t

[tip-bot for Elena Reshetova]

Commit-ID:  e34f5b11cd51fbe723e481c1db03a77260be6f4c
Gitweb:     http://git.kernel.org/tip/e34f5b11cd51fbe723e481c1db03a77260be6f4c
Author:     Elena Reshetova <elena.reshetova@intel.com>
AuthorDate: Tue, 21 Feb 2017 17:35:02 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: David Windsor <dwindsor@gmail.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Kook <keescook@chromium.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Windsor <dwindsor@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Hans Liljestrand <ishkamiel@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kees Kook <keescook@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: alsa-devel@alsa-project.org
Link: http://lkml.kernel.org/r/1487691303-31858-9-git-send-email-elena.reshetova@intel.com
[ Did missing conversion in __machine__remove_thread() ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 71c9720..b9974fe 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -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);
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index f5af87f..74e79d2 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -53,7 +53,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
 			goto err_thread;
 
 		list_add(&comm->list, &thread->comm_list);
-		atomic_set(&thread->refcnt, 1);
+		refcount_set(&thread->refcnt, 1);
 		RB_CLEAR_NODE(&thread->rb_node);
 	}
 
@@ -88,13 +88,13 @@ void thread__delete(struct thread *thread)
 struct thread *thread__get(struct thread *thread)
 {
 	if (thread)
-		atomic_inc(&thread->refcnt);
+		refcount_inc(&thread->refcnt);
 	return thread;
 }
 
 void thread__put(struct thread *thread)
 {
-	if (thread && atomic_dec_and_test(&thread->refcnt)) {
+	if (thread && refcount_dec_and_test(&thread->refcnt)) {
 		/*
 		 * Remove it from the dead_threads list, as last reference
 		 * is gone.
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 99263cb..e571885 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -1,7 +1,7 @@
 #ifndef __PERF_THREAD_H
 #define __PERF_THREAD_H
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/rbtree.h>
 #include <linux/list.h>
 #include <unistd.h>
@@ -23,7 +23,7 @@ struct thread {
 	pid_t			tid;
 	pid_t			ppid;
 	int			cpu;
-	atomic_t		refcnt;
+	refcount_t		refcnt;
 	char			shortname[3];
 	bool			comm_set;
 	int			comm_len;

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

[Elena Reshetova]

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

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

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

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

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

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

I'll try and get this merged ASAP.

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

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

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

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

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

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

  perf top -F 15000 -d 0

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

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

- Arnaldo

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

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

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

- Arnaldo

RE: [PATCH 0/9] tools subsystem refcounter conversions

[Reshetova, Elena]

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

I took a look on it and it looks good. Just one thing I want to double check with regards to this commit:
https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/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." 


> >
> > There are multiple fixes in it to get it to build and test it, so far,
> > with:
> >
> >   perf top -F 15000 -d 0
> >
> > while doing kernel builds and tight usleep 1 loops to create lots of
> > short lived threads with its map_groups, maps, dsos, etc.
> >
> > Now running some build tests in some 36 containers with assorted distros
> > and cross compilers.
> 
> Tomorrow I'll inject some refcount errors to test this all.


Thank you!

Best Regards,
Elena.

RE: [PATCH 0/9] tools subsystem refcounter conversions

[Reshetova, Elena]

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

I took a look on it and it looks good. Just one thing I want to double check with regards to this commit:
https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/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." 


> >
> > There are multiple fixes in it to get it to build and test it, so far,
> > with:
> >
> >   perf top -F 15000 -d 0
> >
> > while doing kernel builds and tight usleep 1 loops to create lots of
> > short lived threads with its map_groups, maps, dsos, etc.
> >
> > Now running some build tests in some 36 containers with assorted distros
> > and cross compilers.
> 
> Tomorrow I'll inject some refcount errors to test this all.


Thank you!

Best Regards,
Elena.

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > Now when new refcount_t type and API are finally merged
> > > > > (see include/linux/refcount.h), the following
> > > > > patches convert various refcounters in the tools susystem from atomic_t
> > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > >
> > > > Thanks for working on this! I was almost going to jump on doing this
> > > > myself!
> > > >
> > > > I'll try and get this merged ASAP.
> > >
> > > So, please take a look at my tmp.perf/refcount branch at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> 
> I took a look on it and it looks good. Just one thing I want to double check with regards to this commit:
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/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." 

So, this looks fishy and I don't recall why that was done that way, this
conversion to refcnt_t and the debug associated with it provides a good
opportunity for us to try to get that to a more familiar reference
counting workflow, I'll take a look at it.

- Arnaldo
 
> > > There are multiple fixes in it to get it to build and test it, so far,
> > > with:
> > >
> > >   perf top -F 15000 -d 0
> > >
> > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > short lived threads with its map_groups, maps, dsos, etc.
> > >
> > > Now running some build tests in some 36 containers with assorted distros
> > > and cross compilers.
> > 
> > Tomorrow I'll inject some refcount errors to test this all.
> 
> 
> Thank you!
> 
> Best Regards,
> Elena.

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > Now when new refcount_t type and API are finally merged
> > > > > (see include/linux/refcount.h), the following
> > > > > patches convert various refcounters in the tools susystem from atomic_t
> > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > >
> > > > Thanks for working on this! I was almost going to jump on doing this
> > > > myself!
> > > >
> > > > I'll try and get this merged ASAP.
> > >
> > > So, please take a look at my tmp.perf/refcount branch at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> 
> I took a look on it and it looks good. Just one thing I want to double check with regards to this commit:
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/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." 

So, this looks fishy and I don't recall why that was done that way, this
conversion to refcnt_t and the debug associated with it provides a good
opportunity for us to try to get that to a more familiar reference
counting workflow, I'll take a look at it.

- Arnaldo
 
> > > There are multiple fixes in it to get it to build and test it, so far,
> > > with:
> > >
> > >   perf top -F 15000 -d 0
> > >
> > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > short lived threads with its map_groups, maps, dsos, etc.
> > >
> > > Now running some build tests in some 36 containers with assorted distros
> > > and cross compilers.
> > 
> > Tomorrow I'll inject some refcount errors to test this all.
> 
> 
> Thank you!
> 
> Best Regards,
> Elena.

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > Now when new refcount_t type and API are finally merged
> > > > > (see include/linux/refcount.h), the following
> > > > > patches convert various refcounters in the tools susystem from atomic_t
> > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > >
> > > > Thanks for working on this! I was almost going to jump on doing this
> > > > myself!
> > > >
> > > > I'll try and get this merged ASAP.
> > >
> > > So, please take a look at my tmp.perf/refcount branch at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> 
> I took a look on it and it looks good. Just one thing I want to double check with regards to this commit:
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/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." 

So, this is an odd use of a reference count, the patch below should help
understand it?

Those perf_mmap objects are created in a batch fashion, it being zero
just means it isn't yet mmaped at all, and we check for that before
using it.

So, it remains a bug to do a dec for a zeroed refcount, and the
refcount_t infrastructure will catch it, which helps tools/.

- Arnaldo

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 564b924fb48a..5a70f08d2518 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -974,8 +974,19 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
 	if (!map)
 		return NULL;
 
-	for (i = 0; i < evlist->nr_mmaps; i++)
+	for (i = 0; i < evlist->nr_mmaps; i++) {
 		map[i].fd = -1;
+		/*
+		 * When the perf_mmap() call is made we grab one refcount, plus
+		 * one extra to let perf_evlist__mmap_consume() get the last
+		 * events after all real references (perf_mmap__get()) are
+		 * dropped.
+		 *
+		 * Each PERF_EVENT_IOC_SET_OUTPUT points to this mmap and
+		 * thus does perf_mmap__get() on it.
+ 		 */
+		refcount_set(&map[i].refcnt, 0);
+	}
 	return map;
 }
 
@@ -988,6 +999,7 @@ struct mmap_params {
 static int perf_mmap__mmap(struct perf_mmap *map,
 			   struct mmap_params *mp, int fd)
 {
+	perf_mmap__get(map);
 	/*
 	 * The last one will be done at perf_evlist__mmap_consume(), so that we
 	 * make sure we don't prevent tools from consuming every last event in
@@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap *map,
 	 * evlist layer can't just drop it when filtering events in
 	 * perf_evlist__filter_pollfd().
 	 */
-	refcount_set(&map->refcnt, 2);
+	perf_mmap__get(map); /* This is not a dup, see the comment above! */
 	map->prev = 0;
 	map->mask = mp->mask;
 	map->base = mmap(NULL, perf_mmap__mmap_len(map), mp->prot,
 
> > > There are multiple fixes in it to get it to build and test it, so far,
> > > with:
> > >
> > >   perf top -F 15000 -d 0
> > >
> > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > short lived threads with its map_groups, maps, dsos, etc.
> > >
> > > Now running some build tests in some 36 containers with assorted distros
> > > and cross compilers.
> > 
> > Tomorrow I'll inject some refcount errors to test this all.
> 
> 
> Thank you!
> 
> Best Regards,
> Elena.

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > Now when new refcount_t type and API are finally merged
> > > > > (see include/linux/refcount.h), the following
> > > > > patches convert various refcounters in the tools susystem from atomic_t
> > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > >
> > > > Thanks for working on this! I was almost going to jump on doing this
> > > > myself!
> > > >
> > > > I'll try and get this merged ASAP.
> > >
> > > So, please take a look at my tmp.perf/refcount branch at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> 
> I took a look on it and it looks good. Just one thing I want to double check with regards to this commit:
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/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." 

So, this is an odd use of a reference count, the patch below should help
understand it?

Those perf_mmap objects are created in a batch fashion, it being zero
just means it isn't yet mmaped at all, and we check for that before
using it.

So, it remains a bug to do a dec for a zeroed refcount, and the
refcount_t infrastructure will catch it, which helps tools/.

- Arnaldo

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 564b924fb48a..5a70f08d2518 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -974,8 +974,19 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
 	if (!map)
 		return NULL;
 
-	for (i = 0; i < evlist->nr_mmaps; i++)
+	for (i = 0; i < evlist->nr_mmaps; i++) {
 		map[i].fd = -1;
+		/*
+		 * When the perf_mmap() call is made we grab one refcount, plus
+		 * one extra to let perf_evlist__mmap_consume() get the last
+		 * events after all real references (perf_mmap__get()) are
+		 * dropped.
+		 *
+		 * Each PERF_EVENT_IOC_SET_OUTPUT points to this mmap and
+		 * thus does perf_mmap__get() on it.
+ 		 */
+		refcount_set(&map[i].refcnt, 0);
+	}
 	return map;
 }
 
@@ -988,6 +999,7 @@ struct mmap_params {
 static int perf_mmap__mmap(struct perf_mmap *map,
 			   struct mmap_params *mp, int fd)
 {
+	perf_mmap__get(map);
 	/*
 	 * The last one will be done at perf_evlist__mmap_consume(), so that we
 	 * make sure we don't prevent tools from consuming every last event in
@@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap *map,
 	 * evlist layer can't just drop it when filtering events in
 	 * perf_evlist__filter_pollfd().
 	 */
-	refcount_set(&map->refcnt, 2);
+	perf_mmap__get(map); /* This is not a dup, see the comment above! */
 	map->prev = 0;
 	map->mask = mp->mask;
 	map->base = mmap(NULL, perf_mmap__mmap_len(map), mp->prot,
 
> > > There are multiple fixes in it to get it to build and test it, so far,
> > > with:
> > >
> > >   perf top -F 15000 -d 0
> > >
> > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > short lived threads with its map_groups, maps, dsos, etc.
> > >
> > > Now running some build tests in some 36 containers with assorted distros
> > > and cross compilers.
> > 
> > Tomorrow I'll inject some refcount errors to test this all.
> 
> 
> Thank you!
> 
> Best Regards,
> Elena.

RE: [PATCH 0/9] tools subsystem refcounter conversions

[Reshetova, Elena]

> Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > > Now when new refcount_t type and API are finally merged
> > > > > > (see include/linux/refcount.h), the following
> > > > > > patches convert various refcounters in the tools susystem from
> atomic_t
> > > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > > >
> > > > > Thanks for working on this! I was almost going to jump on doing this
> > > > > myself!
> > > > >
> > > > > I'll try and get this merged ASAP.
> > > >
> > > > So, please take a look at my tmp.perf/refcount branch at:
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> >
> > I took a look on it and it looks good. Just one thing I want to double check
> with regards to this commit:
> >
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
> 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
> >
> > And more specifically to this chunk:
> >
> > @@ -937,7 +937,7 @@
> >  		munmap(map->base,
> perf_mmap__mmap_len(map));
> >  		map->base = NULL;
> >  		map->fd = -1;
> > -		atomic_set(&map->refcnt, 0);
> > +		refcount_set(&map->refcnt, 0);
> >  	}
> >  	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."
> 
> So, this is an odd use of a reference count, the patch below should help
> understand it?

Yes, it helps, indeed, but I think we have an issue here. See below inline in patch. 

> 
> Those perf_mmap objects are created in a batch fashion, it being zero
> just means it isn't yet mmaped at all, and we check for that before
> using it.
> 
> So, it remains a bug to do a dec for a zeroed refcount, and the
> refcount_t infrastructure will catch it, which helps tools/.
> 
> - Arnaldo
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 564b924fb48a..5a70f08d2518 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -974,8 +974,19 @@ static struct perf_mmap
> *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
>  	if (!map)
>  		return NULL;
> 
> -	for (i = 0; i < evlist->nr_mmaps; i++)
> +	for (i = 0; i < evlist->nr_mmaps; i++) {
>  		map[i].fd = -1;
> +		/*
> +		 * When the perf_mmap() call is made we 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! */

This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two
refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero. 
So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)

When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above. 

Best Regards,
Elena.

>  	map->prev = 0;
>  	map->mask = mp->mask;
>  	map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
> >prot,
> 
> > > > There are multiple fixes in it to get it to build and test it, so far,
> > > > with:
> > > >
> > > >   perf top -F 15000 -d 0
> > > >
> > > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > > short lived threads with its map_groups, maps, dsos, etc.
> > > >
> > > > Now running some build tests in some 36 containers with assorted distros
> > > > and cross compilers.
> > >
> > > Tomorrow I'll inject some refcount errors to test this all.
> >
> >
> > Thank you!
> >
> > Best Regards,
> > Elena.

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Reshetova, Elena]

> Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > > Now when new refcount_t type and API are finally merged
> > > > > > (see include/linux/refcount.h), the following
> > > > > > patches convert various refcounters in the tools susystem from
> atomic_t
> > > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > > >
> > > > > Thanks for working on this! I was almost going to jump on doing this
> > > > > myself!
> > > > >
> > > > > I'll try and get this merged ASAP.
> > > >
> > > > So, please take a look at my tmp.perf/refcount branch at:
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> >
> > I took a look on it and it looks good. Just one thing I want to double check
> with regards to this commit:
> >
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
> 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
> >
> > And more specifically to this chunk:
> >
> > @@ -937,7 +937,7 @@
> >  		munmap(map->base,
> perf_mmap__mmap_len(map));
> >  		map->base = NULL;
> >  		map->fd = -1;
> > -		atomic_set(&map->refcnt, 0);
> > +		refcount_set(&map->refcnt, 0);
> >  	}
> >  	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."
> 
> So, this is an odd use of a reference count, the patch below should help
> understand it?

Yes, it helps, indeed, but I think we have an issue here. See below inline in patch. 

> 
> Those perf_mmap objects are created in a batch fashion, it being zero
> just means it isn't yet mmaped at all, and we check for that before
> using it.
> 
> So, it remains a bug to do a dec for a zeroed refcount, and the
> refcount_t infrastructure will catch it, which helps tools/.
> 
> - Arnaldo
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 564b924fb48a..5a70f08d2518 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -974,8 +974,19 @@ static struct perf_mmap
> *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
>  	if (!map)
>  		return NULL;
> 
> -	for (i = 0; i < evlist->nr_mmaps; i++)
> +	for (i = 0; i < evlist->nr_mmaps; i++) {
>  		map[i].fd = -1;
> +		/*
> +		 * When the perf_mmap() call is made we 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! */

This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two
refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero. 
So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)

When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above. 

Best Regards,
Elena.

>  	map->prev = 0;
>  	map->mask = mp->mask;
>  	map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
> >prot,
> 
> > > > There are multiple fixes in it to get it to build and test it, so far,
> > > > with:
> > > >
> > > >   perf top -F 15000 -d 0
> > > >
> > > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > > short lived threads with its map_groups, maps, dsos, etc.
> > > >
> > > > Now running some build tests in some 36 containers with assorted distros
> > > > and cross compilers.
> > >
> > > Tomorrow I'll inject some refcount errors to test this all.
> >
> >
> > Thank you!
> >
> > Best Regards,
> > Elena.

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

Em Fri, Feb 24, 2017 at 07:27:32AM +0000, Reshetova, Elena escreveu:
> > Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > > > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > > > Now when new refcount_t type and API are finally merged
> > > > > > > (see include/linux/refcount.h), the following
> > > > > > > patches convert various refcounters in the tools susystem from
> > atomic_t
> > > > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > > > >
> > > > > > Thanks for working on this! I was almost going to jump on doing this
> > > > > > myself!
> > > > > >
> > > > > > I'll try and get this merged ASAP.
> > > > >
> > > > > So, please take a look at my tmp.perf/refcount branch at:
> > > > >
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> > >
> > > I took a look on it and it looks good. Just one thing I want to double check
> > with regards to this commit:
> > >
> > https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
> > 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
> > >
> > > And more specifically to this chunk:
> > >
> > > @@ -937,7 +937,7 @@
> > >  		munmap(map->base,
> > perf_mmap__mmap_len(map));
> > >  		map->base = NULL;
> > >  		map->fd = -1;
> > > -		atomic_set(&map->refcnt, 0);
> > > +		refcount_set(&map->refcnt, 0);
> > >  	}
> > >  	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."
> > 
> > So, this is an odd use of a reference count, the patch below should help
> > understand it?
> 
> Yes, it helps, indeed, but I think we have an issue here. See below inline in patch. 
> 
> > 
> > Those perf_mmap objects are created in a batch fashion, it being zero
> > just means it isn't yet mmaped at all, and we check for that before
> > using it.
> > 
> > So, it remains a bug to do a dec for a zeroed refcount, and the
> > refcount_t infrastructure will catch it, which helps tools/.
> > 
> > - Arnaldo
> > 
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 564b924fb48a..5a70f08d2518 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -974,8 +974,19 @@ static struct perf_mmap
> > *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
> >  	if (!map)
> >  		return NULL;
> > 
> > -	for (i = 0; i < evlist->nr_mmaps; i++)
> > +	for (i = 0; i < evlist->nr_mmaps; i++) {
> >  		map[i].fd = -1;
> > +		/*
> > +		 * When the perf_mmap() call is made we 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! */
 
> This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two
> refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero. 
> So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)

Duh, thanks for pointint it out, will leave the comments, remove the
pair of perf_mmap__get()
 
> When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above. 
> 
> Best Regards,
> Elena.
> 
> >  	map->prev = 0;
> >  	map->mask = mp->mask;
> >  	map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
> > >prot,
> > 
> > > > > There are multiple fixes in it to get it to build and test it, so far,
> > > > > with:
> > > > >
> > > > >   perf top -F 15000 -d 0
> > > > >
> > > > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > > > short lived threads with its map_groups, maps, dsos, etc.
> > > > >
> > > > > Now running some build tests in some 36 containers with assorted distros
> > > > > and cross compilers.
> > > >
> > > > Tomorrow I'll inject some refcount errors to test this all.
> > >
> > >
> > > Thank you!
> > >
> > > Best Regards,
> > > Elena.

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Arnaldo Carvalho de Melo]

Em Fri, Feb 24, 2017 at 07:27:32AM +0000, Reshetova, Elena escreveu:
> > Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > > > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > > > Now when new refcount_t type and API are finally merged
> > > > > > > (see include/linux/refcount.h), the following
> > > > > > > patches convert various refcounters in the tools susystem from
> > atomic_t
> > > > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > > > >
> > > > > > Thanks for working on this! I was almost going to jump on doing this
> > > > > > myself!
> > > > > >
> > > > > > I'll try and get this merged ASAP.
> > > > >
> > > > > So, please take a look at my tmp.perf/refcount branch at:
> > > > >
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> > >
> > > I took a look on it and it looks good. Just one thing I want to double check
> > with regards to this commit:
> > >
> > https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
> > 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
> > >
> > > And more specifically to this chunk:
> > >
> > > @@ -937,7 +937,7 @@
> > >  		munmap(map->base,
> > perf_mmap__mmap_len(map));
> > >  		map->base = NULL;
> > >  		map->fd = -1;
> > > -		atomic_set(&map->refcnt, 0);
> > > +		refcount_set(&map->refcnt, 0);
> > >  	}
> > >  	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."
> > 
> > So, this is an odd use of a reference count, the patch below should help
> > understand it?
> 
> Yes, it helps, indeed, but I think we have an issue here. See below inline in patch. 
> 
> > 
> > Those perf_mmap objects are created in a batch fashion, it being zero
> > just means it isn't yet mmaped at all, and we check for that before
> > using it.
> > 
> > So, it remains a bug to do a dec for a zeroed refcount, and the
> > refcount_t infrastructure will catch it, which helps tools/.
> > 
> > - Arnaldo
> > 
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 564b924fb48a..5a70f08d2518 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -974,8 +974,19 @@ static struct perf_mmap
> > *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
> >  	if (!map)
> >  		return NULL;
> > 
> > -	for (i = 0; i < evlist->nr_mmaps; i++)
> > +	for (i = 0; i < evlist->nr_mmaps; i++) {
> >  		map[i].fd = -1;
> > +		/*
> > +		 * When the perf_mmap() call is made we 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! */
 
> This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two
> refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero. 
> So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)

Duh, thanks for pointint it out, will leave the comments, remove the
pair of perf_mmap__get()
 
> When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above. 
> 
> Best Regards,
> Elena.
> 
> >  	map->prev = 0;
> >  	map->mask = mp->mask;
> >  	map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
> > >prot,
> > 
> > > > > There are multiple fixes in it to get it to build and test it, so far,
> > > > > with:
> > > > >
> > > > >   perf top -F 15000 -d 0
> > > > >
> > > > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > > > short lived threads with its map_groups, maps, dsos, etc.
> > > > >
> > > > > Now running some build tests in some 36 containers with assorted distros
> > > > > and cross compilers.
> > > >
> > > > Tomorrow I'll inject some refcount errors to test this all.
> > >
> > >
> > > Thank you!
> > >
> > > Best Regards,
> > > Elena.

[tip:perf/core] perf evlist: Clarify a bit the use of perf_mmap->refcnt

[tip-bot for Arnaldo Carvalho de Melo]

Commit-ID:  4738ca30b4a7a113084d7863846175094f95c62f
Gitweb:     http://git.kernel.org/tip/4738ca30b4a7a113084d7863846175094f95c62f
Author:     Arnaldo Carvalho de Melo <acme@redhat.com>
AuthorDate: Thu, 23 Feb 2017 13:24:34 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Elena Reshetova <elena.reshetova@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170223162344.GD3595@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 564b924..50420cd 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -974,8 +974,19 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
 	if (!map)
 		return NULL;
 
-	for (i = 0; i < evlist->nr_mmaps; i++)
+	for (i = 0; i < evlist->nr_mmaps; i++) {
 		map[i].fd = -1;
+		/*
+		 * When the perf_mmap() call is made we grab one refcount, plus
+		 * one extra to let perf_evlist__mmap_consume() get the last
+		 * events after all real references (perf_mmap__get()) are
+		 * dropped.
+		 *
+		 * Each PERF_EVENT_IOC_SET_OUTPUT points to this mmap and
+		 * thus does perf_mmap__get() on it.
+		 */
+		refcount_set(&map[i].refcnt, 0);
+	}
 	return map;
 }
 

Re: [PATCH 0/9] tools subsystem refcounter conversions

[Peter Zijlstra]

On Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova wrote:
> Now when new refcount_t type and API are finally merged
> (see include/linux/refcount.h), the following
> patches convert various refcounters in the tools susystem from atomic_t
> to refcount_t. By doing this we prevent intentional or accidental
> underflows or overflows that can led to use-after-free vulnerabilities.
> 
> The below patches are fully independent and can be cherry-picked separately.
> Since we convert all kernel subsystems in the same fashion, resulting
> in about 300 patches, we have to group them for sending at least in some
> fashion to be manageable. Please excuse the long cc list.
> 
> Elena Reshetova (9):
>   tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
>   tools: convert cpu_map.refcnt from atomic_t to refcount_t
>   tools: convert comm_str.refcnt from atomic_t to refcount_t
>   tools: convert dso.refcnt from atomic_t to refcount_t
>   tools: convert map.refcnt from atomic_t to refcount_t
>   tools: convert map_groups.refcnt from atomic_t to refcount_t
>   tools: convert perf_map.refcnt from atomic_t to refcount_t
>   tools: convert thread.refcnt from atomic_t to refcount_t
>   tools: convert thread_map.refcnt from atomic_t to refcount_t
> 
>  tools/perf/util/cgroup.c     |  6 +++---
>  tools/perf/util/cgroup.h     |  4 ++--
>  tools/perf/util/comm.c       | 13 +++++--------
>  tools/perf/util/cpumap.c     | 16 ++++++++--------
>  tools/perf/util/cpumap.h     |  4 ++--
>  tools/perf/util/dso.c        |  6 +++---
>  tools/perf/util/dso.h        |  4 ++--
>  tools/perf/util/evlist.c     | 18 +++++++++---------
>  tools/perf/util/evlist.h     |  4 ++--
>  tools/perf/util/map.c        | 10 +++++-----
>  tools/perf/util/map.h        | 10 +++++-----
>  tools/perf/util/thread.c     |  6 +++---
>  tools/perf/util/thread.h     |  4 ++--
>  tools/perf/util/thread_map.c | 20 ++++++++++----------
>  tools/perf/util/thread_map.h |  4 ++--

This is userspace code; did you build this? I see a distinct lack of
adding refcount.h to the userspace headers.

RE: [PATCH 0/9] tools subsystem refcounter conversions

[Reshetova, Elena]

> On Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova wrote:
> > Now when new refcount_t type and API are finally merged
> > (see include/linux/refcount.h), the following
> > patches convert various refcounters in the tools susystem from atomic_t
> > to refcount_t. By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> >
> > The below patches are fully independent and can be cherry-picked
> separately.
> > Since we convert all kernel subsystems in the same fashion, resulting
> > in about 300 patches, we have to group them for sending at least in some
> > fashion to be manageable. Please excuse the long cc list.
> >
> > Elena Reshetova (9):
> >   tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
> >   tools: convert cpu_map.refcnt from atomic_t to refcount_t
> >   tools: convert comm_str.refcnt from atomic_t to refcount_t
> >   tools: convert dso.refcnt from atomic_t to refcount_t
> >   tools: convert map.refcnt from atomic_t to refcount_t
> >   tools: convert map_groups.refcnt from atomic_t to refcount_t
> >   tools: convert perf_map.refcnt from atomic_t to refcount_t
> >   tools: convert thread.refcnt from atomic_t to refcount_t
> >   tools: convert thread_map.refcnt from atomic_t to refcount_t
> >
> >  tools/perf/util/cgroup.c     |  6 +++---
> >  tools/perf/util/cgroup.h     |  4 ++--
> >  tools/perf/util/comm.c       | 13 +++++--------
> >  tools/perf/util/cpumap.c     | 16 ++++++++--------
> >  tools/perf/util/cpumap.h     |  4 ++--
> >  tools/perf/util/dso.c        |  6 +++---
> >  tools/perf/util/dso.h        |  4 ++--
> >  tools/perf/util/evlist.c     | 18 +++++++++---------
> >  tools/perf/util/evlist.h     |  4 ++--
> >  tools/perf/util/map.c        | 10 +++++-----
> >  tools/perf/util/map.h        | 10 +++++-----
> >  tools/perf/util/thread.c     |  6 +++---
> >  tools/perf/util/thread.h     |  4 ++--
> >  tools/perf/util/thread_map.c | 20 ++++++++++----------
> >  tools/perf/util/thread_map.h |  4 ++--
> 
> This is userspace code; did you build this? I see a distinct lack of
> adding refcount.h to the userspace headers.

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!

RE: [PATCH 0/9] tools subsystem refcounter conversions

[Reshetova, Elena]

> On Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova wrote:
> > Now when new refcount_t type and API are finally merged
> > (see include/linux/refcount.h), the following
> > patches convert various refcounters in the tools susystem from atomic_t
> > to refcount_t. By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> >
> > The below patches are fully independent and can be cherry-picked
> separately.
> > Since we convert all kernel subsystems in the same fashion, resulting
> > in about 300 patches, we have to group them for sending at least in some
> > fashion to be manageable. Please excuse the long cc list.
> >
> > Elena Reshetova (9):
> >   tools: convert cgroup_sel.refcnt from atomic_t to refcount_t
> >   tools: convert cpu_map.refcnt from atomic_t to refcount_t
> >   tools: convert comm_str.refcnt from atomic_t to refcount_t
> >   tools: convert dso.refcnt from atomic_t to refcount_t
> >   tools: convert map.refcnt from atomic_t to refcount_t
> >   tools: convert map_groups.refcnt from atomic_t to refcount_t
> >   tools: convert perf_map.refcnt from atomic_t to refcount_t
> >   tools: convert thread.refcnt from atomic_t to refcount_t
> >   tools: convert thread_map.refcnt from atomic_t to refcount_t
> >
> >  tools/perf/util/cgroup.c     |  6 +++---
> >  tools/perf/util/cgroup.h     |  4 ++--
> >  tools/perf/util/comm.c       | 13 +++++--------
> >  tools/perf/util/cpumap.c     | 16 ++++++++--------
> >  tools/perf/util/cpumap.h     |  4 ++--
> >  tools/perf/util/dso.c        |  6 +++---
> >  tools/perf/util/dso.h        |  4 ++--
> >  tools/perf/util/evlist.c     | 18 +++++++++---------
> >  tools/perf/util/evlist.h     |  4 ++--
> >  tools/perf/util/map.c        | 10 +++++-----
> >  tools/perf/util/map.h        | 10 +++++-----
> >  tools/perf/util/thread.c     |  6 +++---
> >  tools/perf/util/thread.h     |  4 ++--
> >  tools/perf/util/thread_map.c | 20 ++++++++++----------
> >  tools/perf/util/thread_map.h |  4 ++--
> 
> This is userspace code; did you build this? I see a distinct lack of
> adding refcount.h to the userspace headers.

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!