Messages in this thread | | | Date | Wed, 6 Dec 2017 23:31:30 +0900 | From | Namhyung Kim <> | Subject | Re: BUG: KASAN: slab-out-of-bounds in perf_callchain_user+0x494/0x530 |
| |
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
| |