lkml.org 
[lkml]   [2018]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 0/7] x86/ptrace: regset access to the GDT and LDT

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

> From: "H. Peter Anvin" <hpa@linux.intel.com>
>
> 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@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Chang S. Bae <chang.seok.bae@intel.com>
> Cc: Markus T. Metzger <markus.t.metzger@intel.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
>
> 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

\
 
 \ /
  Last update: 2018-06-22 03:59    [W:1.668 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site