* [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
@ 2018-09-17 21:33 Matthias Kaehlcke
2018-09-18 6:09 ` Sai Prakash Ranjan
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Matthias Kaehlcke @ 2018-09-17 21:33 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-kernel, Evan Green, Sai Prakash Ranjan, Douglas Anderson,
Stephen Boyd, Manoj Gupta, Nick Desaulniers, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-17 21:33 [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global Matthias Kaehlcke
@ 2018-09-18 6:09 ` Sai Prakash Ranjan
2018-09-18 6:11 ` Jiri Slaby
2018-09-18 11:46 ` David Laight
2 siblings, 0 replies; 11+ messages in thread
From: Sai Prakash Ranjan @ 2018-09-18 6:09 UTC (permalink / raw)
To: Matthias Kaehlcke, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-kernel, Evan Green, Douglas Anderson, Stephen Boyd,
Manoj Gupta, Nick Desaulniers
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-17 21:33 [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global Matthias Kaehlcke
2018-09-18 6:09 ` Sai Prakash Ranjan
@ 2018-09-18 6:11 ` Jiri Slaby
2018-09-18 6:58 ` Sai Prakash Ranjan
2018-09-18 17:24 ` Matthias Kaehlcke
2018-09-18 11:46 ` David Laight
2 siblings, 2 replies; 11+ messages in thread
From: Jiri Slaby @ 2018-09-18 6:11 UTC (permalink / raw)
To: Matthias Kaehlcke, Greg Kroah-Hartman
Cc: Douglas Anderson, Evan Green, Manoj Gupta, Stephen Boyd,
Sai Prakash Ranjan, Nick Desaulniers, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-18 6:11 ` Jiri Slaby
@ 2018-09-18 6:58 ` Sai Prakash Ranjan
2018-09-18 7:20 ` Greg Kroah-Hartman
2018-09-18 17:24 ` Matthias Kaehlcke
1 sibling, 1 reply; 11+ messages in thread
From: Sai Prakash Ranjan @ 2018-09-18 6:58 UTC (permalink / raw)
To: Jiri Slaby, Matthias Kaehlcke, Greg Kroah-Hartman
Cc: Douglas Anderson, Evan Green, Manoj Gupta, Stephen Boyd,
Nick Desaulniers, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-18 6:58 ` Sai Prakash Ranjan
@ 2018-09-18 7:20 ` Greg Kroah-Hartman
2018-09-18 9:05 ` Sai Prakash Ranjan
0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2018-09-18 7:20 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: Jiri Slaby, Matthias Kaehlcke, Douglas Anderson, Evan Green,
Manoj Gupta, Stephen Boyd, Nick Desaulniers, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-18 7:20 ` Greg Kroah-Hartman
@ 2018-09-18 9:05 ` Sai Prakash Ranjan
2018-09-18 9:17 ` Greg Kroah-Hartman
0 siblings, 1 reply; 11+ messages in thread
From: Sai Prakash Ranjan @ 2018-09-18 9:05 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, Matthias Kaehlcke, Douglas Anderson, Evan Green,
Manoj Gupta, Stephen Boyd, Nick Desaulniers, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-18 9:05 ` Sai Prakash Ranjan
@ 2018-09-18 9:17 ` Greg Kroah-Hartman
2018-09-18 9:53 ` Sai Prakash Ranjan
[not found] ` <32bf1760-4967-a37a-6a17-3f7d5f6e071e@suse.cz>
0 siblings, 2 replies; 11+ messages in thread
From: Greg Kroah-Hartman @ 2018-09-18 9:17 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: Jiri Slaby, Matthias Kaehlcke, Douglas Anderson, Evan Green,
Manoj Gupta, Stephen Boyd, Nick Desaulniers, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-18 9:17 ` Greg Kroah-Hartman
@ 2018-09-18 9:53 ` Sai Prakash Ranjan
[not found] ` <32bf1760-4967-a37a-6a17-3f7d5f6e071e@suse.cz>
1 sibling, 0 replies; 11+ messages in thread
From: Sai Prakash Ranjan @ 2018-09-18 9:53 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, Matthias Kaehlcke, Douglas Anderson, Evan Green,
Manoj Gupta, Stephen Boyd, Nick Desaulniers, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-17 21:33 [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global Matthias Kaehlcke
2018-09-18 6:09 ` Sai Prakash Ranjan
2018-09-18 6:11 ` Jiri Slaby
@ 2018-09-18 11:46 ` David Laight
2 siblings, 0 replies; 11+ messages in thread
From: David Laight @ 2018-09-18 11:46 UTC (permalink / raw)
To: 'Matthias Kaehlcke', Greg Kroah-Hartman, Jiri Slaby
Cc: linux-kernel, Evan Green, Sai Prakash Ranjan, Douglas Anderson,
Stephen Boyd, Manoj Gupta, Nick Desaulniers
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)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
2018-09-18 6:11 ` Jiri Slaby
2018-09-18 6:58 ` Sai Prakash Ranjan
@ 2018-09-18 17:24 ` Matthias Kaehlcke
1 sibling, 0 replies; 11+ messages in thread
From: Matthias Kaehlcke @ 2018-09-18 17:24 UTC (permalink / raw)
To: Jiri Slaby
Cc: Greg Kroah-Hartman, Douglas Anderson, Evan Green, Manoj Gupta,
Stephen Boyd, Sai Prakash Ranjan, Nick Desaulniers, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global
[not found] ` <32bf1760-4967-a37a-6a17-3f7d5f6e071e@suse.cz>
@ 2018-09-18 17:32 ` Matthias Kaehlcke
0 siblings, 0 replies; 11+ messages in thread
From: Matthias Kaehlcke @ 2018-09-18 17:32 UTC (permalink / raw)
To: Jiri Slaby
Cc: Greg Kroah-Hartman, Sai Prakash Ranjan, Douglas Anderson,
Evan Green, Manoj Gupta, Stephen Boyd, Nick Desaulniers,
linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-09-18 17:32 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-17 21:33 [PATCH] tty/sysrq: Make local variable 'killer' in sysrq_handle_crash() global Matthias Kaehlcke
2018-09-18 6:09 ` Sai Prakash Ranjan
2018-09-18 6:11 ` Jiri Slaby
2018-09-18 6:58 ` Sai Prakash Ranjan
2018-09-18 7:20 ` Greg Kroah-Hartman
2018-09-18 9:05 ` Sai Prakash Ranjan
2018-09-18 9:17 ` Greg Kroah-Hartman
2018-09-18 9:53 ` Sai Prakash Ranjan
[not found] ` <32bf1760-4967-a37a-6a17-3f7d5f6e071e@suse.cz>
2018-09-18 17:32 ` Matthias Kaehlcke
2018-09-18 17:24 ` Matthias Kaehlcke
2018-09-18 11:46 ` David Laight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).