Page MenuHomeFreeBSD

dtrace/arm64: de-pessimize dtrace_copy
ClosedPublic

Authored by freebsd_dev.thsi.be on Thu, Sep 3, 2:00 PM.
Tags
None
Referenced Files
F171840475: D59341.diff
Sun, Sep 13, 11:34 PM
F171769482: D59341.diff
Sun, Sep 13, 9:50 AM
F171757181: D59341.id185700.diff
Sun, Sep 13, 7:43 AM
Unknown Object (File)
Sat, Sep 12, 4:08 PM
Unknown Object (File)
Sat, Sep 12, 2:08 PM
Unknown Object (File)
Sat, Sep 12, 11:36 AM
Unknown Object (File)
Sat, Sep 12, 8:38 AM
Unknown Object (File)
Sat, Sep 12, 4:58 AM
Subscribers

Details

Summary

When DTrace catches a data abort exception it resumes execution on the
next instruction. If a probe executes copyinto from unmapped memory,
then dtrace_copy keeps faulting on successive bytes until the loop
counter is exhausted. Detect the situation by witnessing the absence
of zero-extension from an aborted unprivileged load.

Diff Detail

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

Event Timeline

I failed at my attempt to produce a reliable testcase, so I ended up hacking a debug counter in dtrace_trap. Running dwatch -X read on a idle system with simple ssh activity went from this counter raising by the several thousands to a couple at a time.

sys/cddl/dev/dtrace/aarch64/dtrace_asm.S
128

Does this routine need to be updated too?

I think so. In Illumos, the condition is well checked. Would you like this review to include the a fix for that? If yes, then I'll update D59280 to put a fix for arm32 too.
Also I believe that riscv is affected, but I don't have hw or the will to delve into riscv emulation and asm manuals right now.

(I'd like to make sure the copyout part works. but I need a bit of time to go around it)

I think so. In Illumos, the condition is well checked. Would you like this review to include the a fix for that? If yes, then I'll update D59280 to put a fix for arm32 too.

It'd be nice to fix both, yeah. Sorry, I already committed the 32-bit arm patch to main, I forgot that it has the same problem.

Also I believe that riscv is affected, but I don't have hw or the will to delve into riscv emulation and asm manuals right now.

(I'd like to make sure the copyout part works. but I need a bit of time to go around it)

Would you be willing to file an issue for that in bugzilla? We can try to get the riscv folks to take a look.

optimize dtrace_copystr
adding a child revision to take care of copyout/copyoutstr.

markj added inline comments.
sys/cddl/dev/dtrace/aarch64/dtrace_asm.S
126

There is still technically a flags parameter, the comment might briefly explain why it is ignored in this implementation.

This revision is now accepted and ready to land.Mon, Sep 7, 2:14 PM

Yes good catch, the flags is technically still there. I agree with you, and I saw it too that the change on the prototype in the comment should have been left for D59449 (where it is formally dropped). I thought let's keep it that way to avoid too much noise, and also because there was already an inconsistency between copy/copystr comment in this regard. In the end, they both need to check that a fault didn't happen. I can update this DR if you prefer having it 100% correct. I'd rather not bother now, but make sure that the Aarch32 copyout case if fixed as well in D59449 (where the IN/OUT are properly split apart, currently for aarch64 only), to explain why we don't need to pass the flags (for aarch32/aarch64 in the IN-direction and for aarch32 in the OUT direction), and align all the prototype definitions at once.

Yes good catch, the flags is technically still there. I agree with you, and I saw it too that the change on the prototype in the comment should have been left for D59449 (where it is formally dropped). I thought let's keep it that way to avoid too much noise, and also because there was already an inconsistency between copy/copystr comment in this regard. In the end, they both need to check that a fault didn't happen. I can update this DR if you prefer having it 100% correct. I'd rather not bother now, but make sure that the Aarch32 copyout case if fixed as well in D59449 (where the IN/OUT are properly split apart, currently for aarch64 only), to explain why we don't need to pass the flags (for aarch32/aarch64 in the IN-direction and for aarch32 in the OUT direction), and align all the prototype definitions at once.

That's fine with me, thank you.

This revision was automatically updated to reflect the committed changes.