On Tue, Dec 05, 2017 at 11:47:18PM +0900, Namhyung Kim wrote:
> Sure, I mean the following code:
>
> mutex_lock(&callchain_mutex);
>
> count = atomic_inc_return(&nr_callchain_events);
> if (WARN_ON_ONCE(count < 1)) {
> err = -EINVAL;
> goto exit;
> }
>
> if (count > 1) {
> /* If the allocation failed, give up */
> if (!callchain_cpus_entries)
> err = -ENOMEM;
>
> goto exit;
> }
>
> err = alloc_callchain_buffers();
> exit:
> if (err)
> atomic_dec(&nr_callchain_events);
>
> mutex_unlock(&callchain_mutex);
>
>
> The callchain_cpus_entries is allocated in alloc_callchain_buffers()
> only when the count is 1. But if it failed to allocate, it decrease
> the count so next event would try to allocate it again. Thus it seems
> not possible to see the callchain_cpus_entries being NULL in the
> 'if (count > 1)' block. If you want to make next event give up, it'd
> need to take an additional count IMHO.
There's also a race against put_callchain_buffers() there, consider:
get_callchain_buffers() put_callchain_buffers()
mutex_lock();
inc()
dec_and_test() // false
dec() // 0
And the buffers leak.
