On 27 February 2017 at 23:47, Josh Poimboeuf <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Brian Gerst <[email protected]>
> Cc: Dave Jones <[email protected]>
> Cc: Denys Vlasenko <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Miroslav Benes <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Link: http://lkml.kernel.org/r/4c575eb288ba9f73d498dfe0acde2f58674598f1.1483978430.git.jpoimboe@redhat.com
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> 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
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
On 7 March 2017 at 00:40, Josh Poimboeuf <[email protected]> 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
On Tue, Mar 07, 2017 at 10:04:11AM +0800, Daniel J Blueman wrote:
> On 7 March 2017 at 00:40, Josh Poimboeuf <[email protected]> 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