Details

Reviewers

markj
kib

Commits

rS344391: Unwire the kcov buffer when freeing the info struct.
rS344390: Call pmap_qenter for each page when creating the kcov buffer.

Summary

Without this we leak physical pages. This can lead to the kernel locking
up as it has no available physical memory to back allocations.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 22583
Build 21713: arc lint + arc unit

Event Timeline

andrew created this revision.Feb 19 2019, 7:33 PM

Harbormaster completed remote builds in B22583: Diff 54084.Feb 19 2019, 7:34 PM

markj added inline comments.Feb 19 2019, 7:43 PM

sys/kern/kern_kcov.c
382	The object is newly allocated, so there's no reason to use vm_page_grab() - use vm_page_alloc() instead.
383	VM_ALLOC_ZERO is an advisory flag; the caller is responsible for zeroing the page if the allocator could not supply a zero-filled page.
407	Use vm_page_unwire_noq() here. The pages are about to be freed, so there's no reason for them to participate in LRU. AFAIK the page daemon doesn't really expect to find pages from an OBJT_PHYS object in the queues, though I don't think they will cause any problems so long as they are not marked dirty.

kib added inline comments.Feb 19 2019, 7:47 PM

sys/kern/kern_kcov.c
383	Well, this is the reason to use _grab and not _alloc. _grab handles !PG_ZERO transparently. Intent was that non-vm callers would prefer _grab.

markj added inline comments.Feb 19 2019, 7:49 PM

sys/kern/kern_kcov.c
383	Ah, I forgot. Sorry for the noise.

Use vm_page_unwire_noq

Harbormaster completed remote builds in B22584: Diff 54087.Feb 19 2019, 8:04 PM

tuexen added inline comments.Feb 19 2019, 9:26 PM

sys/kern/kern_kcov.c

I think

#include <vm/vm_param.h>

is missing here. At least I get on amd64 compile errors like

--- kern_kcov.o ---
../../../kern/kern_kcov.c:406:4: error: use of undeclared identifier 'PA_LOCK_COUNT'
                        vm_page_lock(info->m[i]);
                        ^
../../../vm/vm_page.h:313:35: note: expanded from macro 'vm_page_lock'
#define vm_page_lock(m)         mtx_lock(vm_page_lockptr((m)))
                                         ^

without it.

Add a missing header

Harbormaster completed remote builds in B22610: Diff 54128.Feb 20 2019, 9:37 AM

andrew added inline comments.Feb 20 2019, 9:38 AM

sys/kern/kern_kcov.c
55	Yes. This is extracted from a larger patch, so I missed this header is needed.

Use vm_object_reference rather than manual tracking of the mmap state

Harbormaster completed remote builds in B22611: Diff 54131.Feb 20 2019, 10:20 AM

Using this version, the memory lockup observed does not occur anymore. Tested with running kcovtrace /usr/bin/uptime in a loop.

kib added inline comments.Feb 20 2019, 11:56 AM

sys/kern/kern_kcov.c
125	Why do you need the array ? You have the object where all your pages are put, so you can iterate over the pages of the object.

Use vm_page_lookup to find the page

Harbormaster completed remote builds in B22619: Diff 54138.Feb 20 2019, 3:37 PM

markj added inline comments.Feb 20 2019, 3:42 PM

sys/kern/kern_kcov.c
405	You can do a lookup on pindex 0, then just use vm_page_next() rather than a separate lookup for each one.

kib added inline comments.Feb 20 2019, 3:43 PM

sys/kern/kern_kcov.c
360–361	This can be vm_page_t m;
379	Then remove this line.
382	m = vm_page_grab(...
387	Remove this line.
404	TAILQ_FOREACH(m, &object->memq, listq) {
405	This lookup is not needed then.

andrew added inline comments.Feb 20 2019, 3:51 PM

sys/kern/kern_kcov.c
379	Will this work with `pmap_qenter`? It takes an array of vm pages.

kib added inline comments.Feb 20 2019, 4:20 PM

sys/kern/kern_kcov.c
379	How many pages usually the code set up ? You can pmap_qenter(m, 1) at the time of grab. The cost is an IPI per the page.

andrew added inline comments.Feb 20 2019, 4:33 PM

sys/kern/kern_kcov.c
379	The buffer can be up to 128MB, although syzkaller normally uses 2MB buffers.

kib added inline comments.Feb 20 2019, 4:42 PM

sys/kern/kern_kcov.c
379	For 128MB, you are going to malloc() 256K linear buffer, which is some kind of strength. For 2M buffers it is 511 extra IPIs, while the allocation is 4K, which is more reasonable. If you are definitely need to support 128M, I would say that per-page allocation and mapping is the only route.