Re: [RFC PATCH v2 25/27] x86/cet: Add PTRACE interface for CET

[Yu-cheng Yu]

On Wed, 2018-07-11 at 12:20 +0200, Ingo Molnar wrote:
> * Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx> 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