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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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.