Page MenuHomeFreeBSD

arm64: Check TDP_NOFAULTING in a data abort
ClosedPublic

Authored by andrew on Sep 27 2024, 3:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 4:28 PM
Unknown Object (File)
Tue, Oct 29, 5:28 PM
Unknown Object (File)
Thu, Oct 24, 11:23 AM
Unknown Object (File)
Thu, Oct 24, 5:53 AM
Unknown Object (File)
Oct 12 2024, 6:08 PM
Unknown Object (File)
Oct 9 2024, 2:42 AM
Unknown Object (File)
Oct 9 2024, 2:41 AM
Unknown Object (File)
Oct 9 2024, 2:41 AM
Subscribers

Details

Summary

As with other architectures when the TDP_NOFAULTING flag is set we
shouldn't panic when td_critnest is non-zero or if the witness check
fails.

The EFI runtime service functions will soon set this flag to handle
exceptions in the services.

Sponsored by: Arm Ltd

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60169
Build 57053: arc lint + arc unit

Event Timeline

This change is specifically for efirt fault handling, am I right?

If yes, I suggest to add a different pcb onfault pointer, to not weak the assumptions in case of normal userspace access. E.g. if we take an interrupt while in copyin/out, kernel fault must not result in silent retry.

Is the concern about pcb->pcb_onfault being set when interrupts are enabled so an interrupt handler could trash it calling into an EFI runtime service?

Is the concern about pcb->pcb_onfault being set when interrupts are enabled so an interrupt handler could trash it calling into an EFI runtime service?

No. Suppose a thread executes copyout. The interrupts are enabled, and cpu could receive one with pcb_onfault set to copyout fault handler. Then suppose that the interrupt handler faulted. The patched code would see intr_nesting_level != 0 and pcb_onfault != 0, erronously passing control to copyout fault handler. The result would be further state corruption, perhaps garbled x0/x1 for the handler, and _not_ handled fault.

If intr_nesting_level != 0 then the intr_nesting_level == 0 check will fail and the kernel will fall through to the existing panic calls.

If intr_nesting_level != 0 then the intr_nesting_level == 0 check will fail and the kernel will fall through to the existing panic calls.

Ok, same argument for code that took a spinlock but not incremented td_intr_nesting_level.

Let me explain why this patch triggered me: pcb_onfault, while formally MD, has the same semantic on all arches. Having arm64 being an outlier IMO is not worth it, and the cost of not doing that is easy by adding dedicated pcb_onefifault (name is only an example).

Switch to __predict_true((td->td_pflags & TDP_NOFAULTING) == 0) as on other archs. We can then call vm_fault_disable_pagefaults in efi_arch_enter.

(will update commit message later)

andrew retitled this revision from arm64: Call the on fault handler in more cases to arm64: Check TDP_NOFAULTING in a data abort.Oct 1 2024, 12:52 PM
andrew edited the summary of this revision. (Show Details)
sys/arm64/arm64/trap.c
343

It's a bit weird to have a __predict_true annotation in a condition that we expect to be false. Perhaps it'd make more sense to check the unlikely cases first.

This revision is now accepted and ready to land.Wed, Oct 23, 12:34 PM
This revision was automatically updated to reflect the committed changes.