Initial port of DTrace for Morello #2309
Conversation
No functional change intended. MFC after: 1 week (cherry picked from commit 1905ce3a6bef5652ad36ced7df9da6d2acb96b69)
MFC after: 1 week Sponsored by: Innovate UK (cherry picked from commit 6e6a67e98652e1477973d031b27ce085d7f200c2)
This type is used only to store PC values corresponding to a thread stack trace, so a pointer type is not quite right. Switch to vm_offset_t, as in struct stack, to simplify a port of DTrace to CHERI. No functional change intended. MFC after: 1 week Sponsored by: Innovate UK (cherry picked from commit 8ce6b4f2c48eca758fac90b58924f9b2e38fbc49)
No functional change intended. MFC after: 1 week Sponsored by: Innovate UK (cherry picked from commit 61c4ac2df7b2b866c8ee5e944aedc96aa79bd315)
Remove some unused variables and reduce the scope of some others. No functional change intended. MFC after: 1 week Sponsored by: Innovate UK (cherry picked from commit 8384a19adc88ef31794f5aed1d8c5621b7dff8c9)
This eases porting of DTrace to CHERI, where uintptr_t and size_t aren't interchangeable. No functional change intended. Reviewed by: Domagoj Stolfa <[email protected]> MFC after: 2 weeks Sponsored by: Innovate UK Differential Revision: https://reviews.freebsd.org/D48625
Fixes: 6e6a67e98652 ("dtrace tests: Avoid hard-coding paths to required programs")
Sponsored by: Innovate UK
Make it easier to support data models other than ILP32 and LP64 by avoiding constructs which assume that it must be one or the other. No functional change intended. MFC after: 2 weeks Sponsored by: Innovate UK (cherry picked from commit 096a5c6cd28c417456d5ce3598be15e6b656af5c)
No functional change intended. MFC after: 2 weeks Sponsored by: Innovate UK (cherry picked from commit 51688136b161089b317edc34c3a3e9b40d54a18a)
There was no reason I can find for defining them this way. No functional change intended. Sponsored by: Innovate UK (cherry picked from commit e3f6ef5ade6a1865165bae55bbabd7cd8ec5da76)
On arm64, the FBT provider treats tail calls as return probes. Ignoring the question of whether this is really correct, the implementation is wrong: instr is a pointer to uint32_t, so the removed multiplication by the instruction size is wrong. As a result, FBT would create return probes for intra-function branches. MFC after: 2 weeks Sponsored by: Innovate UK (cherry picked from commit 4da070ce6c015a994ec4ecf3d31ee94810ea19f1)
When linker_file_function_listall() enumerates function symbols on purecap platforms, ifunc symbol values are sealed, whereas regular function symbols are not. This breaks dtrace's fbt and kinst providers, which use the returned capability to disassemble each function in the kernel. Moreover, the size of the symbol is wrong: it's the size of the resolver function rather than that of the implementation function. This latter problem is a bug upstream too. Fix both problems by looking up the full ELF symbol for the resolved function. This change is not quite correct, as it assumes that ifunc resolvers always return a symbol within the same linker file. However, this is true in practice today.
| return (ENOENT); | ||
| es = (const Elf_Sym *)sym1; | ||
| val = (caddr_t)ef->address + es->st_value; | ||
| } | ||
| #ifdef __CHERI_PURE_CAPABILITY__ | ||
| val = make_capability(es, val); | ||
| #endif |
There was a problem hiding this comment.
Is the problem not just that we should be doing make_capability prior to the IFUNC check, rather than after?
There was a problem hiding this comment.
I don't think so, or I don't understand your suggestion.
Consumers of this interface generally don't want to know about ifuncs. What we do for ifuncs is provide a function symbol that refers to the implementation selected by the resolver. Without this change, we were kind of doing that, but the symbol value would be a sentry capability and the size would be that of the ifunc symbol (which in practice is the size of the resolver function). My goal with this change is to eliminate those differences. I don't see how calling make_capability() first would help with that.
There was a problem hiding this comment.
The return value of the resolver is what we want to be giving, because it itself comes from some other non-IFUNC capability that the linker's properly constructed. The problem is that make_capability, for an IFUNC, thinks it's been given a capability that's ef->address plus some constant, but it's not, it's got the actual function pointer. On the flip side, we're calling the resolver via a capability that's ef->address plus some constant, so hasn't had its permissions restricted. IFUNCs are in essence "get a capability to the function as normal, but then call it to get the real capability", but we're failing to implement that here.
There was a problem hiding this comment.
Ok, I think I see, but addressing that won't fix my problem: the resolver returns a function pointer, which isn't what callers of link_elf_each_function_nameval() expect. I'd have to, at least, unseal the symbol value.
There was a problem hiding this comment.
But that's what it returns for normal STT_FUNC today, too? Why are IFUNCs special in that regard?
There was a problem hiding this comment.
The main consumer of this interface is linker_file_lookup_set(), which definitely requires capabilities, though I expect that linker sets generally don't contain function symbols. I don't think we can return a ptraddr_t here.
I don't quite follow your comment about calling the ifunc resolver with the "right" capability - from what I can, relocate_file1() passes ef->address as relocbase to elf_reloc_internal(), where it's used to derive the resolver capability, just as we do here. You said earlier that we should perhaps call make_capability() before invoking the resolver, which is makes sense to me, but it doesn't solve my problem.
There was a problem hiding this comment.
As in the capability we use for the call should have code capability permissions only (and the right bounds, but I believe we don't bound kernel code capabilities even on Morello?)
There was a problem hiding this comment.
To be clear, you are asking me to make that change in this PR?
There was a problem hiding this comment.
No, other than that if we're thinking carefully about and rewriting this code to be less weird it might be something that gets folded into the rewrite rather than preserving the oddity.
There was a problem hiding this comment.
(I need to go and understand exactly what these interfaces are being used for to be able to assess what the right thing to do about your issue is)
_SDT_ASM_WORD is a bit of a misnomer; it's used to store pointers as immediate values. In particular, we store a pointer to a struct sdt_probe in each SDT tracepoint structure, which is built using inline assembler directives.
Taking the address of the probe symbol will cause the compiler to emit a setbounds instruction, but that doesn't work here. Work around that behaviour with __unbounded_addressof, since we just want the symbol name.
This is needed in order to insert a breakpoint when enabling dtrace probes.
MFC after: 1 week Sponsored by: Innovate UK (cherry picked from commit 1e734f15c7099408f45d3b1ea433868d0468916f)
Otherwise capabilities are truncated. Note that dtrace_probe()'s arguments are uintptr_t, so we were already truncating 64-bit syscall arguments on 32-bit platforms.
- Make dtrace_fuword* operate on a capability, even in hybrid kernels. - Add dtrace_fucap(), used to implement ustack(). - Make assembly subroutines, e.g., dtrace_casptr(), work in purecap kernels.
The P128 model is used for purecap targets instead of LP64. It's identical to LP64 except, of course, that pointers are 128 bits wide.
This type needs to be large enough to hold a 64-bit integer or a pointer. Fields of this type might need to be returned to userspace, so we must be sure not to truncate them or clear capability tags.
No functional change intended. Sponsored by: Innovate UK
They do not build on arm64, as we don't implement USDT there.
These arise in purecap binaries, e.g.,:
<1><1954>: Abbrev Number: 8 (DW_TAG_base_type)
<1955> DW_AT_name : (indirect string) unsigned __intcap
<1959> DW_AT_encoding : 161 (UNKNOWN)
<195a> DW_AT_byte_size : 16
and
<1><34eb>: Abbrev Number: 8 (DW_TAG_base_type)
<34ec> DW_AT_name : (indirect string) __intcap
<34f0> DW_AT_encoding : 160 (UNKNOWN)
<34f1> DW_AT_byte_size : 16
We don't seem to have symbolic names for these.
Flag capability violations with CPU_DTRACE_CHERI so that userspace has some clue as to what's causing errors. It would be nice to have more specific errors, but we are limited by a limited range of values for CPU_DTRACE_* flags.
- Make dtrace_getreg() return a uintcap_t on CHERI platforms, otherwise register values get truncated, e.g., regs[R_X0]. - Make stack unwinding work better on arm64. unwind_frame() can't handle exception frames, so stack traces were previously getting truncated. This is a stopgap however, as it doesn't handle unwinding through, e.g., kernel data aborts.
These will be emitted by the libdtrace DIF compiler when loading or storing capabilities.
Such values are generally given 8 byte-alignment instead of 16 byte-alignment since this requires fewer modifications elsewhere and we do not preserve capability tags anyway.
- Dynamic variable chunks need to be rounded up to the capability size in order to preserve alignment. - Dynamic variable keys may be capabilities.
memcpy() might be instrumented by FBT or kinst, so we can end up reentering dtrace_probe(), which is forbidden. We've already checked that the pointer is suitably aligned, so just dereference it directly.
The kernel linker gives us read-only capabilities for function symbol values, but we need to be able to overwrite tracepoints with breakpoints. For each tracepoint, derive a patchpoint capability from kernel_root_cap.
Much of dtrace's functionality is now available, but with some caveats: - dtrace(1)'s -c option doesn't work yet, - capability tags are not preserved, so while they can be pretty-printed, they are always displayed as invalid, even when this is not the case, - there are no intrinsics that operate on CHERI capabilities, e.g., it's not possible to check whether a capability tag is set, - global variables accesses, e.g., `ticks, don't work, - dtrace with hybrid kernels is untested, - the kinst provider is untested and should not be used. Aside from these caveats, most of dtrace's functionality should just work.
The DTrace port is experimental and not suitable for use in production environments. Add some friction to make sure that users understand this.
With this pull request, much of DTrace's functionality is available in arm64 purecap kernels. There are some major caveats at the moment:
To use DTrace in a purecap kernel, set the
debug.dtrace_enabledsysctl to 1 and load dtraceall.ko.Almost all of the changes are in dtrace code, so there is relatively little risk of regressions. There is a pair of changes to link_elf.c, required for the FBT provider to work, which deserve scrutiny. Aside from that, I committed unobtrusive refactorings upstream when possible and backported them.
One of the larger changes introduces a
dtrace_difval_ttype to replace uint64_t in many places. I suspect that this should eventually be upstreamed.I ripped out COMPAT_FREEBSD64 support for dtrace ioctls, as discussed last week. A large portion of the diff is just from that and from regenerating systrace definitions.