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=-8.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED, USER_AGENT_MUTT 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 0A58DECE565 for ; Tue, 18 Sep 2018 09:17:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AE9C12146D for ; Tue, 18 Sep 2018 09:17:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AE9C12146D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linuxfoundation.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729242AbeIROss (ORCPT ); Tue, 18 Sep 2018 10:48:48 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:40652 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726759AbeIROss (ORCPT ); Tue, 18 Sep 2018 10:48:48 -0400 Received: from localhost (unknown [147.67.4.98]) by mail.linuxfoundation.org (Postfix) with ESMTPSA id 8DBD2D63; Tue, 18 Sep 2018 09:17:04 +0000 (UTC) Date: Tue, 18 Sep 2018 11:17:02 +0200 From: Greg Kroah-Hartman To: Sai Prakash Ranjan Cc: Jiri Slaby , Matthias Kaehlcke , Douglas Anderson , Evan Green , Manoj Gupta , Stephen Boyd , Nick Desaulniers , linux-kernel@vger.kernel.org Subject: Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global Message-ID: <20180918091702.GA10846@kroah.com> References: <20180917213304.44476-1-mka@chromium.org> <20180918072037.GA4442@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 18, 2018 at 02:35:02PM +0530, Sai Prakash Ranjan wrote: > On 9/18/2018 12:50 PM, Greg Kroah-Hartman wrote: > > On Tue, Sep 18, 2018 at 12:28:39PM +0530, Sai Prakash Ranjan wrote: > > > On 9/18/2018 11:41 AM, Jiri Slaby wrote: > > > > On 09/17/2018, 11:33 PM, Matthias Kaehlcke wrote: > > > > > sysrq_handle_crash() dereferences a NULL pointer on purpose to force > > > > > an exception, the local variable 'killer' is assigned to NULL and > > > > > dereferenced later. Clang detects the NULL pointer dereference at compile > > > > > time and emits a BRK instruction (on arm64) instead of the expected NULL > > > > > pointer exception. Change 'killer' to a global variable (and rename it > > > > > to 'sysrq_killer' to avoid possible clashes) to prevent Clang from > > > > > detecting the condition. By default global variables are initialized > > > > > with zero/NULL in C, therefore an explicit initialization is not needed. > > > > > > > > > > Reported-by: Sai Prakash Ranjan > > > > > Suggested-by: Evan Green > > > > > Signed-off-by: Matthias Kaehlcke > > > > > --- > > > > > drivers/tty/sysrq.c | 6 +++--- > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > > > > > index 06ed20dd01ba..49fa8e758690 100644 > > > > > --- a/drivers/tty/sysrq.c > > > > > +++ b/drivers/tty/sysrq.c > > > > > @@ -132,10 +132,10 @@ static struct sysrq_key_op sysrq_unraw_op = { > > > > > #define sysrq_unraw_op (*(struct sysrq_key_op *)NULL) > > > > > #endif /* CONFIG_VT */ > > > > > +char *sysrq_killer; > > > > > + > > > > > static void sysrq_handle_crash(int key) > > > > > { > > > > > - char *killer = NULL; > > > > > - > > > > > /* we need to release the RCU read lock here, > > > > > * otherwise we get an annoying > > > > > * 'BUG: sleeping function called from invalid context' > > > > > @@ -144,7 +144,7 @@ static void sysrq_handle_crash(int key) > > > > > rcu_read_unlock(); > > > > > panic_on_oops = 1; /* force panic */ > > > > > wmb(); > > > > > - *killer = 1; > > > > > + *sysrq_killer = 1; > > > > > > > > Just because a static analyzer is wrong? Oh wait, even compiler is > > > > wrong. At least make it a static global. Or what about OPTIMIZER_HIDE_VAR? > > > > > > > > > > static global does not work, clang still inserts brk. As for > > > OPTIMIZE_HIDE_VAR, it seems to work. > > > But, I dont think it is defined for clang in which case it defaults to using > > > barrier(). There is already one wmb(), so will it be right? > > > > Ick, why is this needed at all? Why are we trying to "roll our own > > panic" in this code? > > > > Hi Greg, do you mean like why there is a killer var at all or why this > change is required? I understand you are using a compiler that thinks it wants to protect yourself from your code and tries to "fix" it for you. That's fine, and is up to the compiler writers (personally that seems not a good idea.) My question is why we just don't call panic() here instead of trying to duplicate the logic of that function here. Why is that happening? thanks, greg k-h