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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f994d403-df7b-d88e-8324-c29d0ef2034e@amd.com>
Date:   Tue, 2 Aug 2022 11:41:42 +0530
From:   Ravi Bangoria <ravi.bangoria@....com>
To:     Peter Zijlstra <peterz@...radead.org>
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, ravi.bangoria@....com
Subject: Re: [RFC v2] perf: Rewrite core context handling


> pulling up the ctx->mutex makes things simpler, but also violates the
> locking order vs exec_update_lock.
> 
> Pull that lock up as well...

I'm not able to apply this patch as is but I get the idea. Few
questions below...

> 
> ---
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -12254,13 +12254,29 @@ SYSCALL_DEFINE5(perf_event_open,
>  	if (pmu->task_ctx_nr == perf_sw_context)
>  		event->event_caps |= PERF_EV_CAP_SOFTWARE;
>  
> +	if (task) {
> +		err = down_read_interruptible(&task->signal->exec_update_lock);
> +		if (err)
> +			goto err_alloc;
> +
> +		/*
> +		 * 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;
> +	}
> +
>  	/*
>  	 * Get the target context (task or percpu):
>  	 */
>  	ctx = find_get_context(task, event);
>  	if (IS_ERR(ctx)) {
>  		err = PTR_ERR(ctx);
> -		goto err_alloc;
> +		goto err_cred;
>  	}
>  
>  	mutex_lock(&ctx->mutex);
> @@ -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?

> -
>  	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?

Thanks,
Ravi