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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

andrew created this revision.Feb 19 2019, 7:33 PM
markj added inline comments.Feb 19 2019, 7:43 PM
sys/kern/kern_kcov.c
382 ↗(On Diff #54084)

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

383 ↗(On Diff #54084)

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 ↗(On Diff #54084)

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 ↗(On Diff #54084)

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 ↗(On Diff #54084)

Ah, I forgot. Sorry for the noise.

andrew updated this revision to Diff 54087.Feb 19 2019, 8:04 PM

Use vm_page_unwire_noq

tuexen added inline comments.Feb 19 2019, 9:26 PM
sys/kern/kern_kcov.c
55 ↗(On Diff #54087)

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.

andrew updated this revision to Diff 54128.Feb 20 2019, 9:37 AM

Add a missing header

andrew added inline comments.Feb 20 2019, 9:38 AM
sys/kern/kern_kcov.c
55 ↗(On Diff #54087)

Yes. This is extracted from a larger patch, so I missed this header is needed.

andrew updated this revision to Diff 54131.Feb 20 2019, 10:20 AM

Use vm_object_reference rather than manual tracking of the mmap state

tuexen added a comment.EditedFeb 20 2019, 10:47 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
126 ↗(On Diff #54131)

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.

andrew updated this revision to Diff 54138.Feb 20 2019, 3:36 PM

Use vm_page_lookup to find the page

markj added inline comments.Feb 20 2019, 3:42 PM
sys/kern/kern_kcov.c
410 ↗(On Diff #54138)

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
362 ↗(On Diff #54138)

This can be vm_page_t m;

381 ↗(On Diff #54138)

Then remove this line.

384 ↗(On Diff #54138)

m = vm_page_grab(...

390 ↗(On Diff #54138)

Remove this line.

409 ↗(On Diff #54138)
	TAILQ_FOREACH(m, &object->memq, listq) {
410 ↗(On Diff #54138)

This lookup is not needed then.

andrew added inline comments.Feb 20 2019, 3:51 PM
sys/kern/kern_kcov.c
381 ↗(On Diff #54138)

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
381 ↗(On Diff #54138)

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
381 ↗(On Diff #54138)

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
381 ↗(On Diff #54138)

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.

emaste added a subscriber: emaste.Feb 20 2019, 4:57 PM
andrew updated this revision to Diff 54143.Feb 20 2019, 6:02 PM

pmap_qenter each page
Use vm_page_next

kib accepted this revision.Feb 20 2019, 6:10 PM
kib added inline comments.
sys/kern/kern_kcov.c
408 ↗(On Diff #54143)

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.
andrew reopened this revision.Feb 20 2019, 10:32 PM
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.