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.