linux-kernel - Re: [PATCH 2/8] perf: Sanitize get_callchain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130801132833.GA431@somewhere>
Date:	Thu, 1 Aug 2013 15:28:34 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 2/8] perf: Sanitize get_callchain_buffer()

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@...il.com>
> > Cc: Jiri Olsa <jolsa@...hat.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Namhyung Kim <namhyung@...nel.org>
> > Cc: Ingo Molnar <mingo@...nel.org>
> > Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> > Cc: Stephane Eranian <eranian@...gle.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/