Messages in this thread | | | Date | Tue, 2 Aug 2022 11:41:42 +0530 | From | Ravi Bangoria <> | 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
| |