Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 23 Mar 2021 11:32:03 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Marco Elver <elver@google.com>
Cc:     alexander.shishkin@linux.intel.com, acme@kernel.org,
        mingo@redhat.com, jolsa@redhat.com, mark.rutland@arm.com,
        namhyung@kernel.org, tglx@linutronix.de, glider@google.com,
        viro@zeniv.linux.org.uk, arnd@arndb.de, christian@brauner.io,
        dvyukov@google.com, jannh@google.com, axboe@kernel.dk,
        mascasa@google.com, pcc@google.com, irogers@google.com,
        kasan-dev@googlegroups.com, linux-arch@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        x86@kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH RFC v2 8/8] selftests/perf: Add kselftest for
 remove_on_exec
Message-ID: <YFnDo7dczjDzLP68@hirez.programming.kicks-ass.net>
References: <20210310104139.679618-1-elver@google.com>
 <20210310104139.679618-9-elver@google.com>
 <YFiamKX+xYH2HJ4E@elver.google.com>
 <YFjI5qU0z3Q7J/jF@hirez.programming.kicks-ass.net>
 <YFm6aakSRlF2nWtu@elver.google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YFm6aakSRlF2nWtu@elver.google.com>
Precedence: bulk

On Tue, Mar 23, 2021 at 10:52:41AM +0100, Marco Elver wrote:

> with efs->func==__perf_event_enable. I believe it's sufficient to add
> 
> 	mutex_lock(&parent_event->child_mutex);
> 	list_del_init(&event->child_list);
> 	mutex_unlock(&parent_event->child_mutex);
> 
> right before removing from context. With the version I have now (below
> for completeness), extended torture with the above test results in no
> more warnings and the test also passes.
> 

> +	list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) {
> +		struct perf_event *parent_event = event->parent;
> +
> +		if (!event->attr.remove_on_exec)
>  			continue;
>  
> +		if (!is_kernel_event(event))
> +			perf_remove_from_owner(event);
>  
> +		modified = true;
> +
> +		if (parent_event) {
>  			/*
> +			 * Remove event from parent, to avoid race where the
> +			 * parent concurrently iterates through its children to
> +			 * enable, disable, or otherwise modify an event.
>  			 */
> +			mutex_lock(&parent_event->child_mutex);
> +			list_del_init(&event->child_list);
> +			mutex_unlock(&parent_event->child_mutex);
>  		}

		^^^ this, right?

But that's something perf_event_exit_event() alread does. So then you're
worried about the order of things.

> +
> +		perf_remove_from_context(event, !!event->parent * DETACH_GROUP);
> +		perf_event_exit_event(event, ctx, current, true);
>  	}

perf_event_release_kernel() first does perf_remove_from_context() and
then clears the child_list, and that makes sense because if we're there,
there's no external access anymore, the filedesc is gone and nobody will
be iterating child_list anymore.

perf_event_exit_task_context() and perf_event_exit_event() OTOH seem to
rely on ctx->task == TOMBSTONE to sabotage event_function_call() such
that if anybody is iterating the child_list, it'll NOP out.

But here we don't have neither, and thus need to worry about the order
vs child_list iteration.

I suppose we should stick sync_child_event() in there as well.

And at that point there's very little value in still using
perf_event_exit_event()... let me see if there's something to be done
about that.