Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1592960 > unrolled thread

Re: stack frame unwindind KASAN errors

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: stack frame unwindind KASAN errors Daniel J Blueman <daniel@quora.org> - 2017-03-06 08:10 +0100
    Re: stack frame unwindind KASAN errors Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-06 18:00 +0100
      Re: stack frame unwindind KASAN errors Daniel J Blueman <daniel@quora.org> - 2017-03-07 03:10 +0100
        Re: stack frame unwindind KASAN errors Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-07 15:40 +0100

#1592960 — Re: stack frame unwindind KASAN errors

On 27 February 2017 at 23:47, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, Feb 27, 2017 at 12:49:59PM +0800, Daniel J Blueman wrote:
>> On 4.9.13 with KASAN enabled [1], we see a number of stack unwinding
>> errors reported [2,3].
>>
>> This seems to occur at half of boots.
>>
>> Let me know for further debug info/patch testing and thanks,
>>   Daniel
>>
>> [1] https://quora.org/config
>> [2] https://quora.org/dmesg.txt
>
> Hi Daniel,
>
> Can you try the following patch?  It's a backport of the following
> upstream commit:
>
>   09ae68dd0a8d ("x86/unwind: Disable KASAN checks for non-current tasks")
>
> If it fixes it then I'll submit it for 4.9 stable.
>
> ---
>
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH] x86/unwind: Disable KASAN checks for non-current tasks
>
> There are a handful of callers to save_stack_trace_tsk() and
> show_stack() which try to unwind the stack of a task other than current.
> In such cases, it's remotely possible that the task is running on one
> CPU while the unwinder is reading its stack from another CPU, causing
> the unwinder to see stack corruption.
>
> These cases seem to be mostly harmless.  The unwinder has checks which
> prevent it from following bad pointers beyond the bounds of the stack.
> So it's not really a bug as long as the caller understands that
> unwinding another task will not always succeed.
>
> In such cases, it's possible that the unwinder may read a KASAN-poisoned
> region of the stack.  Account for that by using READ_ONCE_NOCHECK() when
> reading the stack of another task.
>
> Use READ_ONCE() when reading the stack of the current task, since KASAN
> warnings can still be useful for finding bugs in that case.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Brian Gerst <brgerst@gmail.com>
> Cc: Dave Jones <davej@codemonkey.org.uk>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Miroslav Benes <mbenes@suse.cz>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/4c575eb288ba9f73d498dfe0acde2f58674598f1.1483978430.git.jpoimboe@redhat.com
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/include/asm/stacktrace.h |  5 ++++-
>  arch/x86/kernel/unwind_frame.c    | 20 ++++++++++++++++++--
>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
> index 37f2e0b..4141ead 100644
> --- a/arch/x86/include/asm/stacktrace.h
> +++ b/arch/x86/include/asm/stacktrace.h
> @@ -55,13 +55,16 @@ extern int kstack_depth_to_print;
>  static inline unsigned long *
>  get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
>  {
> +       struct inactive_task_frame *frame;
> +
>         if (regs)
>                 return (unsigned long *)regs->bp;
>
>         if (task == current)
>                 return __builtin_frame_address(0);
>
> -       return (unsigned long *)((struct inactive_task_frame *)task->thread.sp)->bp;
> +       frame = (struct inactive_task_frame *)task->thread.sp;
> +       return (unsigned long *)READ_ONCE_NOCHECK(frame->bp);
>  }
>  #else
>  static inline unsigned long *
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> index a2456d4..caff129 100644
> --- a/arch/x86/kernel/unwind_frame.c
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -6,6 +6,21 @@
>
>  #define FRAME_HEADER_SIZE (sizeof(long) * 2)
>
> +/*
> + * This disables KASAN checking when reading a value from another task's stack,
> + * since the other task could be running on another CPU and could have poisoned
> + * the stack in the meantime.
> + */
> +#define READ_ONCE_TASK_STACK(task, x)                  \
> +({                                                     \
> +       unsigned long val;                              \
> +       if (task == current)                            \
> +               val = READ_ONCE(x);                     \
> +       else                                            \
> +               val = READ_ONCE_NOCHECK(x);             \
> +       val;                                            \
> +})
> +
>  unsigned long unwind_get_return_address(struct unwind_state *state)
>  {
>         unsigned long addr;
> @@ -14,7 +29,8 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
>         if (unwind_done(state))
>                 return 0;
>
> -       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> +       addr = READ_ONCE_TASK_STACK(state->task, *addr_p);
> +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, addr,
>                                      addr_p);
>
>         return __kernel_text_address(addr) ? addr : 0;
> @@ -48,7 +64,7 @@ bool unwind_next_frame(struct unwind_state *state)
>         if (unwind_done(state))
>                 return false;
>
> -       next_bp = (unsigned long *)*state->bp;
> +       next_bp = (unsigned long *)READ_ONCE_TASK_STACK(state->task, *state->bp);
>
>         /* make sure the next frame's data is accessible */
>         if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))

