Messages in this thread |  | | From | Namhyung Kim <> | Subject | Re: [PATCH 10/13] tracing/uprobes: Fetch args before reserving a ring buffer | Date | Mon, 04 Nov 2013 17:06:05 +0900 |
| |
Hi Oleg,
On Thu, 31 Oct 2013 19:16:54 +0100, Oleg Nesterov wrote: > On 10/29, Namhyung Kim wrote: >> >> @@ -630,6 +653,19 @@ probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter) >> if (trace_probe_is_enabled(&tu->p)) >> return -EINTR; >> >> + if (atomic_inc_return(&uprobe_buffer_ref) == 1) { >> + int cpu; >> + >> + uprobe_cpu_buffer = __alloc_percpu(PAGE_SIZE, PAGE_SIZE); >> + if (uprobe_cpu_buffer == NULL) { >> + atomic_dec(&uprobe_buffer_ref); >> + return -ENOMEM; >> + } >> + >> + for_each_possible_cpu(cpu) >> + mutex_init(&per_cpu(uprobe_cpu_mutex, cpu)); >> + } >> + >> WARN_ON(!uprobe_filter_is_empty(&tu->filter)); >> >> tu->p.flags |= flag; >> @@ -646,6 +682,11 @@ static void probe_event_disable(struct trace_uprobe *tu, int flag) >> if (!trace_probe_is_enabled(&tu->p)) >> return; >> >> + if (atomic_dec_and_test(&uprobe_buffer_ref)) { >> + free_percpu(uprobe_cpu_buffer); >> + uprobe_cpu_buffer = NULL; >> + } >> + >> WARN_ON(!uprobe_filter_is_empty(&tu->filter)); > > Do we really need atomic_t? probe_event_enable/disable is called under > event_mutex and we rely on this fact anyway.
Looking at the code, it seems probe_event_enable/disable() is called without event_mutex when it called from sys_perf_event_open(). So we still need to protect refcount from concurrent accesses.
> > Otherwise this logic looks racy even with atomic_t, another thread could > use the uninitialized uprobe_cpu_buffer/mutex if it registers another probe > and the handler runs before we complete the initialization, no?
But yeah, this is indeed a problem. Thanks for pointing it out. I'll put a mutex to prevent such cases.
Thanks, Namhyung
|  |