dtrace/arm: Really trap translation faults
ClosedPublic
Actions

Authored by freebsd_dev.thsi.be on Sun, Aug 30, 10:51 PM.

Details

Reviewers

andrew
fuz
gnn
markj

Commits

rG00678f5e5c08: dtrace/arm: Really trap translation faults

Summary

Fix the constant case label to properly handle translation faults
caused by DTrace probes. Alignment errors are not expected to be
generated, so stop handling them.

While at it, correct an amd64-specific comment and add a comment
regarding the missing faulting address which could be addressed by a
later improvement.

PR: 298064
MFC after: 1 month

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76340
Build 73223: arc lint + arc unit

Event Timeline

freebsd_dev.thsi.be created this revision.Sun, Aug 30, 10:51 PM

Herald added a subscriber: imp. · View Herald TranscriptSun, Aug 30, 10:51 PM

freebsd_dev.thsi.be requested review of this revision.Sun, Aug 30, 10:51 PM

Harbormaster completed remote builds in B76340: Diff 185461.Sun, Aug 30, 10:51 PM

freebsd_dev.thsi.be added a parent revision: D59280: dtrace/arm: Fix and de-pessimize dtrace_copy.Sun, Aug 30, 10:51 PM

freebsd_dev.thsi.be updated this revision to Diff 185503.Mon, Aug 31, 5:46 PM

Harbormaster completed remote builds in B76355: Diff 185503.Mon, Aug 31, 5:46 PM

Added FAULT_TRAN_L1.

markj added inline comments.Wed, Sep 2, 1:17 PM

sys/cddl/dev/dtrace/arm/dtrace_subr.c
191	Shouldn't this handle FAULT_PERM_L2 and FAULT_PERM_L1 as well? Otherwise we won't intercept, e.g., dtrace_copyout to read-only memory.

added case for FAULT_PERM_L2 and FAULT_PERM_L1

Harbormaster completed remote builds in B76423: Diff 185699.Thu, Sep 3, 12:13 PM

Good point, I didn't think about this situation. I think that is what the DTrace toxic_range is supposed to prevent, but I'm not sure it is kept updated with such read-only mapping, so better safe than sorry. FWIW I think that copyout needs it own implementation because right now it is using copyin with inversed uaddr and kaddr argument and that's expected to not work. I'll have a closer look on how one can use copyout (aka destructive mode) and if it needs fixing.

In D59281#1361576, @freebsd_dev.thsi.be wrote:

Good point, I didn't think about this situation. I think that is what the DTrace toxic_range is supposed to prevent, but I'm not sure it is kept updated with such read-only mapping, so better safe than sorry. FWIW I think that copyout needs it own implementation because right now it is using copyin with inversed uaddr and kaddr argument and that's expected to not work. I'll have a closer look on how one can use copyout (aka destructive mode) and if it needs fixing.

Toxic ranges are for preventing accesses to device memory where reads may have side effects. See the comment above the definition of struct dtrace_toxrange. We rely on the MMU to raise a trap if copyout() tries to write to a read-only mapping.

This revision is now accepted and ready to land.Thu, Sep 3, 1:26 PM

Closed by commit rG00678f5e5c08: dtrace/arm: Really trap translation faults (authored by freebsd_dev.thsi.be, committed by markj). · Explain WhyThu, Sep 3, 3:05 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rG00678f5e5c08: dtrace/arm: Really trap translation faults.