From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938038AbdAEUiY (ORCPT ); Thu, 5 Jan 2017 15:38:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36558 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1034465AbdAEUh5 (ORCPT ); Thu, 5 Jan 2017 15:37:57 -0500 Date: Thu, 5 Jan 2017 14:37:55 -0600 From: Josh Poimboeuf To: Dmitry Vyukov Cc: syzkaller , Andrey Konovalov , Andrey Ryabinin , Alexander Potapenko , kasan-dev , "linux-mm@kvack.org" , LKML , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , Kostya Serebryany Subject: Re: x86: warning in unwind_get_return_address Message-ID: <20170105203755.ai3ida5p2twwtvx6@treble> References: <20161220233640.pc4goscldmpkvtqa@treble> <20161222051701.soqwh47frxwsbkni@treble> <20170105144942.whqucdwmeqybng3s@treble> <20170105151700.4n7wpynvsv2yjotp@treble> <20170105170352.4i57lv6ka2k6nqsk@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 05 Jan 2017 20:37:58 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 05, 2017 at 09:23:14PM +0100, Dmitry Vyukov wrote: > On Thu, Jan 5, 2017 at 6:03 PM, Josh Poimboeuf wrote: > > On Thu, Jan 05, 2017 at 09:17:00AM -0600, Josh Poimboeuf wrote: > >> On Thu, Jan 05, 2017 at 03:59:52PM +0100, Dmitry Vyukov wrote: > >> > On Thu, Jan 5, 2017 at 3:49 PM, Josh Poimboeuf wrote: > >> > > On Tue, Dec 27, 2016 at 05:38:59PM +0100, Dmitry Vyukov wrote: > >> > >> On Thu, Dec 22, 2016 at 6:17 AM, Josh Poimboeuf wrote: > >> > >> > On Wed, Dec 21, 2016 at 01:46:36PM +0100, Andrey Konovalov wrote: > >> > >> >> On Wed, Dec 21, 2016 at 12:36 AM, Josh Poimboeuf wrote: > >> > >> >> > > >> > >> >> > Thanks. Looking at the stack trace, my guess is that an interrupt hit > >> > >> >> > while running in generated BPF code, and the unwinder got confused > >> > >> >> > because regs->ip points to the generated code. I may need to disable > >> > >> >> > that warning until we figure out a better solution. > >> > >> >> > > >> > >> >> > Can you share your .config file? > >> > >> >> > >> > >> >> Sure, attached. > >> > >> > > >> > >> > Ok, I was able to recreate with your config. The culprit was generated > >> > >> > code, as I suspected, though it wasn't BPF, it was a kprobe (created by > >> > >> > dccpprobe_init()). > >> > >> > > >> > >> > I'll make a patch to disable the warning. > >> > >> > >> > >> Hi, > >> > >> > >> > >> I am also seeing the following warnings: > >> > >> > >> > >> [ 281.889259] WARNING: kernel stack regs at ffff8801c29a7ea8 in > >> > >> syz-executor8:1302 has bad 'bp' value ffff8801c29a7f28 > >> > >> [ 833.994878] WARNING: kernel stack regs at ffff8801c4e77ea8 in > >> > >> syz-executor1:13094 has bad 'bp' value ffff8801c4e77f28 > >> > >> > >> > >> Can it also be caused by bpf/kprobe? > >> > > > >> > > This is a different warning. I suspect it's due to unwinding the stack > >> > > of another CPU while it's running, which is still possible in a few > >> > > places. I'm going to have to disable all these warnings for now. > >> > > >> > > >> > I also have the following diff locally. These loads trigger episodic > >> > KASAN warnings about stack-of-bounds reads on rcu stall warnings when > >> > it does backtrace of all cpus. > >> > If it looks correct to you, can you please also incorporate it into your patch? > >> > >> Ok, will do. > >> > >> BTW, I think there's an issue with your mail client. Your last two > >> replies to me didn't have me on To/Cc. > > > > Would you mind testing if the following patch fixes it? It's based on > > the latest linus/master. > > > > > > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c > > index 4443e49..05adf9a 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 UNWIND_READ_ONCE(state, x) \ > > +({ \ > > + unsigned long val; \ > > + if (state->task == current) \ > > + val = READ_ONCE(x); \ > > + else \ > > + val = READ_ONCE_NOCHECK(x); \ > > + val; \ > > +}) > > + > > static void unwind_dump(struct unwind_state *state, unsigned long *sp) > > { > > static bool dumped_before = false; > > @@ -48,7 +63,8 @@ unsigned long unwind_get_return_address(struct unwind_state *state) > > if (state->regs && user_mode(state->regs)) > > return 0; > > > > - addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p, > > + addr = UNWIND_READ_ONCE(state, *addr_p); > > + addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, addr, > > addr_p); > > > > return __kernel_text_address(addr) ? addr : 0; > > @@ -162,7 +178,7 @@ bool unwind_next_frame(struct unwind_state *state) > > if (state->regs) > > next_bp = (unsigned long *)state->regs->bp; > > else > > - next_bp = (unsigned long *)*state->bp; > > + next_bp = (unsigned long *)UNWIND_READ_ONCE(state, *state->bp); > > > > /* is the next frame pointer an encoded pointer to pt_regs? */ > > regs = decode_frame_pointer(next_bp); > > @@ -207,6 +223,16 @@ bool unwind_next_frame(struct unwind_state *state) > > return true; > > > > bad_address: > > + /* > > + * When dumping a task other than current, the task might actually be > > + * running on another CPU, in which case it could be modifying its > > + * stack while we're reading it. This is generally not a problem and > > + * can be ignored as long as the caller understands that unwinding > > + * another task will not always succeed. > > + */ > > + if (state->task != current) > > + goto the_end; > > + > > if (state->regs) { > > printk_deferred_once(KERN_WARNING > > "WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n", > > > Applied locally for testing. > > > What about this part? > > diff --git a/arch/x86/include/asm/stacktrace.h > b/arch/x86/include/asm/stacktrace.h > index a3269c897ec5..d8d4fc66ffec 100644 > --- a/arch/x86/include/asm/stacktrace.h > +++ b/arch/x86/include/asm/stacktrace.h > @@ -58,7 +58,7 @@ get_frame_pointer(struct task_struct *task, struct > pt_regs *regs) > if (task == current) > return __builtin_frame_address(0); > > - return (unsigned long *)((struct > inactive_task_frame*)task->thread.sp)->bp; > + return (unsigned long *)READ_ONCE_NOCHECK(((struct > inactive_task_frame *)task->thread.sp)->bp); > } > #else > static inline unsigned long * Oops, I missed that part. That's needed too. BTW, I'm still not on your email To: list. -- Josh