Page MenuHomeFreeBSD

Unwire pages when cleaning up the kcov state
ClosedPublic

Authored by andrew on Feb 19 2019, 7:33 PM.

Details

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 22611
Build 21726: arc lint + arc unit

Event Timeline

sys/kern/kern_kcov.c
384

The object is newly allocated, so there's no reason to use vm_page_grab() - use vm_page_alloc() instead.

385

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.

418

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.

sys/kern/kern_kcov.c
385

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.

sys/kern/kern_kcov.c
385

Ah, I forgot. Sorry for the noise.

sys/kern/kern_kcov.c
55

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.

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

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

sys/kern/kern_kcov.c
126

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

sys/kern/kern_kcov.c
407

You can do a lookup on pindex 0, then just use vm_page_next() rather than a separate lookup for each one.

sys/kern/kern_kcov.c
362–363

This can be vm_page_t m;

381

Then remove this line.

384

m = vm_page_grab(...

389

Remove this line.

406
	TAILQ_FOREACH(m, &object->memq, listq) {
407

This lookup is not needed then.

sys/kern/kern_kcov.c
381

Will this work with pmap_qenter? It takes an array of vm pages.

sys/kern/kern_kcov.c
381

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.

sys/kern/kern_kcov.c
381

The buffer can be up to 128MB, although syzkaller normally uses 2MB buffers.

sys/kern/kern_kcov.c
381

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.

pmap_qenter each page
Use vm_page_next

kib added inline comments.
sys/kern/kern_kcov.c
407

You do not need the counter.

This revision is now accepted and ready to land.Feb 20 2019, 6:10 PM

Latest version tested again with running kcovtrace /usr/bin/uptime in a loop and in addition with running syzkaller. Both work fine.

This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state Needs Review.Feb 20 2019, 10:41 PM
This revision was automatically updated to reflect the committed changes.