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.