Thanks Josh!

With this patch, the KASAN warning still occurs, but at
unwind_get_return_address+0x1d3/0x130 instead; the rest of the trace
is identical.

(gdb) list *(unwind_get_return_address+0x1d3)
0xffffffff8112bca3 is in unwind_get_return_address
(./include/linux/compiler.h:243).
238    })
239
240    static __always_inline
241    void __read_once_size(const volatile void *p, void *res, int size)
242    {
243        __READ_ONCE_SIZE;

Thanks,
  Daniel
-- 
Daniel J Blueman

[toc] | [next] | [standalone]

#1593534

On Mon, Mar 06, 2017 at 02:52:01PM +0800, Daniel J Blueman wrote:
> Thanks Josh!
> 
> With this patch, the KASAN warning still occurs, but at
> unwind_get_return_address+0x1d3/0x130 instead; the rest of the trace
> is identical.
> 
> (gdb) list *(unwind_get_return_address+0x1d3)
> 0xffffffff8112bca3 is in unwind_get_return_address
> (./include/linux/compiler.h:243).
> 238    })
> 239
> 240    static __always_inline
> 241    void __read_once_size(const volatile void *p, void *res, int size)
> 242    {
> 243        __READ_ONCE_SIZE;

Looking deeper, I have an idea about what's going on:

  https://quora.org/dmesg.txt

Each of the warnings seems to show an interrupt happening during an EFI
call.  I'm guessing EFI modified the frame pointer, at least
temporarily, which confused the unwinder :-(

Would it be possible for you to test again with 4.10?  It has some
additional unwinder output which should hopefully confirm my suspicions.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1593858

On 7 March 2017 at 00:40, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, Mar 06, 2017 at 02:52:01PM +0800, Daniel J Blueman wrote:
>> Thanks Josh!
>>
>> With this patch, the KASAN warning still occurs, but at
>> unwind_get_return_address+0x1d3/0x130 instead; the rest of the trace
>> is identical.
>>
>> (gdb) list *(unwind_get_return_address+0x1d3)
>> 0xffffffff8112bca3 is in unwind_get_return_address
>> (./include/linux/compiler.h:243).
>> 238    })
>> 239
>> 240    static __always_inline
>> 241    void __read_once_size(const volatile void *p, void *res, int size)
>> 242    {
>> 243        __READ_ONCE_SIZE;
>
> Looking deeper, I have an idea about what's going on:
>
>   https://quora.org/dmesg.txt
>
> Each of the warnings seems to show an interrupt happening during an EFI
> call.  I'm guessing EFI modified the frame pointer, at least
> temporarily, which confused the unwinder :-(
>
> Would it be possible for you to test again with 4.10?  It has some
> additional unwinder output which should hopefully confirm my suspicions.

Very good; I don't see the KASAN warnings with 4.10 in the same environment.

Thanks,
  Daniel
-- 
Daniel J Blueman

[toc] | [prev] | [next] | [standalone]

#1594283

On Tue, Mar 07, 2017 at 10:04:11AM +0800, Daniel J Blueman wrote:
> On 7 March 2017 at 00:40, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Mon, Mar 06, 2017 at 02:52:01PM +0800, Daniel J Blueman wrote:
> >> Thanks Josh!
> >>
> >> With this patch, the KASAN warning still occurs, but at
> >> unwind_get_return_address+0x1d3/0x130 instead; the rest of the trace
> >> is identical.
> >>
> >> (gdb) list *(unwind_get_return_address+0x1d3)
> >> 0xffffffff8112bca3 is in unwind_get_return_address
> >> (./include/linux/compiler.h:243).
> >> 238    })
> >> 239
> >> 240    static __always_inline
> >> 241    void __read_once_size(const volatile void *p, void *res, int size)
> >> 242    {
> >> 243        __READ_ONCE_SIZE;
> >
> > Looking deeper, I have an idea about what's going on:
> >
> >   https://quora.org/dmesg.txt
> >
> > Each of the warnings seems to show an interrupt happening during an EFI
> > call.  I'm guessing EFI modified the frame pointer, at least
> > temporarily, which confused the unwinder :-(
> >
> > Would it be possible for you to test again with 4.10?  It has some
> > additional unwinder output which should hopefully confirm my suspicions.
> 
> Very good; I don't see the KASAN warnings with 4.10 in the same environment.

To be honest, that's not the result I was expecting :-)

It would be nice to know what fixed it.  Any chance you could bisect?

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web