Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 23 May 2019 11:50:13 +0900
From:   Namhyung Kim <namhyung@kernel.org>
To:     Arnaldo Carvalho de Melo <acme@kernel.org>
Cc:     Wei Li <liwei391@huawei.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        linux-kernel@vger.kernel.org, xiezhipeng1@huawei.com
Subject: Re: [PATCH v2] fix use-after-free in perf_sched__lat
Message-ID: <20190523025011.GC196218@google.com>
References: <20190508143648.8153-1-liwei391@huawei.com>
 <20190522065555.GA206606@google.com>
 <20190522110823.GR8945@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20190522110823.GR8945@kernel.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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