Re: [PATCH 2/8] perf: Sanitize get_callchain_buffer()

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Aug 01, 2013 at 03:01:46PM +0200, Jiri Olsa wrote:
> On Tue, Jul 23, 2013 at 02:31:00AM +0200, Frederic Weisbecker wrote:
> > In case of allocation failure, get_callchain_buffer() keeps the
> > refcount incremented for the current event.
> > 
> > As a result, when get_callchain_buffers() returns an error,
> > we must cleanup what it did by cancelling its last refcount
> > with a call to put_callchain_buffers().
> > 
> > This is a hack in order to be able to call free_event()
> > after that failure.
> > 
> > The original purpose of that was to simplify the failure
> > path. But this error handling is actually counter intuitive,
> > ugly and not very easy to follow because one expect to
> > see the resources used to perform a service to be cleaned
> > by the callee if case of failure, not by the caller.
> > 
> > So lets clean this up by cancelling the refcount from
> > get_callchain_buffer() in case of failure. And correctly free
> > the event accordingly in perf_event_alloc().
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Stephane Eranian <eranian@google.com>
> > ---
> >  kernel/events/callchain.c |    2 ++
> >  kernel/events/core.c      |   41 +++++++++++++++++++++--------------------
> >  2 files changed, 23 insertions(+), 20 deletions(-)
> > 
> > diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
> > index c772061..76a8bc5 100644
> > --- a/kernel/events/callchain.c
> > +++ b/kernel/events/callchain.c
> > @@ -117,6 +117,8 @@ int get_callchain_buffers(void)
> >  	err = alloc_callchain_buffers();
> >  exit:
> >  	mutex_unlock(&callchain_mutex);
> > +	if (err)
> > +		atomic_dec(&nr_callchain_events);
> 
> shouldn't we touch this under above lock?

Right, we should move that under the lock, or another user of the callchains may see that we failed
the allocation and simply give up instead of retrying.

> 
> also that above hunk decrements nr_callchain_events
> also for following case:
> 
>         count = atomic_inc_return(&nr_callchain_events);
>         if (WARN_ON_ONCE(count < 1)) {
>                 err = -EINVAL;
>                 goto exit;
>         }
> 
> seems like it screws the count

I'm not sure what you mean here. You mean that it could be negative when the dec is done
outside the lock?

> 
> jirka