Page MenuHomeFreeBSD

Add PT_GETREGSET
Needs ReviewPublic

Authored by andrew on Apr 5 2019, 12:36 PM.

Details

Reviewers
jhb
kib
Summary

This adds the PT_GETREGSET ptrace type. This reads all the registers from
a specified core dump note type. Because the data field is an int the
arguments are backwards when compared to the Linux PTRACE_GETREGSET
call.

The ptrace addr points to a struct iovec pointing at memory to hold the
registers along with its length. On success the length in the iovec is
updated to tell userspace the actual length the kernel wrote.

Currently there's no way to ask for the length userspace needs to
allocate from the kernel. One possible solution is to use a null vec,
where the base is NULL and length is 0, and have the kernel set the
length to be correct.

Add support to read NT_X86_XSTATE. This is sufficienrly complex as its
length is only known at runtime. To handle this case use a SYSINIT to
set the size on boot.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 25763
Build 24339: arc lint + arc unit

Event Timeline

andrew created this revision.Apr 5 2019, 12:36 PM
jhb added a comment.Apr 5 2019, 3:43 PM

Hmm, do you have plans for a PT_SETREGSET as well? To replace the ARM VFP, and PPC VMX, etc. sets we need set, not just get. Also, as I said previously on IRC I would like to have a PT_GETREGSET_INFO where the iovec in addr points to a structure. The first field would always be a uint64_t size member, but the rest of the fields could vary by note type. For XSTATE the structure would also hold the xcr0 mask to replace PT_GETXSTATE_INFO, so something like:

struct regset_info {
    uint64_t len;
};

and then in x86/include/ptrace.h:

struct xstate_regset_info {
     uint64_t len;
     uint64_t xsave_mask;
sys/kern/sys_process.c
773

style(9) is to use ()'s with sizeof

839

Perhaps use 'goto out' here instead?

890

And move the free of kern_vec.iov_base to here under an 'out' label. This fixes a leak when the first copyout fails. It will also make adding other functions that use an iovec simpler by consolidating the error handling.

jhb added a reviewer: kib.Apr 5 2019, 3:44 PM
kib added a comment.Apr 5 2019, 4:42 PM

If you return whole XSTATE from ptrace(PT_GETREGSET, NT_X86_XSTATE), what is the point of the new API ? It would make much more sense to me if there was an API to get components like YMM/ZMM/PKRU instead of returning whole blob and then requiring the caller to parse it combining XCR0 bitmask with either prior knowledge of the component sizes, or querying the offsets on live machine.

sys/amd64/amd64/elf_machdep.c
172 ↗(On Diff #55838)

I believe you can do the assignment in fpu,c where cpu_max_ext_state_size is calculated, there is no need in SYSINIT.

179 ↗(On Diff #55838)

This looks strange, I see that it matches the note behavior but still. If !use_xsave, you would return x87+XMM state, which is strict subset of XSAVE state.

sys/sys/ptrace.h
40

Why this include is needed in public header ? I believe it is fair to require users to #include <sys/uio.h> if they need PT_GETREGSET.

jhb added a comment.Apr 5 2019, 4:52 PM
In D19831#425341, @kib wrote:

If you return whole XSTATE from ptrace(PT_GETREGSET, NT_X86_XSTATE), what is the point of the new API ? It would make much more sense to me if there was an API to get components like YMM/ZMM/PKRU instead of returning whole blob and then requiring the caller to parse it combining XCR0 bitmask with either prior knowledge of the component sizes, or querying the offsets on live machine.

Both GDB and lldb are happy to just get the blob. However, we could use this to replace some of the "one-off" register sets we have now like the VFP registers for ARM, the two separate vector register sets for PPC. I plan to add a new register set on x86 for fs_base/gs_base mostly because I need a new core dump note to permit TLS to work in gdb on core dumps. ARM needs the TLS register available in cores and via ptrace for which a new register set would make sense (and/or extending the existing 'struct reg' which PT_GETREGSET makes easier). One thing that you should be able to do, btw is use PT_GETREGSET to fetch the normal and floating point register sets. Floating point is just NT_FPREGSET. struct reg is a bit harder. My guess is you'd let NT_PRSTATUS return struct reg (but not the full core dump note), but we should see what Linux does perhaps for this.

andrew added a comment.May 7 2019, 5:28 PM
In D19831#425330, @jhb wrote:

Hmm, do you have plans for a PT_SETREGSET as well?

Yes, when I have time to investigate how Linux handles a set with an incorrect length.

To replace the ARM VFP, and PPC VMX, etc. sets we need set, not just get. Also, as I said previously on IRC I would like to have a PT_GETREGSET_INFO where the iovec in addr points to a structure. The first field would always be a uint64_t size member, but the rest of the fields could vary by note type.

What would the length hold if the buffer was too small for the data but large enough for the length? I have an update that will set the iovec length to the expected buffer length when the base is NULL so userspace can query it.

In D19831#425359, @jhb wrote:

One thing that you should be able to do, btw is use PT_GETREGSET to fetch the normal and floating point register sets. Floating point is just NT_FPREGSET. struct reg is a bit harder. My guess is you'd let NT_PRSTATUS return struct reg (but not the full core dump note), but we should see what Linux does perhaps for this.

It looks like Linux does this. It then seems to have a special case for NT_PRSTATUS when building a core dump.

sys/amd64/amd64/elf_machdep.c
172 ↗(On Diff #55838)

I'm planning on splitting this out into a new review. I kept it in for some context on how the KPI should be used.

sys/kern/sys_process.c
773

This is mostly to be consistent with the style in the switch. I'm planning on adding COMPAT32 support, but will need to add an iovec32 first. This will need this non-style sizeof.

andrew updated this revision to Diff 57213.May 9 2019, 12:41 PM
  • Remove xsave. It will move to a new review.
  • Add PT_SETREGSET
  • Use goto out for error handling
  • Remove sys/_iovec.h from sys/ptrace.h

I also return the size when the base is NULL in PT_GETREGSET.

kib added a comment.May 10 2019, 11:08 AM

This looks fine to me, modulo compat32 and man page updates.

And compat32 rises interesting question, is ELF_REGSET() interface should be per-ABI ? E.g., do we want to have it specialized for the case when Linux process asks for a registers blob, or linuxolator should decode the values of the argument and re-arrange the FreeBSD native regsets into linuxish structures ?

andrew updated this revision to Diff 60611.Aug 9 2019, 12:02 PM

Add COMPAT32 and update the man page

andrew updated this revision to Diff 60612.Aug 9 2019, 12:16 PM

Correct the man page, addr points at a struct iovec.