Page MenuHomeFreeBSD

amd64: Relax the assertion added in commit 4a59cbc12
ClosedPublic

Authored by markj on Jun 1 2021, 2:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 8:46 PM
Unknown Object (File)
Tue, Nov 12, 2:46 AM
Unknown Object (File)
Tue, Nov 12, 2:12 AM
Unknown Object (File)
Oct 3 2024, 6:30 AM
Unknown Object (File)
Oct 2 2024, 9:29 PM
Unknown Object (File)
Oct 2 2024, 7:36 PM
Unknown Object (File)
Oct 1 2024, 8:01 AM
Unknown Object (File)
Oct 1 2024, 6:42 AM
Subscribers

Details

Summary

We only need to ensure that interrupts are disabled when handling a
fault from iret. Otherwise it's possible to trigger the assertion
legitimately, e.g., by copying in from an invalid address.

Fixes: 4a59cbc12
Reported by: pho

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 39651
Build 36540: arc lint + arc unit

Event Timeline

markj requested review of this revision.Jun 1 2021, 2:19 PM

Ok, but I think that segment load faults should not have interrupts enabled. Since we do not context-switch them, having indefinite state for any of the segments is probably not too healthy.

This revision is now accepted and ready to land.Jun 1 2021, 2:43 PM
In D30594#687126, @kib wrote:

Ok, but I think that segment load faults should not have interrupts enabled. Since we do not context-switch them, having indefinite state for any of the segments is probably not too healthy.

Yeah, that's why I wrote it this way originally. I missed that we handle pcb_onfault != NULL in the same block.

I can add the same assertion to all of the segment load fault cases. Do you see a better approach?

In D30594#687126, @kib wrote:

Ok, but I think that segment load faults should not have interrupts enabled. Since we do not context-switch them, having indefinite state for any of the segments is probably not too healthy.

Yeah, that's why I wrote it this way originally. I missed that we handle pcb_onfault != NULL in the same block.

I can add the same assertion to all of the segment load fault cases. Do you see a better approach?

No, I think explicitly mark all cases with asserts is the only way. Or it could be a table of addresses and new %rips for segment faults, with one loop iterating over, and one assert in case of the match. But I think it is not needed (yet).

Add assertions for segment register update faults. Convert to using a table.

This revision now requires review to proceed.Jun 1 2021, 5:20 PM

Actually I am having trouble convincing myself that

In D30594#687163, @kib wrote:
In D30594#687126, @kib wrote:

Ok, but I think that segment load faults should not have interrupts enabled. Since we do not context-switch them, having indefinite state for any of the segments is probably not too healthy.

Yeah, that's why I wrote it this way originally. I missed that we handle pcb_onfault != NULL in the same block.

I can add the same assertion to all of the segment load fault cases. Do you see a better approach?

No, I think explicitly mark all cases with asserts is the only way. Or it could be a table of addresses and new %rips for segment faults, with one loop iterating over, and one assert in case of the match. But I think it is not needed (yet).

I converted to a table, it seemed quite ugly to duplicate the assertion for each case. I tested with a program that uses setcontext() to set bogus segment register values.

This revision is now accepted and ready to land.Jun 1 2021, 9:36 PM