Improve MD page fault handlers.

[kib]

Summary

Centralize calculation of signal and ucode delivered on unhandled page fault.

Change the delivered signal for accesses to mapped area after the backing object was truncated.
According to POSIX description for mmap(2),

The system shall always zero-fill any partial page at the end of an object. Further, the system
shall never write out any modified portions of the last page of an object which are beyond its
end. References within the address range starting at pa and continuing for len bytes to whole
pages following the end of an object shall result in delivery of a SIGBUS signal.

An implementation may generate SIGBUS signals when a reference would cause an error in the
mapped object, such as out-of-space condition.

Adjust according to the description, keeping the existing compatibility code for SIGSEGV/SIGBUS on protection failures.

Use the opportunity and move the signal calculation to trap_pfault().

Not sure what better ucode to specify when SIGBUS is delivered for swap reservation failure on CoW.

PR: 211924

[kib]

Handle jilles' notes. Add changes for !x86 (not even compiled).

[alc]

sys/amd64/amd64/trap.c

114 ↗

(On Diff #61808)

The return value is never used.

[alc]

sys/amd64/amd64/trap.c

352–355 ↗

(On Diff #61952)

I think that you actually want to use the return value from trap_pfault() here rather than signo.

[alc]

sys/amd64/amd64/trap.c

792 ↗

(On Diff #61952)

This assignment no longer serves any purpose.

[kib]

Fix bugs pointed out by alc, for amd64 and i386.

[alc]

sys/amd64/amd64/trap.c

191 ↗

(On Diff #61966)

Style: "pf" should be first.

196–198 ↗

(On Diff #61966)

As an aside, I question if any of these three assignments are needed, even by gcc 4.2.1. Arguably, these assignments are harmful in the respect that they guarantee that the compiler won't generate a warning in case a future change failed to assign a meaningful value to one of these variables and that value was needed, i.e., we were generating a signal.

410 ↗

(On Diff #61966)

Style: Eliminate the space after the cast.

692 ↗

(On Diff #61966)

Please consider adding KASSERT((signo != NULL && ucode != NULL) || !usermode, ...) as "documentation" for the new parameters.

741 ↗

(On Diff #61966)

The truncation here is pointless. In fact, the truncation is repeated in vm_fault_trap().

[kib]

sys/amd64/amd64/trap.c

196–198 ↗

(On Diff #61966)

Yes, I believe signo/ucode/addr initializations can be eliminated. But dr6 cannot, because of T_BPTFLT for kernel.

gcc 4.2 should be irrelevant already, I think -Werror was turned off for it recently.

[kib]

More changes requested by alc.

[alc]

sys/amd64/amd64/trap.c

678 ↗

(On Diff #61984)

Please add a comment here describing the meaning of the three possible return values.

sys/amd64/vmm/vmm.c

1414 ↗

(On Diff #61984)

I would suggest renaming vm_fault_hold() to vm_fault().

sys/arm/arm/trap-v4.c

357 ↗

(On Diff #61984)

Shouldn't this be error == KERN_SUCCESS

sys/arm64/arm64/trap.c

214 ↗

(On Diff #61984)

Like amd64, the truncation here is pointless.

[kib]

Rename vm_fault_hold->vm_fault.
Add comment for amd64 trap_pfault.
Minor changes for arm*.

[kib]

• Stop overloading KERN_INVALID_ADDRESS for out-of-object error. This breaks other situations, it seems that the best route is to add a new Mach-style error.
• Switch to use switch in signal decoding in vm_fault_trap().
• Do not assume that map == kernel_map implies signo != NULL. copyin faults on user address, but in kernel mode.

I imported the test from D21624 to ease testing.

[alc]

sys/riscv/riscv/trap.c

220–242 ↗

(On Diff #62028)

I'm surprised to see this snippet brought back to life in the riscv code. We removed it from elsewhere in the source tree almost exactly 4 years ago (r287625).

I would suggest committing the removal of this snippet now, and thus separately from the rest.

sys/vm/vm_fault.c

620 ↗

(On Diff #62028)

1->0

[alc]

In D21566#470372, @kib wrote:

... Note that vm_map_wire() use of vm_fault() caused ktrace recording the wired pages, which IMO is not right.

I agree.

[kib]

Rebase after riscv commit.

[jilles]

sys/amd64/amd64/trap.c

792–793 ↗

(On Diff #61875)

si_pkey contains the protection key (between 0 and 15) that caused the fault.

[kib]

sys/amd64/amd64/trap.c

792–793 ↗

(On Diff #61875)

I cannot provide this information without race, where userspace might update pk for the faulting address whle handler did not yet fetched the pte.

I think FreeBSD would not provide si_pkey.

[kib]

Build fixes: tb passes.

[kib]

Any further comments on the change ?

[alc]

sys/amd64/amd64/trap.c

341 ↗

(On Diff #62097)

"Emulator can handle this trap?"

347 ↗

(On Diff #62097)

This assignment could be moved down below the two "if" statements, just before the "break".

682 ↗

(On Diff #62097)

"If this fault was fatal, typically from kernel mode"

684 ↗

(On Diff #62097)

"If this fault was handled by updating either the user or kernel page table, execution ..."

686 ↗

(On Diff #62097)

"If this fault was from user mode and it was not handled, a synchronous signal

sys/i386/i386/trap.c

733–735 ↗

(On Diff #62097)

The amd64 comment could be used here with the addition of -2 to the list of return values.

sys/vm/vm_fault.c

1092 ↗

(On Diff #62097)

Can you double-check this case? Does jilles@ comment still apply here? I ask because KERN_INVALID_ADDRESS is going to generate a SIGSEGV, not a SIGBUS.

sys/vm/vm_param.h

116 ↗

(On Diff #62097)

I would suggest "KERN_OUT_OF_BOUNDS". I have never seen this phrase used except with the plural "bounds".

[kib]

sys/vm/vm_fault.c

1092 ↗

(On Diff #62097)

I believe that this line change was done before introduction of KERN_OUT_OF_BOUNDS, then the signal generation code was rewritten to accommodate new errors but this line was left behind.

[kib]

Latest round of fixes.

[kib]

Alan, any more comments ? I want to close the review.

[alc]

In D21566#475488, @kib wrote:

Alan, any more comments ? I want to close the review.

I should be able to look at this change again today.

[alc]

sys/mips/mips/trap.c

672 ↗

(On Diff #62431)

Another redundant trunc_page()

709 ↗

(On Diff #62431)

Another redundant trunc_page()

sys/powerpc/powerpc/trap.c

463 ↗

(On Diff #62431)

Why pass &sig and &ucode in this case? They don't appear to be used after the call.

756–757 ↗

(On Diff #62431)

I don't see why we need to do this.

sys/vm/vm_extern.h

90–91 ↗

(On Diff #62431)

If we are trying to maintain the alphabetical ordering of the declarations in this contiguous block, then this one move down several places.

sys/vm/vm_fault.c

528 ↗

(On Diff #62431)

Feel free to ignore or defer action on the following comment: To me, the wording of this description suggests that I should set this sysctl to the signal number that I want delivered.

[kib]

sys/vm/vm_fault.c

528 ↗

(On Diff #62431)

s/select/control/

[kib]

More alc' notes.

[alc]

sys/amd64/amd64/trap.c

341 ↗

(On Diff #62619)

Get rid of the space before the '?'.

sys/arm/arm/trap-v4.c

103 ↗

(On Diff #62619)

This is now redundant, and can be deleted.

sys/powerpc/powerpc/trap.c

90–91 ↗

(On Diff #62619)

Consider changing "user" into a "bool".

763 ↗

(On Diff #62619)

Another redundant trunc_page()

[kib]

More notes by alc.