linux-kernel - Re: [PATCH] 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] [day] [month] [year] [list]

Message-ID: <YU3QyTz5Lo0L+6fg@hirez.programming.kicks-ass.net>
Date:   Fri, 24 Sep 2021 15:21:13 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Song Liu <songliubraving@...com>
Cc:     open list <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] perf/core: fix userpage->time_enabled of inactive events

On Thu, Sep 23, 2021 at 05:47:40PM +0000, Song Liu wrote:

> > 
> > So we already iterate all the right events. So I'm thinking we can do
> > something like the below, hmm?
> > 
> > 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 0c000cb01eeb..4d1e962c2ebe 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -3707,6 +3712,28 @@ static noinline int visit_groups_merge(struct perf_cpu_context *cpuctx,
> > 	return 0;
> > }
> > 
> > +static inline bool event_update_userpage(struct perf_event *event)
> > +{
> > +	if (!atomic_read(&event->mmap_count))
> > +		return false;
> 
> Technically, the user could mmap a sibling but not the group leader, right?
> It is weird though. 

Yeah, I went with the same logic that a disabled leader disables the
whole group, I suppose we can revisit if anybody ever gets a sane
use-case for this.

> There is also a corner case we didn't cover. If the user enable the event
> before mmap it. The event is first scheduled in via:
> 
> __perf_event_enable (or __perf_install_in_context)
>    -> ctx_resched
>       -> perf_event_sched_in
>           -> ctx_sched_in
> 
> but it doesn't have mmap_count yet. .And we won't call perf_event_update_userpage
> for it before the first rotation after mmap. As a result, the user page will
> contain garbage data before the first rotation. 

That was already a problem, because without the mmap, the event->rb will
be NULL and perf_event_update_userpage() will not actually do anything.

Should not the mmap() itself update the state before populating those
fields for the first time?

> Other than this corner case, this version works well in my tests. Shall I send 
> v2 in this version?

Please,