Remove __end_entry_SYSENTER_compat?

Remove __end_entry_SYSENTER_compat?

[Borislav Petkov]

Anyone think this is an OK-ish idea?

It saves us the global symbol but requires the two functions to remained
glued together. :-\

---
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index e1721dafbcb1..262519da8661 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -131,7 +131,6 @@ ENTRY(entry_SYSENTER_compat)
 	pushq	$X86_EFLAGS_FIXED
 	popfq
 	jmp	.Lsysenter_flags_fixed
-GLOBAL(__end_entry_SYSENTER_compat)
 ENDPROC(entry_SYSENTER_compat)
 
 /*
@@ -180,6 +179,9 @@ ENDPROC(entry_SYSENTER_compat)
  * edi  arg5
  * esp  user stack
  * 0(%esp) arg6
+ *
+ * DO NOT! move this function and the above before adjusting
+ * is_sysenter_singlestep().
  */
 ENTRY(entry_SYSCALL_compat)
 	/* Interrupts are off on entry. */
diff --git a/arch/x86/include/asm/proto.h b/arch/x86/include/asm/proto.h
index 8d3964fc5f91..afdef9f3f0f0 100644
--- a/arch/x86/include/asm/proto.h
+++ b/arch/x86/include/asm/proto.h
@@ -21,7 +21,6 @@ void __end_SYSENTER_singlestep_region(void);
 
 #ifdef CONFIG_IA32_EMULATION
 void entry_SYSENTER_compat(void);
-void __end_entry_SYSENTER_compat(void);
 void entry_SYSCALL_compat(void);
 void entry_INT80_compat(void);
 #endif
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index bf54309b85da..143902ffe9ff 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -668,7 +668,7 @@ static bool is_sysenter_singlestep(struct pt_regs *regs)
 		(unsigned long)__begin_SYSENTER_singlestep_region;
 #elif defined(CONFIG_IA32_EMULATION)
 	return (regs->ip - (unsigned long)entry_SYSENTER_compat) <
-		(unsigned long)__end_entry_SYSENTER_compat -
+		(unsigned long)entry_SYSCALL_compat -
 		(unsigned long)entry_SYSENTER_compat;
 #else
 	return false;

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Remove __end_entry_SYSENTER_compat?

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> Anyone think this is an OK-ish idea?
> 
> It saves us the global symbol but requires the two functions to remained
> glued together. :-\
> 
> ---
> diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
> index e1721dafbcb1..262519da8661 100644
> --- a/arch/x86/entry/entry_64_compat.S
> +++ b/arch/x86/entry/entry_64_compat.S
> @@ -131,7 +131,6 @@ ENTRY(entry_SYSENTER_compat)
>  	pushq	$X86_EFLAGS_FIXED
>  	popfq
>  	jmp	.Lsysenter_flags_fixed
> -GLOBAL(__end_entry_SYSENTER_compat)
>  ENDPROC(entry_SYSENTER_compat)
>  
>  /*
> @@ -180,6 +179,9 @@ ENDPROC(entry_SYSENTER_compat)
>   * edi  arg5
>   * esp  user stack
>   * 0(%esp) arg6
> + *
> + * DO NOT! move this function and the above before adjusting
> + * is_sysenter_singlestep().
>   */
>  ENTRY(entry_SYSCALL_compat)
>  	/* Interrupts are off on entry. */
> diff --git a/arch/x86/include/asm/proto.h b/arch/x86/include/asm/proto.h
> index 8d3964fc5f91..afdef9f3f0f0 100644
> --- a/arch/x86/include/asm/proto.h
> +++ b/arch/x86/include/asm/proto.h
> @@ -21,7 +21,6 @@ void __end_SYSENTER_singlestep_region(void);
>  
>  #ifdef CONFIG_IA32_EMULATION
>  void entry_SYSENTER_compat(void);
> -void __end_entry_SYSENTER_compat(void);
>  void entry_SYSCALL_compat(void);
>  void entry_INT80_compat(void);
>  #endif
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index bf54309b85da..143902ffe9ff 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -668,7 +668,7 @@ static bool is_sysenter_singlestep(struct pt_regs *regs)
>  		(unsigned long)__begin_SYSENTER_singlestep_region;
>  #elif defined(CONFIG_IA32_EMULATION)
>  	return (regs->ip - (unsigned long)entry_SYSENTER_compat) <
> -		(unsigned long)__end_entry_SYSENTER_compat -
> +		(unsigned long)entry_SYSCALL_compat -
>  		(unsigned long)entry_SYSENTER_compat;
>  #else
>  	return false;

Hm, I'd argue that the old code is much clearer: we need both the start and the 
end of a function and have the properly named symbols for that.

That entry_SYSCALL_compat() happens to start just where 
__end_entry_SYSENTER_compat is an accident of placement.

Is it even true - doesn't ENTRY() imply an .align, in which case it might be that 
__end_entry_SYSENTER_compat != entry_SYSCALL_compat?

In fact that appears to be the case for my defconfig:

  ffffffff81942f90 T entry_SYSENTER_compat
  ffffffff81942feb T __end_entry_SYSENTER_compat
  ffffffff81942ff0 T entry_SYSCALL_compat

So unless there's some disadvantage beyond having one more symbol, I'd favor the 
old approach.

Thanks,

	Ingo

Re: Remove __end_entry_SYSENTER_compat?

[Borislav Petkov]

On Wed, Jul 12, 2017 at 10:48:03AM +0200, Ingo Molnar wrote:
> Hm, I'd argue that the old code is much clearer: we need both the start and the 
> end of a function and have the properly named symbols for that.
> 
> That entry_SYSCALL_compat() happens to start just where 
> __end_entry_SYSENTER_compat is an accident of placement.

Yeah, probably not worth the effort of actually making it less reliable
this way...

> Is it even true - doesn't ENTRY() imply an .align, in which case it might be that 
> __end_entry_SYSENTER_compat != entry_SYSCALL_compat?

Yes, ENTRY does .p2align:

.globl __end_entry_SYSENTER_compat; __end_entry_SYSENTER_compat:
.type entry_SYSENTER_compat, @function ; .size entry_SYSENTER_compat, .-entry_SYSENTER_compat
# 184 "arch/x86/entry/entry_64_compat.S"
.globl entry_SYSCALL_compat ; .p2align 4, 0x90 ; entry_SYSCALL_compat:
				^^^^^^
Pads to an alignment of 4 with NOPs.

> In fact that appears to be the case for my defconfig:
> 
>   ffffffff81942f90 T entry_SYSENTER_compat
>   ffffffff81942feb T __end_entry_SYSENTER_compat
>   ffffffff81942ff0 T entry_SYSCALL_compat
> 
> So unless there's some disadvantage beyond having one more symbol, I'd favor the 
> old approach.

Yeah, my only concern was getting rid of the global symbol but it
doesn't work in this case.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--