Page MenuHomeFreeBSD

Improve MD page fault handlers.
ClosedPublic

Authored by kib on Sep 8 2019, 4:27 PM.

Details

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

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
kib updated this revision to Diff 61952.Sep 11 2019, 8:15 PM
kib edited the summary of this revision. (Show Details)
kib marked 2 inline comments as done.

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

kib retitled this revision from Change the delivered signal for accesses to mapped area after the backing object was truncated. to Improve MD page fault handlers..Sep 11 2019, 8:18 PM
kib edited the summary of this revision. (Show Details)
alc added inline comments.Sep 11 2019, 10:46 PM
sys/amd64/amd64/trap.c
114 ↗(On Diff #61808)

The return value is never used.

alc added inline comments.Sep 11 2019, 11:11 PM
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 added inline comments.Sep 11 2019, 11:26 PM
sys/amd64/amd64/trap.c
792 ↗(On Diff #61952)

This assignment no longer serves any purpose.

kib updated this revision to Diff 61966.Sep 12 2019, 6:28 AM
kib marked 3 inline comments as done.

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

alc added inline comments.Sep 12 2019, 3:26 PM
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 marked 5 inline comments as done.Sep 12 2019, 3:57 PM
kib added inline comments.
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 updated this revision to Diff 61984.Sep 12 2019, 3:58 PM
kib marked an inline comment as done.

More changes requested by alc.

alc added inline comments.Sep 12 2019, 5:43 PM
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 updated this revision to Diff 61994.Sep 12 2019, 6:21 PM
kib marked 4 inline comments as done.

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

kib updated this revision to Diff 62028.Sep 13 2019, 9:47 AM
  • 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 added inline comments.Sep 13 2019, 3:00 PM
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 added a comment.Sep 13 2019, 5:48 PM
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 marked 2 inline comments as done.Sep 13 2019, 7:24 PM
kib updated this revision to Diff 62066.Sep 13 2019, 8:21 PM

Rebase after riscv commit.

jilles added inline comments.Sep 14 2019, 11:10 AM
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 added inline comments.Sep 14 2019, 1:45 PM
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 updated this revision to Diff 62097.Sep 14 2019, 4:49 PM

Build fixes: tb passes.

kib added a comment.Sep 21 2019, 3:30 PM

Any further comments on the change ?

alc added inline comments.Sep 21 2019, 11:00 PM
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 marked 9 inline comments as done.Sep 22 2019, 7:30 PM
kib added inline comments.
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 updated this revision to Diff 62431.Sep 22 2019, 7:30 PM
kib marked an inline comment as done.

Latest round of fixes.

kib added a comment.Sep 25 2019, 12:35 PM

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

alc added a comment.Sep 26 2019, 5:14 PM
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 added inline comments.Sep 26 2019, 7:54 PM
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 marked 6 inline comments as done.Sep 26 2019, 8:23 PM
kib added inline comments.
sys/vm/vm_fault.c
528 ↗(On Diff #62431)

s/select/control/

kib updated this revision to Diff 62619.Sep 26 2019, 8:27 PM
kib marked an inline comment as done.

More alc' notes.

alc accepted this revision.Sep 27 2019, 6:26 AM
alc added inline comments.
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()

This revision is now accepted and ready to land.Sep 27 2019, 6:26 AM
kib updated this revision to Diff 62630.Sep 27 2019, 7:15 AM
kib marked 4 inline comments as done.

More notes by alc.

This revision now requires review to proceed.Sep 27 2019, 7:15 AM
alc accepted this revision.Sep 27 2019, 2:08 PM
This revision is now accepted and ready to land.Sep 27 2019, 2:08 PM
This revision was automatically updated to reflect the committed changes.