Hi Peter,
On Wed, Dec 06, 2017 at 02:47:06PM +0100, Peter Zijlstra wrote:
> 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.
Hmm.. did you mean that get_callchain_buffers() returns an error?
AFAICS it cannot fail when it sees count > 1 (and callchain_cpus_
entries is allocated). So I think it won't decrease the count and
should be fine.
Thanks,
Namhyung
