Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global

[Greg Kroah-Hartman <gregkh@linuxfoundation.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 <saiprakash.ranjan@codeaurora.org>
> > > > > Suggested-by: Evan Green <evgreen@chromium.org>
> > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > ---
> > > > >    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