Re: [PATCH v3 0/7] x86/ptrace: regset access to the GDT and LDT

[Ingo Molnar]

* H. Peter Anvin, Intel <h.peter.anvin@xxxxxxxxx> wrote:

> From: "H. Peter Anvin" <hpa@xxxxxxxxxxxxxxx>
> 
> Give a debugger access to the visible part of the GDT and LDT.  This
> allows a debugger to find out what a particular segment descriptor
> corresponds to; e.g. if %cs is 16, 32, or 64 bits.
> 
> v3:
> 	Requalify LDT segments for selectors that have actually changed.
> 
> v2:
> 	Add missing change to elf.h for the very last patch.
> 
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Cc: Chang S. Bae <chang.seok.bae@xxxxxxxxx>
> Cc: Markus T. Metzger <markus.t.metzger@xxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> 
>  arch/x86/Kconfig               |   4 +
>  arch/x86/include/asm/desc.h    |  24 +++-
>  arch/x86/include/asm/ldt.h     |  16 +++
>  arch/x86/include/asm/segment.h |  10 ++
>  arch/x86/kernel/Makefile       |   3 +-
>  arch/x86/kernel/ldt.c          | 283 ++++++++++++++++++++++++++++++++---------
>  arch/x86/kernel/ptrace.c       | 103 ++++++++++++++-
>  arch/x86/kernel/tls.c          | 102 +++++----------
>  arch/x86/kernel/tls.h          |   8 +-
>  include/uapi/linux/elf.h       |   2 +
>  10 files changed, 413 insertions(+), 142 deletions(-)

Ok, this looks mostly good to me at a quick glance, but could you please do one 
more iteration and more explicitly describe where you change/extend existing ABIs?

I.e. instead of a terse and somewhat vague summary:

> x86/tls,ptrace: provide regset access to the GDT
>
> Give a debugger access to the visible part of the GDT and LDT.  This
> allows a debugger to find out what a particular segment descriptor
> corresponds to; e.g. if %cs is 16, 32, or 64 bits.

Please make it really explicit how the ABI is affected, both in the title and in 
the description, and also _explain_ how this helps us over what we had before, 
plus also list limitations of the new ABI.

I.e. something like:

   x86/tls, ptrace: Extend the ptrace ABI with the new REGSET_GDT method

   Add the new REGSET_GDT ptrace variant to PTRACE_{GET|SET}REGSET,
   to give read and write access to the GDT:

   - Previously only certain parts of the GDT were visible, and only via random
     ABIs and instructions - there was no architectured access to all of it.

   - The SETREGSET variant is only allowed to change the TLS area of the GDT.

(or so.)

This is important not just for documentation and library support purposes, but 
also to be able to review it properly, to match 'intent' to 'implementation'.

(It might also help later on in finding bugs/quirks, if any.)

Please do this for all patches in the series that change the ABI.

Thanks,

	Ingo