arm64: Don't enable interrupts when in a spinlock
ClosedPublic
Actions

Authored by andrew on Sep 27 2024, 3:50 PM.

Details

Reviewers

kib
markj
manu
mmel

Group Reviewers

arm64

Commits

rGa84653c5db25: arm64: Don't enable interrupts when in a spinlock

Summary

When we receive an exception while in a spinlock we shouldn't enable
interrupts. When entering a spinlock we disable interrupts so enabling
them here could cause surprising results.

The three cases that could cause this are:

A break-before-make sequence
Accessing possibly unmapped code with a fault handler
Buggy code

1 and 2 are supported later in the data abort handler, and 3 should be
fixed when found.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 60171
Build 57055: arc lint + arc unit

Event Timeline

andrew created this revision.Sep 27 2024, 3:50 PM

Herald added a reviewer: manu. · View Herald TranscriptSep 27 2024, 3:50 PM

Herald added subscribers: emaste, imp. · View Herald Transcript

andrew requested review of this revision.Sep 27 2024, 3:50 PM

Harbormaster completed remote builds in B59627: Diff 143811.Sep 27 2024, 3:50 PM

andrew added a parent revision: D46815: arm64: Check TDP_NOFAULTING in a data abort.Sep 27 2024, 3:50 PM

andrew added a child revision: D46817: arm64: Implement efi_rt_arch_call.

markj accepted this revision.Sep 30 2024, 9:00 AM

markj added inline comments.

sys/arm64/arm64/trap.c
345	IMO this shouldn't be conditional on INVARIANTS...

This revision is now accepted and ready to land.Sep 30 2024, 9:00 AM

andrew added inline comments.Sep 30 2024, 5:52 PM

sys/arm64/arm64/trap.c
345	I think it was a `KASSERT` earlier, so was changes to be this to add the registers.

markj added inline comments.Oct 1 2024, 7:32 AM

sys/arm64/arm64/trap.c
345	Sure, I'm just saying that the whole block should be unconditional.

imho, arm32 has slightly more correct code/condition, see|: https://cgit.freebsd.org/src/tree/sys/arm/arm/trap-v6.c#n403

In D46816#1069327, @mmel wrote:

imho, arm32 has slightly more correct code/condition, see|: https://cgit.freebsd.org/src/tree/sys/arm/arm/trap-v6.c#n403

spinlock_count > 0 implies that interrupts were disabled, so that code is a bit redundant - it could just check whether PSR_I is clear. But yes, I agree it's more correct.

Also check I and F flags before enabling interrupts

This revision now requires review to proceed.Oct 23 2024, 1:43 PM

Harbormaster completed remote builds in B60171: Diff 145387.Oct 23 2024, 1:43 PM

mmel accepted this revision.Oct 23 2024, 6:58 PM

This revision is now accepted and ready to land.Oct 23 2024, 6:58 PM

kib accepted this revision.Oct 23 2024, 10:51 PM

Closed by commit rGa84653c5db25: arm64: Don't enable interrupts when in a spinlock (authored by andrew). · Explain WhyOct 24 2024, 10:37 AM

This revision was automatically updated to reflect the committed changes.

andrew added a commit: rGa84653c5db25: arm64: Don't enable interrupts when in a spinlock.