linux-kernel - Re: [PATCH v2] perf/core: fix userpage->time

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]

Date:   Wed, 29 Sep 2021 19:38:12 +0000
From:   Song Liu <songliubraving@...com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     linux-kernel <linux-kernel@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Kernel Team <Kernel-team@...com>,
        Stephane Eranian <eranian@...gle.com>,
        Lucian Grijincu <lucian@...com>
Subject: Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive
 events



> On Sep 29, 2021, at 2:18 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> 
> On Thu, Sep 23, 2021 at 06:28:00PM -0700, Song Liu wrote:
> 
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 1cb1f9b8392e2..d73f986eef7b3 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -3707,6 +3707,46 @@ static noinline int visit_groups_merge(struct perf_cpu_context *cpuctx,
>> 	return 0;
>> }
>> 
>> +static inline bool event_update_userpage(struct perf_event *event)
>> +{
>> +	/*
>> +	 * Checking mmap_count to avoid unnecessary work. This does leave a
>> +	 * corner case: if the event is enabled before mmap(), the first
>> +	 * time the event gets scheduled is via:
>> +	 *
>> +	 *  __perf_event_enable (or __perf_install_in_context)
>> +	 *      -> ctx_resched
>> +	 *         -> perf_event_sched_in
>> +	 *            -> ctx_sched_in
>> +	 *
>> +	 * with mmap_count of 0, so we will skip here. As a result,
>> +	 * userpage->offset is not accurate after mmap and before the
>> +	 * first rotation.
>> +	 *
>> +	 * To avoid the discrepancy of this window, the user space should
>> +	 * mmap the event before enabling it.
>> +	 */
> 
> It seems to me that writing that comment was more work than actually
> fixing perf_mmap() to DTRT, no? AFAICT all we need is something like:
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index fd2ae70fa6c4..1e33c2e6b0dc 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6324,6 +6324,8 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
> 
> 		ring_buffer_attach(event, rb);
> 
> +		perf_event_update_time(event);
> +		perf_set_shadow_time(event, event->ctx);
> 		perf_event_init_userpage(event);
> 		perf_event_update_userpage(event);
> 	} else {

Yeah, this does work. I will fold this in v3. 

Thanks,
Song