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

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

[Matthias Kaehlcke]

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;
 }
 static struct sysrq_key_op sysrq_crash_op = {
 	.handler	= sysrq_handle_crash,
-- 
2.19.0.397.gdd90340f6a-goog

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

[Sai Prakash Ranjan]

On 9/18/2018 3:03 AM, 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;
>   }
>   static struct sysrq_key_op sysrq_crash_op = {
>   	.handler	= sysrq_handle_crash,
> 

Tested-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Jiri Slaby]

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?

thanks,
-- 
js
suse labs

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

[Sai Prakash Ranjan]

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?

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Greg Kroah-Hartman]

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?

thanks,

greg k-h

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

[Sai Prakash Ranjan]

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?

Below are some additional info about this issue:

CLANG generated:

   ffffff800842bc1c <sysrq_handle_crash>:
   ffffff800842bc1c:       a9bf7bfd        stp     x29, x30, [sp,#-16]!
   ffffff800842bc20:       910003fd        mov     x29, sp
   ffffff800842bc24:       97f1a34e        bl      ffffff800809495c 
<_mcount>
   ffffff800842bc28:       97f38876        bl      ffffff800810de00 
<__rcu_read_unlock>
   ffffff800842bc2c:       f0006888        adrp    x8, ffffff800913e000 
<reset_devices>
   ffffff800842bc30:       320003e9        orr     w9, wzr, #0x1
   ffffff800842bc34:       b9091109        str     w9, [x8,#2320]
   ffffff800842bc38:       d5033e9f        dsb     st
   ffffff800842bc3c:       d4200020        brk     #0x1          <----


GCC generated:

ffffff8008452f60 <sysrq_handle_crash>:
ffffff8008452f60:       a9bf7bfd        stp     x29, x30, [sp,#-16]!
ffffff8008452f64:       910003fd        mov     x29, sp
ffffff8008452f68:       aa1e03e0        mov     x0, x30
ffffff8008452f6c:       97f1078c        bl      ffffff8008094d9c <_mcount>
ffffff8008452f70:       97f2f2cc        bl      ffffff800810faa0 
<__rcu_read_unlock>
ffffff8008452f74:       900067e1        adrp    x1, ffffff800914e000 
<reset_devices>
ffffff8008452f78:       52800020        mov     w0, #0x1 
        // #1
ffffff8008452f7c:       b9096820        str     w0, [x1,#2408]
ffffff8008452f80:       d5033e9f        dsb     st
ffffff8008452f84:       d2800001        mov     x1, #0x0 
        // #0
ffffff8008452f88:       39000020        strb    w0, [x1]
ffffff8008452f8c:       a8c17bfd        ldp     x29, x30, [sp],#16
ffffff8008452f90:       d65f03c0        ret

Trigger sysrq crash:

localhost ~ # echo c > /proc/sysrq-trigger
  [   78.320401] sysrq: SysRq : Trigger a crash
  [   78.324752] Unexpected kernel BRK exception at EL1
  [   78.329691] Unhandled debug exception: ptrace BRK handler 
(0xf2000001) at 0x000000000e25c368
  [   78.338384] Internal error: : 0 [#1] PREEMPT SMP

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Greg Kroah-Hartman]

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

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

[Sai Prakash Ranjan]

On 9/18/2018 2:47 PM, Greg Kroah-Hartman wrote:
> 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?
> 

It seems fine to call panic() here. Dont no why they chose to have a 
null pointer dereference.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Matthias Kaehlcke]

On Tue, Sep 18, 2018 at 11:51:55AM +0200, Jiri Slaby wrote:
> On 09/18/2018, 11:17 AM, Greg Kroah-Hartman wrote:
> > 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?
> 
> Historically (before d6580a9f1), we had crash_kexec called() from here.
> To be honest, I see no reason not to call panic() or crash_kexec() again
> if there is any issue with the former (I cannot foresee one).

It seems in the end we don't need to change anything for Clang. With
-fno-delete-null-pointer-checks the NULL pointer access is not
'optimized' away, support for this option landed recently in upstream
LLVM, and it is already set in the kernel.

Using panic() instead of roll-your-own-crash still seems a desirable
improvement though.

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

[Matthias Kaehlcke]

On Tue, Sep 18, 2018 at 08:11:33AM +0200, 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?

With a static global the compiler still can know that the value is
constant.

Anyway, our compiler folks raised concerns that the variable could
still be identified as constant with Link Time Optimization (LTO)
enabled, so this patch isn't a good solution.

On the positive side the compiler engs don't expect the NULL pointer
access being optimized away with -fno-delete-null-pointer-checks
enabled, which recently landed in upstream LLVM.

I confirmed that with -fno-delete-null-pointer-checks Clang does not
emit 'brk' in this case and the kernel crashes with a NULL pointer
exception when this code path is triggered.

Seems we can forget about this patch, though we might still want to
replace the forced crash with a panic() as mentioned in this thread.

Thanks

m.

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

[David Laight]

From: Matthias Kaehlcke
> Sent: 17 September 2018 22:33
> 
> 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.

You need an explicit initialiser in order to make it global data
rather than a common section.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)