Em Fri, Feb 24, 2017 at 07:27:32AM +0000, Reshetova, Elena escreveu:
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo escreveu:
Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
escreveu:
Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu: > Now when new refcount_t type and API are finally merged > (see include/linux/refcount.h), the following > patches convert various refcounters in the tools susystem from
atomic_t
> to refcount_t. By doing this we prevent intentional or accidental > underflows or overflows that can led to use-after-free vulnerabilities.
Thanks for working on this! I was almost going to jump on doing this myself!
I'll try and get this merged ASAP.
So, please take a look at my tmp.perf/refcount branch at:
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
I took a look on it and it looks good. Just one thing I want to double check
with regards to this commit:
https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
And more specifically to this chunk:
@@ -937,7 +937,7 @@ munmap(map->base,
perf_mmap__mmap_len(map));
map->base = NULL; map->fd = -1;
atomic_set(&map->refcnt, 0);
refcount_set(&map->refcnt, 0);}
So, when the refcount set to zero in this place, what exactly happens to the
perf_map object after?
I just want to double check that we don't have another hiding reusage case
here when refcounter later on is simply incremented vs. set to "2."
So, this is an odd use of a reference count, the patch below should help understand it?
Yes, it helps, indeed, but I think we have an issue here. See below inline in patch.
Those perf_mmap objects are created in a batch fashion, it being zero just means it isn't yet mmaped at all, and we check for that before using it.
So, it remains a bug to do a dec for a zeroed refcount, and the refcount_t infrastructure will catch it, which helps tools/.
- Arnaldo
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 564b924fb48a..5a70f08d2518 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -974,8 +974,19 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist) if (!map) return NULL;
- for (i = 0; i < evlist->nr_mmaps; i++)
- for (i = 0; i < evlist->nr_mmaps; i++) { map[i].fd = -1;
/** When the perf_mmap() call is made we grabone refcount, plus
* one extra to let perf_evlist__mmap_consume()get the last
* events after all real references(perf_mmap__get()) are
* dropped.** Each PERF_EVENT_IOC_SET_OUTPUT points tothis mmap and
* thus does perf_mmap__get() on it.*/refcount_set(&map[i].refcnt, 0);- } return map;
}
@@ -988,6 +999,7 @@ struct mmap_params { static int perf_mmap__mmap(struct perf_mmap *map, struct mmap_params *mp, int fd) {
- perf_mmap__get(map); /*
- The last one will be done at perf_evlist__mmap_consume(),
so that we * make sure we don't prevent tools from consuming every last event in @@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap *map, * evlist layer can't just drop it when filtering events in * perf_evlist__filter_pollfd(). */
- refcount_set(&map->refcnt, 2);
- perf_mmap__get(map); /* This is not a dup, see the comment
above! */
This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero. So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)
Duh, thanks for pointint it out, will leave the comments, remove the pair of perf_mmap__get()
When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above.
Best Regards, Elena.
map->prev = 0; map->mask = mp->mask; map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
prot,
There are multiple fixes in it to get it to build and test it, so far, with:
perf top -F 15000 -d 0
while doing kernel builds and tight usleep 1 loops to create lots of short lived threads with its map_groups, maps, dsos, etc.
Now running some build tests in some 36 containers with assorted distros and cross compilers.
Tomorrow I'll inject some refcount errors to test this all.
Thank you!
Best Regards, Elena.