Fix handling of page wirings exec_map_first_page().

[markj]

Summary

If the first page is not fully valid, or the attempt to page in
subsequent pages fails, we currently free the pages. However, the pages
may have already been resident and wired by mlock(). This can happen if
execve() attempts to activate an image which has been mlock()ed and
truncated.

Test Plan

syzkaller found the bug and generated a reproducer. I verified that this
patch fixes the crash triggered by that reproducer.

[kib]

What is the point of deactivating them ? Inactive scan frees invalid pages upon encountering them.

[markj]

In D21767#474999, @kib wrote:

What is the point of deactivating them ? Inactive scan frees invalid pages upon encountering them.

As opposed to doing nothing? vm_page_deactivate_noreuse() ensures that the pages skip LRU, so the page daemon (which handles races with concurrent wirings) will free them sooner.

[kib]

In D21767#475000, @markj wrote:

In D21767#474999, @kib wrote:

What is the point of deactivating them ? Inactive scan frees invalid pages upon encountering them.

As opposed to doing nothing? vm_page_deactivate_noreuse() ensures that the pages skip LRU, so the page daemon (which handles races with concurrent wirings) will free them sooner.

Instead of freeing them. If unwire returned true.

[markj]

In D21767#475003, @kib wrote:

In D21767#475000, @markj wrote:

In D21767#474999, @kib wrote:

What is the point of deactivating them ? Inactive scan frees invalid pages upon encountering them.

As opposed to doing nothing? vm_page_deactivate_noreuse() ensures that the pages skip LRU, so the page daemon (which handles races with concurrent wirings) will free them sooner.

Instead of freeing them. If unwire returned true.

Indeed, in the first block we could simply free the page if vm_page_unwire_noq() returns true.

I thought the second block was trickier, but now I see it is sufficient to simply check vm_page_wired().

[markj]

Instead of deactivating pages, check for wirings before freeing them.

[markj]

Unbusy pages if they are not freed.

[markj]

sys/kern/kern_exec.c

1012

Obviously this should be ma[0], I did not commit the typo fix before uploading.

[alc]

Do we actually allow truncation on a running executable file?

[kib]

In D21767#475356, @alc wrote:

Do we actually allow truncation on a running executable file?

No for truncation, yes for invalidation.

[markj]

In D21767#475357, @kib wrote:

In D21767#475356, @alc wrote:

Do we actually allow truncation on a running executable file?

No for truncation, yes for invalidation.

My review description is confusing. A concurrent truncation is not needed to trigger the problem. The key is that an mlock() of a read-only mapping of a file forces the backing pages to stay resident after a truncation. A subsequent execve() of the file will trigger the assertion, since the pages will be invalid and wired.

[markj]

Here is a program that reproduces the panic: https://reviews.freebsd.org/P319

The program that syzkaller generated uses fewer system calls but is multi-threaded.

[kib]

In D21767#475394, @markj wrote:

Here is a program that reproduces the panic: https://reviews.freebsd.org/P319

The program that syzkaller generated uses fewer system calls but is multi-threaded.

Yes, and I think that a similar effect can be achieved with msync(MS_INVALIDATE). BTW, we can disable MS_INVALIDATE on text mappings now.

That said, what is the use of msync(MS_INVALIDATE) except for writing test cases ?

[markj]

In D21767#475487, @kib wrote:

In D21767#475394, @markj wrote:

Here is a program that reproduces the panic: https://reviews.freebsd.org/P319

The program that syzkaller generated uses fewer system calls but is multi-threaded.

Yes, and I think that a similar effect can be achieved with msync(MS_INVALIDATE). BTW, we can disable MS_INVALIDATE on text mappings now.

Probably, though note that msync(MS_INVALIDATE) fails if part of the region is user-wired.

Are you suggesting to check vn_writechk() in vm_object_sync()?

That said, what is the use of msync(MS_INVALIDATE) except for writing test cases ?

I'm not sure. I wonder if it made more sense before the VM and buffer cache unification. Maybe it is still useful for some types of device mappings where userspace must manage coherence.