Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 7 Dec 2017 00:46:14 +0900 | Subject | Re: BUG: KASAN: slab-out-of-bounds in perf_callchain_user+0x494/0x530 |
| |
On Wed, Dec 6, 2017 at 11:31 PM, Namhyung Kim <namhyung@kernel.org> wrote: > 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.
Oh, I missed the max_stack case (since it was dropped in the above). Sorry for the noise..
Thanks, Namhyung
| |