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;
Subject: Re: [PATCH] fix use-after-free in perf_sched__lat
To:     Jiri Olsa <jolsa@redhat.com>
CC:     <acme@kernel.org>, <namhyung@kernel.org>,
        <alexander.shishkin@linux.intel.com>, <peterz@infradead.org>,
        <mingo@redhat.com>, <linux-kernel@vger.kernel.org>,
        <xiezhipeng1@huawei.com>
References: <20190503023555.24736-1-liwei391@huawei.com>
 <20190507085153.GC17416@krava>
From:   "liwei (GF)" <liwei391@huawei.com>
Message-ID: <87032042-d814-dde9-8c35-31794e91209f@huawei.com>
Date:   Wed, 8 May 2019 11:45:15 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <20190507085153.GC17416@krava>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jiri，
Thanks for your reply.

On 2019/5/7 16:51, Jiri Olsa wrote:
> On Fri, May 03, 2019 at 10:35:55AM +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
> 
> what's this for?

When we set env "MALLOC_MMAP_THRESHOLD_=0"，the memory-allocation functions employ
mmap instead of increasing the program break using sbrk what may be maintained
with cache. Thus it can report "Segmentation fault" immediately when going into
this use-after-free code.

> 
>> 	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>
>> ---
>>  tools/perf/builtin-sched.c | 44 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
>> index cbf39dab19c1..17849ae2eb1e 100644
>> --- a/tools/perf/builtin-sched.c
>> +++ b/tools/perf/builtin-sched.c
>> @@ -3130,11 +3130,48 @@ static void perf_sched__merge_lat(struct perf_sched *sched)
>>  static int perf_sched__lat(struct perf_sched *sched)
>>  {
>>  	struct rb_node *next;
>> +	const struct perf_evsel_str_handler handlers[] = {
>> +		{ "sched:sched_switch",	      process_sched_switch_event, },
>> +		{ "sched:sched_stat_runtime", process_sched_runtime_event, },
>> +		{ "sched:sched_wakeup",	      process_sched_wakeup_event, },
>> +		{ "sched:sched_wakeup_new",   process_sched_wakeup_event, },
>> +		{ "sched:sched_migrate_task", process_sched_migrate_task_event, },
>> +	};
>> +	struct perf_session *session;
>> +	struct perf_data data = {
>> +		.file      = {
>> +			.path = input_name,
>> +		},
> 
> I can't compile this:
> 
> builtin-sched.c: In function ‘perf_sched__lat’:
> builtin-sched.c:3144:12: error: initialization discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
>     .path = input_name,
> 
Sorry, this place has been changed recently in mainline code, i will update to
the latest code.

> 
>> +		.mode      = PERF_DATA_MODE_READ,
>> +		.force     = sched->force,
>> +	};
>> +	int rc = -1;
>>  
>>  	setup_pager();
>>  
>> -	if (perf_sched__read_events(sched))
> 
> so it's basically perf_sched__read_events code in here now, right?
> 
> might be better to add __perf_sched__read_events function
> that would take session argument, something like:
> 
>         session = perf_session__new(&data, false, &sched->tool);
> 	...
> 	__perf_sched__read_events(sched, session)
> 	...
> 	perf_session__delete(session);
> 
> to avoid the code ducplication

Yes, what you suggest is reasonable, i will send a v2 with your suggestion soon.

Thanks，
Wei