From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 08D8DC169C4 for ; Wed, 6 Feb 2019 08:46:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BB8592081B for ; Wed, 6 Feb 2019 08:46:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="IU6RZszG" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728383AbfBFIqF (ORCPT ); Wed, 6 Feb 2019 03:46:05 -0500 Received: from mail-vs1-f66.google.com ([209.85.217.66]:36611 "EHLO mail-vs1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726509AbfBFIqE (ORCPT ); Wed, 6 Feb 2019 03:46:04 -0500 Received: by mail-vs1-f66.google.com with SMTP id v205so3922863vsc.3; Wed, 06 Feb 2019 00:46:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=KMz/FfJ8citTyocOVR5b7eAlmHncYAbXwxZkfzhjJxo=; b=IU6RZszGpt+rwJ6J0ZjjtdGmS3AeOhxZAdFbTK6tm4DGXGqoLg3cwjNfUkiPg45N6O mCSUl0yz2i1zCztmwGT8ylNXVXPD6IPqq/8cC6GQ1+gdmEDWmAm6DNKijk4qUeHRKQoh 1PIlODCO1MD8K3ocGBJukXFu2l/8PzRIGfD82qL0jmusi7t04fF5dQY2hMdxWKvyChH6 rgC/V4O+aaTBWpihdVcJSWL2iBf8sOAKxr5gfvbgdeMmLjOfFlsqBXMFkFpGKq486Erx 3SZJyzImH5dtA6ZIX2FHUX/GG23BjhRVmGiPc1Hvr+58rPr81Giw3jpSkvBD0ZhdJE1P aUNQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KMz/FfJ8citTyocOVR5b7eAlmHncYAbXwxZkfzhjJxo=; b=DmfNGJhu4SN/NBupzuFart6FpC3/u2Hc8Rajf/XOU6VFeBr/QqZ+NmVjhVut/x7/D9 VjQ7Oato8Fgi2/KPxXe22i19nedczFGZSV+BDBrAyvZ6lxkt0x9LyoZwwb5658PJyLZC tMfNwAXLFsV07kCDM1ybeTlzgINk5ed8X8nz9xcBGmDA/l5toc0Vu6f22xuyQEAMNVu+ p/920Q8+IVIGU8QEfPW+o5S4h/BYIciYLxbMIG4H5aWOeU+l79BJ5m32v4MA0QXdYCp7 hOObn7IQGmGELz91VwjB8N9vQDB7Efns7b+NG2QIjUbafi5TS7J7gihLkXCc5XCTxFjB AFfA== X-Gm-Message-State: AHQUAubVu3FVZeHbcQwhOu8iq/RkeDm1fmUaBfrt3xNEm/uYRtUIujr6 1Eiiryhyq1zPMPQ4B9VatzXZ7Iqh45ugUEt+ysw= X-Google-Smtp-Source: AHgI3IaAk3aSqSa6NDWDV1qK+6WUWCp9O1gZBt5ShXdPm3BZ2/zppBFbV4Ut/djyEYWros1oN2la8Bwet/D3e2C9XKo= X-Received: by 2002:a67:a944:: with SMTP id s65mr3757403vse.108.1549442762656; Wed, 06 Feb 2019 00:46:02 -0800 (PST) MIME-Version: 1.0 References: <20190122155724.27557-1-joe.lawrence@redhat.com> <20190122155724.27557-2-joe.lawrence@redhat.com> <20190202011455.GN26056@350D> <87bm3qmqak.fsf@concordia.ellerman.id.au> <8736p1msov.fsf@concordia.ellerman.id.au> In-Reply-To: <8736p1msov.fsf@concordia.ellerman.id.au> From: Balbir Singh Date: Wed, 6 Feb 2019 19:45:50 +1100 Message-ID: Subject: Re: [PATCH 1/4] powerpc/64s: Clear on-stack exception marker upon exception return To: Michael Ellerman Cc: Joe Lawrence , "linux-kernel@vger.kernel.org" , "open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)" , live-patching@vger.kernel.org, Jiri Kosina , Josh Poimboeuf , Nicolai Stange , Torsten Duwe Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 6, 2019 at 3:44 PM Michael Ellerman wrote: > > Balbir Singh writes: > > On Tue, Feb 5, 2019 at 10:24 PM Michael Ellerman wrote: > >> Balbir Singh writes: > >> > On Sat, Feb 2, 2019 at 12:14 PM Balbir Singh wrote: > >> >> On Tue, Jan 22, 2019 at 10:57:21AM -0500, Joe Lawrence wrote: > >> >> > From: Nicolai Stange > >> >> > > >> >> > The ppc64 specific implementation of the reliable stacktracer, > >> >> > save_stack_trace_tsk_reliable(), bails out and reports an "unreliable > >> >> > trace" whenever it finds an exception frame on the stack. Stack frames > >> >> > are classified as exception frames if the STACK_FRAME_REGS_MARKER magic, > >> >> > as written by exception prologues, is found at a particular location. > >> >> > > >> >> > However, as observed by Joe Lawrence, it is possible in practice that > >> >> > non-exception stack frames can alias with prior exception frames and thus, > >> >> > that the reliable stacktracer can find a stale STACK_FRAME_REGS_MARKER on > >> >> > the stack. It in turn falsely reports an unreliable stacktrace and blocks > >> >> > any live patching transition to finish. Said condition lasts until the > >> >> > stack frame is overwritten/initialized by function call or other means. > >> >> > > >> >> > In principle, we could mitigate this by making the exception frame > >> >> > classification condition in save_stack_trace_tsk_reliable() stronger: > >> >> > in addition to testing for STACK_FRAME_REGS_MARKER, we could also take into > >> >> > account that for all exceptions executing on the kernel stack > >> >> > - their stack frames's backlink pointers always match what is saved > >> >> > in their pt_regs instance's ->gpr[1] slot and that > >> >> > - their exception frame size equals STACK_INT_FRAME_SIZE, a value > >> >> > uncommonly large for non-exception frames. > >> >> > > >> >> > However, while these are currently true, relying on them would make the > >> >> > reliable stacktrace implementation more sensitive towards future changes in > >> >> > the exception entry code. Note that false negatives, i.e. not detecting > >> >> > exception frames, would silently break the live patching consistency model. > >> >> > > >> >> > Furthermore, certain other places (diagnostic stacktraces, perf, xmon) > >> >> > rely on STACK_FRAME_REGS_MARKER as well. > >> >> > > >> >> > Make the exception exit code clear the on-stack STACK_FRAME_REGS_MARKER > >> >> > for those exceptions running on the "normal" kernel stack and returning > >> >> > to kernelspace: because the topmost frame is ignored by the reliable stack > >> >> > tracer anyway, returns to userspace don't need to take care of clearing > >> >> > the marker. > >> >> > > >> >> > Furthermore, as I don't have the ability to test this on Book 3E or > >> >> > 32 bits, limit the change to Book 3S and 64 bits. > >> >> > > >> >> > Finally, make the HAVE_RELIABLE_STACKTRACE Kconfig option depend on > >> >> > PPC_BOOK3S_64 for documentation purposes. Before this patch, it depended > >> >> > on PPC64 && CPU_LITTLE_ENDIAN and because CPU_LITTLE_ENDIAN implies > >> >> > PPC_BOOK3S_64, there's no functional change here. > >> >> > > >> >> > Fixes: df78d3f61480 ("powerpc/livepatch: Implement reliable stack tracing for the consistency model") > >> >> > Reported-by: Joe Lawrence > >> >> > Signed-off-by: Nicolai Stange > >> >> > Signed-off-by: Joe Lawrence > >> >> > --- > >> >> > arch/powerpc/Kconfig | 2 +- > >> >> > arch/powerpc/kernel/entry_64.S | 7 +++++++ > >> >> > 2 files changed, 8 insertions(+), 1 deletion(-) > >> >> > > >> >> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > >> >> > index 2890d36eb531..73bf87b1d274 100644 > >> >> > --- a/arch/powerpc/Kconfig > >> >> > +++ b/arch/powerpc/Kconfig > >> >> > @@ -220,7 +220,7 @@ config PPC > >> >> > select HAVE_PERF_USER_STACK_DUMP > >> >> > select HAVE_RCU_TABLE_FREE if SMP > >> >> > select HAVE_REGS_AND_STACK_ACCESS_API > >> >> > - select HAVE_RELIABLE_STACKTRACE if PPC64 && CPU_LITTLE_ENDIAN > >> >> > + select HAVE_RELIABLE_STACKTRACE if PPC_BOOK3S_64 && CPU_LITTLE_ENDIAN > >> >> > select HAVE_SYSCALL_TRACEPOINTS > >> >> > select HAVE_VIRT_CPU_ACCOUNTING > >> >> > select HAVE_IRQ_TIME_ACCOUNTING > >> >> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > >> >> > index 435927f549c4..a2c168b395d2 100644 > >> >> > --- a/arch/powerpc/kernel/entry_64.S > >> >> > +++ b/arch/powerpc/kernel/entry_64.S > >> >> > @@ -1002,6 +1002,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > >> >> > ld r2,_NIP(r1) > >> >> > mtspr SPRN_SRR0,r2 > >> >> > > >> >> > + /* > >> >> > + * Leaving a stale exception_marker on the stack can confuse > >> >> > + * the reliable stack unwinder later on. Clear it. > >> >> > + */ > >> >> > + li r2,0 > >> >> > + std r2,STACK_FRAME_OVERHEAD-16(r1) > >> >> > + > >> >> > >> >> Could you please double check, r4 is already 0 at this point > >> >> IIUC. So the change might be a simple > >> >> > >> >> std r4,STACK_FRAME_OVERHEAD-16(r1) > >> >> > >> > > >> > r4 is not 0, sorry for the noise > >> > >> Isn't it? > > > > It is, I seem to be reading the wrong bits and confused myself, had to > > re-read mtmsrd to ensure it does not modify RS, just MSR. So I guess > > we could reuse r4. > > Yeah it's a bit hard to follow now that we have the split exit paths for > user vs kernel. r4 does get used on the return to userspace case, by > ACCOUNT_CPU_USER_EXIT(), but for the return to kernel it's still got > zero in it. > > > Should I send a patch on top of this? I have limited testing > > infrastructure at the moment, I could use qemu > > I'm not sure. It's a bit fragile relying on the r4 value being zero, it > would be easy to accidentally reuse r4. Though it actually wouldn't > matter as long as r4 never has "regshere" in it. > Yep, r4 will eventually get reloaded right below, so unless reuses it as a scratch register, shouldn't matter > In fact we could store any random value there, it just needs to not be > the exception marker. eg. we could just stick the SRR0 value in there, > that should never alias with "regshere". > > But I think maybe we're over thinking it, the cost of the li is pretty > minimal compared to everything else going on here, and this is only on > the return to kernel case, which is arguably not a super hot path. Agreed Cheers Balbir Singh.