Unwire pages when cleaning up the kcov state

[andrew]

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.

[markj]

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]

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]

sys/kern/kern_kcov.c

383 ↗

(On Diff #54084)

Ah, I forgot. Sorry for the noise.

[andrew]

Use vm_page_unwire_noq

[tuexen]

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]

Add a missing header

[andrew]

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]

Use vm_object_reference rather than manual tracking of the mmap state

[tuexen]

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

[kib]

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]

Use vm_page_lookup to find the page

[markj]

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]

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]

sys/kern/kern_kcov.c

381 ↗

(On Diff #54138)

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

[kib]

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]

sys/kern/kern_kcov.c

381 ↗

(On Diff #54138)

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

[kib]

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.

[andrew]

pmap_qenter each page
Use vm_page_next

[kib]

sys/kern/kern_kcov.c

408 ↗

(On Diff #54143)

You do not need the counter.

[tuexen]

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