| Message ID | 20180710222639.8241-26-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > Add PTRACE interface for CET MSRs. Please *always* describe new ABIs in the changelog, in a precise, well-documented way. > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c > index e2ee403865eb..ac2bc3a18427 100644 > --- a/arch/x86/kernel/ptrace.c > +++ b/arch/x86/kernel/ptrace.c > @@ -49,7 +49,9 @@ enum x86_regset { > REGSET_IOPERM64 = REGSET_XFP, > REGSET_XSTATE, > REGSET_TLS, > + REGSET_CET64 = REGSET_TLS, > REGSET_IOPERM32, > + REGSET_CET32, > }; Why does REGSET_CET64 alias on REGSET_TLS? > struct pt_regs_offset { > @@ -1276,6 +1278,13 @@ static struct user_regset x86_64_regsets[] __ro_after_init = { > .size = sizeof(long), .align = sizeof(long), > .active = ioperm_active, .get = ioperm_get > }, > + [REGSET_CET64] = { > + .core_note_type = NT_X86_CET, > + .n = sizeof(struct cet_user_state) / sizeof(u64), > + .size = sizeof(u64), .align = sizeof(u64), > + .active = cetregs_active, .get = cetregs_get, > + .set = cetregs_set > + }, Ok, could we first please make this part of the regset code more readable and start the series with a standalone clean-up patch that changes these initializers to something more readable: [REGSET_CET64] = { .core_note_type = NT_X86_CET, .n = sizeof(struct cet_user_state) / sizeof(u64), .size = sizeof(u64), .align = sizeof(u64), .active = cetregs_active, .get = cetregs_get, .set = cetregs_set }, ? (I'm demonstrating the cleanup based on REGSET_CET64, but this should be done on every other entry first.) > --- a/include/uapi/linux/elf.h > +++ b/include/uapi/linux/elf.h > @@ -401,6 +401,7 @@ typedef struct elf64_shdr { > #define NT_386_TLS 0x200 /* i386 TLS slots (struct user_desc) */ > #define NT_386_IOPERM 0x201 /* x86 io permission bitmap (1=deny) */ > #define NT_X86_XSTATE 0x202 /* x86 extended state using xsave */ > +#define NT_X86_CET 0x203 /* x86 cet state */ Acronyms in comments should be in capital letters. Also, I think I asked this before: why does "Control Flow Enforcement" abbreviate to "CET" (which is a well-known acronym for "Central European Time"), not to CFE? Thanks, Ingo
On Wed, 2018-07-11 at 12:20 +0200, Ingo Molnar wrote: > * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > > Add PTRACE interface for CET MSRs. > Please *always* describe new ABIs in the changelog, in a precise, > well-documented > way. Ok! > > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c > > index e2ee403865eb..ac2bc3a18427 100644 > > --- a/arch/x86/kernel/ptrace.c > > +++ b/arch/x86/kernel/ptrace.c > > @@ -49,7 +49,9 @@ enum x86_regset { > > REGSET_IOPERM64 = REGSET_XFP, > > REGSET_XSTATE, > > REGSET_TLS, > > + REGSET_CET64 = REGSET_TLS, > > REGSET_IOPERM32, > > + REGSET_CET32, > > }; > Why does REGSET_CET64 alias on REGSET_TLS? In x86_64_regsets[], there is no [REGSET_TLS]. The core dump code cannot handle holes in the array. > > > > > struct pt_regs_offset { > > @@ -1276,6 +1278,13 @@ static struct user_regset x86_64_regsets[] > > __ro_after_init = { > > .size = sizeof(long), .align = sizeof(long), > > .active = ioperm_active, .get = ioperm_get > > }, > > + [REGSET_CET64] = { > > + .core_note_type = NT_X86_CET, > > + .n = sizeof(struct cet_user_state) / sizeof(u64), > > + .size = sizeof(u64), .align = sizeof(u64), > > + .active = cetregs_active, .get = cetregs_get, > > + .set = cetregs_set > > + }, > Ok, could we first please make this part of the regset code more > readable and > start the series with a standalone clean-up patch that changes these > initializers > to something more readable: > > [REGSET_CET64] = { > .core_note_type = NT_X86_CET, > .n = sizeof(struct cet_user_state) / > sizeof(u64), > .size = sizeof(u64), > .align = sizeof(u64), > .active = cetregs_active, > .get = cetregs_get, > .set = cetregs_set > }, > > ? (I'm demonstrating the cleanup based on REGSET_CET64, but this > should be done on > every other entry first.) > I will fix it. > > > > > --- a/include/uapi/linux/elf.h > > +++ b/include/uapi/linux/elf.h > > @@ -401,6 +401,7 @@ typedef struct elf64_shdr { > > #define NT_386_TLS 0x200 /* i386 TLS slots > > (struct user_desc) */ > > #define NT_386_IOPERM 0x201 /* x86 io > > permission bitmap (1=deny) */ > > #define NT_X86_XSTATE 0x202 /* x86 extended > > state using xsave */ > > +#define NT_X86_CET 0x203 /* x86 cet state */ > Acronyms in comments should be in capital letters. > > Also, I think I asked this before: why does "Control Flow > Enforcement" abbreviate > to "CET" (which is a well-known acronym for "Central European Time"), > not to CFE? > I don't know if I can change that, will find out. Thanks, Yu-cheng
* Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c > > > index e2ee403865eb..ac2bc3a18427 100644 > > > --- a/arch/x86/kernel/ptrace.c > > > +++ b/arch/x86/kernel/ptrace.c > > > @@ -49,7 +49,9 @@ enum x86_regset { > > > REGSET_IOPERM64 = REGSET_XFP, > > > REGSET_XSTATE, > > > REGSET_TLS, > > > + REGSET_CET64 = REGSET_TLS, > > > REGSET_IOPERM32, > > > + REGSET_CET32, > > > }; > > Why does REGSET_CET64 alias on REGSET_TLS? > > In x86_64_regsets[], there is no [REGSET_TLS]. The core dump code > cannot handle holes in the array. Is there a fundamental (ABI) reason for that? > > to "CET" (which is a well-known acronym for "Central European Time"), > > not to CFE? > > > > I don't know if I can change that, will find out. So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal names, except for the Intel feature bit and any MSR enumeration which can be CET if Intel named it that way, and a short comment explaining the acronym difference. Or something like that. Thanks, Ingo
On Thu, 2018-07-12 at 16:03 +0200, Ingo Molnar wrote: > * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > > > > > > > > > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c > > > > index e2ee403865eb..ac2bc3a18427 100644 > > > > --- a/arch/x86/kernel/ptrace.c > > > > +++ b/arch/x86/kernel/ptrace.c > > > > @@ -49,7 +49,9 @@ enum x86_regset { > > > > REGSET_IOPERM64 = REGSET_XFP, > > > > REGSET_XSTATE, > > > > REGSET_TLS, > > > > + REGSET_CET64 = REGSET_TLS, > > > > REGSET_IOPERM32, > > > > + REGSET_CET32, > > > > }; > > > Why does REGSET_CET64 alias on REGSET_TLS? > > In x86_64_regsets[], there is no [REGSET_TLS]. The core dump code > > cannot handle holes in the array. > Is there a fundamental (ABI) reason for that? What I did was, ran Linux with 'slub_debug', and forced a core dump (kill -abrt <pid>), then there was a red zone warning in the dmesg. My feeling is there could be issues in the core dump code. These enum's are only local to arch/x86/kernel/ptrace.c and not exported. I am not aware this is in the ABI. > > > > > > > > > to "CET" (which is a well-known acronym for "Central European Time"), > > > not to CFE? > > > > > I don't know if I can change that, will find out. > So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal > names, except for the Intel feature bit and any MSR enumeration which can be CET > if Intel named it that way, and a short comment explaining the acronym difference. > > Or something like that. Ok, I will make changes in the next version and probably revise from that if still not optimal. Yu-cheng
On Thu, 12 Jul 2018, Yu-cheng Yu wrote: > On Thu, 2018-07-12 at 16:03 +0200, Ingo Molnar wrote: > > * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c > > > > > index e2ee403865eb..ac2bc3a18427 100644 > > > > > --- a/arch/x86/kernel/ptrace.c > > > > > +++ b/arch/x86/kernel/ptrace.c > > > > > @@ -49,7 +49,9 @@ enum x86_regset { > > > > > REGSET_IOPERM64 = REGSET_XFP, > > > > > REGSET_XSTATE, > > > > > REGSET_TLS, > > > > > + REGSET_CET64 = REGSET_TLS, > > > > > REGSET_IOPERM32, > > > > > + REGSET_CET32, > > > > > }; > > > > Why does REGSET_CET64 alias on REGSET_TLS? > > > In x86_64_regsets[], there is no [REGSET_TLS]. The core dump code > > > cannot handle holes in the array. > > Is there a fundamental (ABI) reason for that? > > What I did was, ran Linux with 'slub_debug', and forced a core dump > (kill -abrt <pid>), then there was a red zone warning in the dmesg. > My feeling is there could be issues in the core dump code. These Kernel development is not about feelings. Either you can track down the root cause or you cannot. There is no place for feelings and no place in between. And if you cannot track down the root cause and explain it proper then the resulting patch is just papering over the symptoms and will come back to hunt you (or others) sooner than later. No if, no could, no feelings. Facts is what matters. Really. Thanks, tglx
> > > to "CET" (which is a well-known acronym for "Central European Time"), > > > not to CFE? > > > > > > > I don't know if I can change that, will find out. > > So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal > names, except for the Intel feature bit and any MSR enumeration which can be CET > if Intel named it that way, and a short comment explaining the acronym difference. > > Or something like that. Actually, I don't think CFT is much better -- there's limited number of TLAs (*). "ENFORCE_FLOW"? "FLOWE"? "EFLOW"? Pavel (*) Three letter accronyms.
* Pavel Machek <pavel@ucw.cz> wrote: > > > > > to "CET" (which is a well-known acronym for "Central European Time"), > > > > not to CFE? > > > > > > > > > > I don't know if I can change that, will find out. > > > > So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal > > names, except for the Intel feature bit and any MSR enumeration which can be CET > > if Intel named it that way, and a short comment explaining the acronym difference. > > > > Or something like that. > > Actually, I don't think CFT is much better -- there's limited number > of TLAs (*). "ENFORCE_FLOW"? "FLOWE"? "EFLOW"? Erm, I wanted to say 'CFE', i.e. the abbreviation of 'Control Flow Enforcement'. But I guess I can live with CET as well ... Thanks, Ingo
On Fri, 2018-07-13 at 01:08 +0200, Thomas Gleixner wrote: > On Thu, 12 Jul 2018, Yu-cheng Yu wrote: > > > > On Thu, 2018-07-12 at 16:03 +0200, Ingo Molnar wrote: > > > > > > * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c > > > > > > index e2ee403865eb..ac2bc3a18427 100644 > > > > > > --- a/arch/x86/kernel/ptrace.c > > > > > > +++ b/arch/x86/kernel/ptrace.c > > > > > > @@ -49,7 +49,9 @@ enum x86_regset { > > > > > > REGSET_IOPERM64 = REGSET_XFP, > > > > > > REGSET_XSTATE, > > > > > > REGSET_TLS, > > > > > > + REGSET_CET64 = REGSET_TLS, > > > > > > REGSET_IOPERM32, > > > > > > + REGSET_CET32, > > > > > > }; > > > > > Why does REGSET_CET64 alias on REGSET_TLS? > > > > In x86_64_regsets[], there is no [REGSET_TLS]. The core dump code > > > > cannot handle holes in the array. > > > Is there a fundamental (ABI) reason for that? > > What I did was, ran Linux with 'slub_debug', and forced a core dump > > (kill -abrt <pid>), then there was a red zone warning in the dmesg. > > My feeling is there could be issues in the core dump code. These > Kernel development is not about feelings. I got that :-) > > Either you can track down the root cause or you cannot. There is no place > for feelings and no place in between. And if you cannot track down the root > cause and explain it proper then the resulting patch is just papering over > the symptoms and will come back to hunt you (or others) sooner than later. > > No if, no could, no feelings. Facts is what matters. Really. In kernel/ptrace.c, find_regset(const struct user_regset_view *view, unsigned int type) { const struct user_regset *regset; int n; for (n = 0; n < view->n; ++n) { regset = view->regsets + n; if (regset->core_note_type == type) return regset; } return NULL; } If there is a hole in the regset array, the empty slot's regset->core_note_type is not defined. We can add some comments near those enum's. Yu-cheng
On Fri 2018-07-13 15:33:58, Ingo Molnar wrote: > > * Pavel Machek <pavel@ucw.cz> wrote: > > > > > > > > to "CET" (which is a well-known acronym for "Central European Time"), > > > > > not to CFE? > > > > > > > > > > > > > I don't know if I can change that, will find out. > > > > > > So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal > > > names, except for the Intel feature bit and any MSR enumeration which can be CET > > > if Intel named it that way, and a short comment explaining the acronym difference. > > > > > > Or something like that. > > > > Actually, I don't think CFT is much better -- there's limited number > > of TLAs (*). "ENFORCE_FLOW"? "FLOWE"? "EFLOW"? > > Erm, I wanted to say 'CFE', i.e. the abbreviation of 'Control Flow Enforcement'. > > But I guess I can live with CET as well ... Yeah, and I am trying to say that perhaps we should use something longer than three letters. It will make code longer but easier to read. Pavel
diff --git a/arch/x86/include/asm/fpu/regset.h b/arch/x86/include/asm/fpu/regset.h index d5bdffb9d27f..edad0d889084 100644 --- a/arch/x86/include/asm/fpu/regset.h +++ b/arch/x86/include/asm/fpu/regset.h @@ -7,11 +7,12 @@ #include <linux/regset.h> -extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active; +extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active, + cetregs_active; extern user_regset_get_fn fpregs_get, xfpregs_get, fpregs_soft_get, - xstateregs_get; + xstateregs_get, cetregs_get; extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set, - xstateregs_set; + xstateregs_set, cetregs_set; /* * xstateregs_active == regset_fpregs_active. Please refer to the comment diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c index bc02f5144b95..7008eb084d36 100644 --- a/arch/x86/kernel/fpu/regset.c +++ b/arch/x86/kernel/fpu/regset.c @@ -160,6 +160,47 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset, return ret; } +int cetregs_active(struct task_struct *target, const struct user_regset *regset) +{ +#ifdef CONFIG_X86_INTEL_CET + if (target->thread.cet.shstk_enabled || target->thread.cet.ibt_enabled) + return regset->n; +#endif + return 0; +} + +int cetregs_get(struct task_struct *target, const struct user_regset *regset, + unsigned int pos, unsigned int count, + void *kbuf, void __user *ubuf) +{ + struct fpu *fpu = &target->thread.fpu; + struct cet_user_state *cetregs; + + if (!boot_cpu_has(X86_FEATURE_SHSTK)) + return -ENODEV; + + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_MASK_SHSTK_USER); + + fpu__prepare_read(fpu); + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, cetregs, 0, -1); +} + +int cetregs_set(struct task_struct *target, const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + struct fpu *fpu = &target->thread.fpu; + struct cet_user_state *cetregs; + + if (!boot_cpu_has(X86_FEATURE_SHSTK)) + return -ENODEV; + + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_MASK_SHSTK_USER); + + fpu__prepare_write(fpu); + return user_regset_copyin(&pos, &count, &kbuf, &ubuf, cetregs, 0, -1); +} + #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION /* diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index e2ee403865eb..ac2bc3a18427 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -49,7 +49,9 @@ enum x86_regset { REGSET_IOPERM64 = REGSET_XFP, REGSET_XSTATE, REGSET_TLS, + REGSET_CET64 = REGSET_TLS, REGSET_IOPERM32, + REGSET_CET32, }; struct pt_regs_offset { @@ -1276,6 +1278,13 @@ static struct user_regset x86_64_regsets[] __ro_after_init = { .size = sizeof(long), .align = sizeof(long), .active = ioperm_active, .get = ioperm_get }, + [REGSET_CET64] = { + .core_note_type = NT_X86_CET, + .n = sizeof(struct cet_user_state) / sizeof(u64), + .size = sizeof(u64), .align = sizeof(u64), + .active = cetregs_active, .get = cetregs_get, + .set = cetregs_set + }, }; static const struct user_regset_view user_x86_64_view = { @@ -1331,6 +1340,13 @@ static struct user_regset x86_32_regsets[] __ro_after_init = { .size = sizeof(u32), .align = sizeof(u32), .active = ioperm_active, .get = ioperm_get }, + [REGSET_CET32] = { + .core_note_type = NT_X86_CET, + .n = sizeof(struct cet_user_state) / sizeof(u64), + .size = sizeof(u64), .align = sizeof(u64), + .active = cetregs_active, .get = cetregs_get, + .set = cetregs_set + }, }; static const struct user_regset_view user_x86_32_view = { diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index dc93982b9664..0898ba719fd7 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -401,6 +401,7 @@ typedef struct elf64_shdr { #define NT_386_TLS 0x200 /* i386 TLS slots (struct user_desc) */ #define NT_386_IOPERM 0x201 /* x86 io permission bitmap (1=deny) */ #define NT_X86_XSTATE 0x202 /* x86 extended state using xsave */ +#define NT_X86_CET 0x203 /* x86 cet state */ #define NT_S390_HIGH_GPRS 0x300 /* s390 upper register halves */ #define NT_S390_TIMER 0x301 /* s390 timer register */ #define NT_S390_TODCMP 0x302 /* s390 TOD clock comparator register */
Add PTRACE interface for CET MSRs. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- arch/x86/include/asm/fpu/regset.h | 7 +++--- arch/x86/kernel/fpu/regset.c | 41 +++++++++++++++++++++++++++++++ arch/x86/kernel/ptrace.c | 16 ++++++++++++ include/uapi/linux/elf.h | 1 + 4 files changed, 62 insertions(+), 3 deletions(-)