| Message ID | 1487691303-31858-2-git-send-email-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Tue, Feb 21, 2017 at 05:34:55PM +0200, Elena Reshetova escreveu: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. > #define __CGROUP_H__ > > -#include <linux/atomic.h> > +#include <linux/refcount.h> So this is the first one, I was expecting the copy from include/linux/refcount.h to be made to tools/include/linux/refcount.h, as was done for tools/include/linux/atomic.h and all the other stuff in tools/include/ See: commit c4b6014e8bb0c8d47fe5c71ebc604f31091e5d3f Author: Arnaldo Carvalho de Melo <acme@redhat.com> Date: Mon Jul 11 10:28:48 2016 -0300 tools: Add copy of perf_event.h to tools/include/linux/ -------------- For one of the reasons we've been doing this. - Arnaldo > struct option; > > struct cgroup_sel { > char *name; > int fd; > - atomic_t refcnt; > + refcount_t refcnt; > }; > > > -- > 2.7.4
> Em 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
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; };