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.ran...@codeaurora.org>
> > > Suggested-by: Evan Green <evgr...@chromium.org>
> > > Signed-off-by: Matthias Kaehlcke <m...@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
