Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 22 Aug 2022 18:52:06 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Ravi Bangoria <ravi.bangoria@amd.com>
Cc:     acme@kernel.org, alexander.shishkin@linux.intel.com,
        jolsa@redhat.com, namhyung@kernel.org, songliubraving@fb.com,
        eranian@google.com, alexey.budankov@linux.intel.com,
        ak@linux.intel.com, mark.rutland@arm.com, megha.dey@intel.com,
        frederic@kernel.org, maddy@linux.ibm.com, irogers@google.com,
        kim.phillips@amd.com, linux-kernel@vger.kernel.org,
        santosh.shukla@amd.com
Subject: Re: [RFC v2] perf: Rewrite core context handling
Message-ID: <YwO0Nh5LHZ7uzWvm@worktop.programming.kicks-ass.net>
References: <20220113134743.1292-1-ravi.bangoria@amd.com>
 <YqdLH+ZU/sf4n0pa@hirez.programming.kicks-ass.net>
 <YqdL8LsOvxNqhz/v@hirez.programming.kicks-ass.net>
 <f994d403-df7b-d88e-8324-c29d0ef2034e@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f994d403-df7b-d88e-8324-c29d0ef2034e@amd.com>
Precedence: bulk

On Tue, Aug 02, 2022 at 11:41:42AM +0530, Ravi Bangoria wrote:

> > @@ -12358,58 +12374,14 @@ SYSCALL_DEFINE5(perf_event_open,
> >  			goto err_context;
> >  	}
> >  
> > -	event_file = anon_inode_getfile("[perf_event]", &perf_fops, event, f_flags);
> > -	if (IS_ERR(event_file)) {
> > -		err = PTR_ERR(event_file);
> > -		event_file = NULL;
> > -		goto err_context;
> > -	}
> > -
> > -	if (task) {
> > -		err = down_read_interruptible(&task->signal->exec_update_lock);
> > -		if (err)
> > -			goto err_file;
> > -
> > -		/*
> > -		 * We must hold exec_update_lock across this and any potential
> > -		 * perf_install_in_context() call for this new event to
> > -		 * serialize against exec() altering our credentials (and the
> > -		 * perf_event_exit_task() that could imply).
> > -		 */
> > -		err = -EACCES;
> > -		if (!perf_check_permission(&attr, task))
> > -			goto err_cred;
> > -	}
> > -
> > -	if (ctx->task == TASK_TOMBSTONE) {
> > -		err = -ESRCH;
> > -		goto err_locked;
> > -	}
> 
> I think we need to keep (ctx->task == TASK_TOMBSTONE) check?

I think so too; in fact the code I have still has it, perhaps it was
there write before this patch?

> > -
> >  	if (!perf_event_validate_size(event)) {
> >  		err = -E2BIG;
> > -		goto err_locked;
> > -	}
> > -
> > -	if (!task) {
> > -		/*
> > -		 * Check if the @cpu we're creating an event for is online.
> > -		 *
> > -		 * We use the perf_cpu_context::ctx::mutex to serialize against
> > -		 * the hotplug notifiers. See perf_event_{init,exit}_cpu().
> > -		 */
> > -		struct perf_cpu_context *cpuctx =
> > -			container_of(ctx, struct perf_cpu_context, ctx);
> > -
> > -		if (!cpuctx->online) {
> > -			err = -ENODEV;
> > -			goto err_locked;
> > -		}
> > +		goto err_context;
> 
> Why did you remove this hunk? We should confirm whether cpu is online or not
> before creating event. No?

Idem.

Perhaps it is best if we look at the end result of all these patches
combined and then I'll fold the lot if we're in agreement and then we
can forget about these intermediate steps.