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

[hpa]

On June 21, 2018 6:58:51 PM PDT, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>* 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

ACK.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.