Re: [PATCH v2] fix use-after-free in perf_sched__lat

[Namhyung Kim <namhyung@kernel.org>]

On Wed, May 22, 2019 at 08:08:23AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, May 22, 2019 at 03:56:10PM +0900, Namhyung Kim escreveu:
> > On Wed, May 08, 2019 at 10:36:48PM +0800, Wei Li wrote:
> > > After thread is added to machine->threads[i].dead in
> > > __machine__remove_thread, the machine->threads[i].dead is freed
> > > when calling free(session) in perf_session__delete(). So it get a
> > > Segmentation fault when accessing it in thread__put().
> > > 
> > > In this patch, we delay the perf_session__delete until all threads
> > > have been deleted.
> > > 
> > > This can be reproduced by following steps:
> > > 	ulimit -c unlimited
> > > 	export MALLOC_MMAP_THRESHOLD_=0
> > > 	perf sched record sleep 10
> > > 	perf sched latency --sort max
> > > 	Segmentation fault (core dumped)
> > > 
> > > Signed-off-by: Zhipeng Xie <xiezhipeng1@huawei.com>
> > > Signed-off-by: Wei Li <liwei391@huawei.com>
> > 
> > Acked-by: Namhyung Kim <namhyung@kernel.org>
> 
> I'll try to analyse this one soon, but my first impression was that we
> should just grab reference counts when keeping a pointer to those
> threads instead of keeping _all_ threads alive when supposedly we could
> trow away unreferenced data structures.
> 
> But this is just a first impression from just reading the patch
> description, probably I'm missing something.

No, thread refcounting is fine.  We already did it and threads with the
refcount will be accessed only.

But the problem is the head of the list.  After using the thread, the
refcount is gone and thread is removed from the list and destroyed.
However the head of list is in a struct machine which was freed with
session already.

Thanks,
Namhyung


> 
> Thanks for providing instructions on readily triggering the segfault.
> 
> - Arnaldo