vm_fault_copy_entry: accept invalid source pages.

[kib]

Summary

Either msync(MS_INVALIDATE) or the object unlock during vnode truncation can expose invalid pages backing wired entries. Accept them, but do not install them into destrination pmap. We must create copied pages in the copy case, because e.g. vm_object_unwire() expects that the entry is fully backed.

Reported by: syzkaller, via emaste
Reported by: [e-mail]

[emaste]

Assuming it was this report panic: invalid dst page 0xfffff8007f14af98 https://syzkaller.appspot.com/bug?id=de0b1fa4863a64133877eb032d3ef876cb70d00a, eventual commit message should contain Reported-by: [e-mail]

[markj]

sys/vm/vm_fault.c

1759 ↗

(On Diff #55174)

Should we zero the invalid region? Otherwise I don't see the purpose of zeroing a completely invalid page.

1785 ↗

(On Diff #55174)

"truncated the backing vnode or shared memory object"

[kib]

sys/vm/vm_fault.c

1759 ↗

(On Diff #55174)

I believe this already have been done for src_m, otherwise it is problematic as is. I mean that dst_m is not worse than src_m.

[markj]

In D19615#420052, @emaste wrote:

Assuming it was this report panic: invalid dst page 0xfffff8007f14af98 https://syzkaller.appspot.com/bug?id=de0b1fa4863a64133877eb032d3ef876cb70d00a, eventual commit message should contain Reported-by: [e-mail]

Dmitry noted that you shouldn't need the dash, i.e., "Reported by: [e-mail]" is fine.

sys/vm/vm_fault.c

1759 ↗

(On Diff #55174)

I don't see the point in checking src_m->valid != 0 then.

[markj]

sys/vm/vm_fault.c

1759 ↗

(On Diff #55174)

I mean, I understand that partially valid pages have zero invalid regions, but we are not mapping the page unless it is completely valid.

[emaste]

In D19615#420152, @markj wrote:

Dmitry noted that you shouldn't need the dash, i.e., "Reported by: [e-mail]" is fine.

Indeed, I just cut and pasted the string from the report.

[kib]

Unconditionally copy src_m to dst_m (ignore valid bits).
Update comment.

[emaste]

Reported by: emaste using syzcaller

Perhaps more correct as "Reported by: syzkaller, via emaste" - this was reported by Google's public syzkaller while "emaste using syzkaller" implies it's something I directly did myself.

[markj]

I'm writing a few test cases for mlock as part of the user wiring work. I added one which does the following:

1. truncate an empty file to 4KB
2. map the file and mlock the mapping
3. write some non-zero bytes to the mapping
4. truncate the file to 0
5. truncate the file to 4KB
6. verify that the mapping is zero-filled

Step 5 does not preemptively fault in pages, which is technically a violation of mlock()'s contract.

[kib]

Peter, could you, please, test this change ?

[pho]

In D19615#420270, @kib wrote:

Peter, could you, please, test this change ?

I ran all of the stress2 tests + a buildworld on amd64. No problems seen.