Re: [PATCH 3/9] x86/entry/32: fix the end of the stack for newly forked tasks

[Brian Gerst]

On Tue, Sep 20, 2016 at 4:02 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> Thanks to all the recent x86 entry code refactoring, most tasks' kernel
> stacks start at the same offset right below their saved pt_regs,
> regardless of which syscall was used to enter the kernel.  That creates
> a nice convention which makes it straightforward to identify the end of
> the stack, which can be useful for the unwinder to verify the stack is
> sane.
>
> Calling schedule_tail() directly breaks that convention because its an
> asmlinkage function so its argument has to be pushed on the stack.  Add
> a wrapper which creates a proper "end of stack" frame header before the
> call.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  arch/x86/entry/entry_32.S | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index deef561..f0a7444 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -44,6 +44,7 @@
>  #include <asm/alternative-asm.h>
>  #include <asm/asm.h>
>  #include <asm/smap.h>
> +#include <asm/frame.h>
>
>         .section .entry.text, "ax"
>
> @@ -237,6 +238,23 @@ ENTRY(__switch_to_asm)
>  END(__switch_to_asm)
>
>  /*
> + * The unwinder expects the last frame on the stack to always be at the same
> + * offset from the end of the page, which allows it to validate the stack.
> + * Calling schedule_tail() directly would break that convention because its an
> + * asmlinkage function so its argument has to be pushed on the stack.  This
> + * wrapper creates a proper "end of stack" frame header before the call.
> + */
> +ENTRY(schedule_tail_wrapper)
> +       FRAME_BEGIN
> +
> +       pushl   %eax
> +       call    schedule_tail
> +       popl    %eax
> +
> +       FRAME_END
> +       ret
> +ENDPROC(schedule_tail_wrapper)
> +/*
>   * A newly forked process directly context switches into this address.
>   *
>   * eax: prev task we switched from
> @@ -244,9 +262,7 @@ END(__switch_to_asm)
>   * edi: kernel thread arg
>   */
>  ENTRY(ret_from_fork)
> -       pushl   %eax
> -       call    schedule_tail
> -       popl    %eax
> +       call    schedule_tail_wrapper
>
>         testl   %ebx, %ebx
>         jnz     1f              /* kernel threads are uncommon */
> --
> 2.7.4
>

Dropping asmlinkage from schedule_tail() would be a better option if possible.

--
Brian Gerst