linux-kernel - Re: [RFC v2] perf: Rewrite core context handling

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]

Message-ID: <YwO0Nh5LHZ7uzWvm@worktop.programming.kicks-ass.net>
Date:   Mon, 22 Aug 2022 18:52:06 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Ravi Bangoria <ravi.bangoria@....com>
Cc:     acme@...nel.org, alexander.shishkin@...ux.intel.com,
        jolsa@...hat.com, namhyung@...nel.org, songliubraving@...com,
        eranian@...gle.com, alexey.budankov@...ux.intel.com,
        ak@...ux.intel.com, mark.rutland@....com, megha.dey@...el.com,
        frederic@...nel.org, maddy@...ux.ibm.com, irogers@...gle.com,
        kim.phillips@....com, linux-kernel@...r.kernel.org,
        santosh.shukla@....com
Subject: Re: [RFC v2] perf: Rewrite core context handling

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.