Messages in this thread | | | From | Dmitry Vyukov <> | Date | Tue, 19 Jan 2016 13:47:55 +0100 | Subject | Re: [PATCH v3] kernel: add kcov code coverage |
| |
On Mon, Jan 18, 2016 at 10:52 PM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > On Mon, Jan 18, 2016 at 08:28:04PM +0100, Dmitry Vyukov wrote: >> On Mon, Jan 18, 2016 at 1:42 PM, Kirill A. Shutemov >> <kirill@shutemov.name> wrote: >> > On Thu, Jan 14, 2016 at 03:22:21PM +0100, Dmitry Vyukov wrote: >> >> + area = t->kcov_area; >> >> + /* The first u32 is number of subsequent PCs. */ >> >> + pos = READ_ONCE(area[0]) + 1; >> >> + if (likely(pos < t->kcov_size)) { >> >> + area[pos] = (u32)_RET_IP_; >> > >> > If area[0] is UINT32_MAX the condition will be true and you'll make >> > area[0] temporary set to (u32)_RET_IP_. That's may be fun. :) >> >> Well, if user wants to corrupt this region, he is free to do so. But >> it wont't corrupt kernel, right? > > Right. But it's easy enough to avoid overflow too: > "if (likely(pos && pos < t->kcov_size)"
It is easy but I don't understand what we are trying to fix. Such check won't fix operation of kcov if user makes random memory writes. User can also write random values which are not UINT32_MAX to area[0]. In such case coverage will be intermixed and some pieces will be missing. Also this function is quite hot. A single register check and branch is not too expensive, but it does not worth it if it does not bring any value.
>> >> + >> >> + page = vmalloc_to_page(kcov->area + off); >> >> + get_page(page); >> >> + vmf->page = page; >> >> + return 0; >> >> +} >> > >> > BTW, since all pages pre-allocated, you don't really need fault handler -- >> > just setup all pages table enties during mmap. This would shorten warm up >> > period for userspace. See vm_insert_page(). >> >> Done in v4 >> I don't need to get_page() before vm_insert_page(), right? > > Right. vm_insert_page() will take the referenece for you.
ack
| |