Re: [RFC v2] perf: Rewrite core context handling

Ravi Bangoria
2022-08-02T06:12:05Z

> 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