[PATCH] 3.4.28-rt40 tglx fix imx.c spinlock

[PATCH] 3.4.28-rt40 tglx fix imx.c spinlock

[Tim Sander]

Hi Thomas
> > > > Just another update on this stuff. I noticed that there has been a
> > > > patch
> > > > added* with spinlocks to the imx.c serial driver. I reverted this
> > > > patch
> > > > and now my serialfuz programm "only" kills the serial port with a not
> > > > so
> > > > nice oom condition. But at least it does not show the runaway
> > > > interrupt
> > > > problem.
> > > 
> > > That does not make any sense, really. The runnaway interrupt issue is
> > > completely unrelated to this commit.
> > 
> > One thing that puzzles me is the fact that it does! So removing the
> > mentioned spinlock patch creates the same result as using your fixed
> > patch.
> > 
> > As the other problem with the driver i posted shows the same interrupt
> > runaway symptom as this i have a very bad gut feeling about this...
> > Or probably its not a runnaway interrupt problem but a double
> > spin_lock_irqsave.
> 
> Well, you claimed that it is a runnaway interrupt. So much for the
> theory :)
Well i sleept over that one. The symptoms in both cases seem to be identical:
system locked up,interrupt pending, no output at all. 

But there are some differences. Concerning the issue described in this mail:
http://www.spinics.net/lists/linux-rt-users/msg09303.html
After the complete lockup of the system i could get into a working system
 by switching of the interrupt source of the respective interrupt. After that the
interrupt readings in /proc/interrupts where insanely high. So definetly a runnaway
 interrupt issue. But this issue seems to be not related to preempt rt. They where
just so similar on the first glance that i thought they where related.

I could not get the system back running in switching of the serial interrupt.
Concerning the serial problem i consider the problem solved by the patch below. 
Thanks!

A patch for 3.6-rt will be sent in another reply. As it does not apply cleanly.

> The issue with the recursive locking is not an RT issue. You can
> observe the problem on mainline, when you enable PROVE_LOCKING and
> issue a sysrq or oopsing in a section which holds the port.lock
> already.
> 
> The reason why you can't observe it with PREEMPT_RT_FULL=n and
> PROVE_LOCKING=n is, that the spinlocks on UP machines are compiled
> away, so recursive locking does not lead to an observable dead lock.
Ah, yes that perfectly explains why i have not seen this error in non preempt-rt
mode.

> > Also the fact that my serialfuz program i posted is able to give the
> > system an Out Of Memory condition is strange. I mean throwing random
> > chars at a getty should'nt exhaust memory so fast.
> 
> That's true, but w/o seing the OOM output I can't tell what's
> exhausting the memory.
When fuzzing the serial port one probably should switch of sysreq. It seems 
as if there is a break send somehow and then it selects the OOM option.
So when switching of MAGIC_SYSRQ the OOMs are gone. So its a non issue.

Best regards
Tim


Signed-off-by: Tim Sander <tim@krieglstein.org>

---
 drivers/tty/serial/imx.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 0de7ed7..f1f5c4e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -47,6 +47,7 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/kdb.h>

 #include <asm/io.h>
 #include <asm/irq.h>
@@ -1225,8 +1226,12 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
    struct imx_port_ucrs old_ucr;
    unsigned int ucr1;
    unsigned long flags;
+   int locked = 1;

-   spin_lock_irqsave(&sport->port.lock, flags);
+   if (sport->port.sysrq || oops_in_progress || in_kdb_printk())
+       locked = spin_trylock_irqsave(&sport->port.lock, flags);
+   else
+       spin_lock_irqsave(&sport->port.lock, flags);

    /*
     *  First, save UCR1/2/3 and then disable interrupts
@@ -1253,7 +1258,8 @@ imx_console_write(struct console *co, const char *s, unsigned int count)

    imx_port_ucrs_restore(&sport->port, &old_ucr);

-   spin_unlock_irqrestore(&sport->port.lock, flags);
+   if (locked)
+       spin_unlock_irqrestore(&sport->port.lock, flags);
 }

 /*
-- 
1.7.9.5

Re: [PATCH] 3.4.28-rt40 tglx fix imx.c spinlock

[Thomas Gleixner]

On Thu, 14 Feb 2013, Tim Sander wrote:
> > That's true, but w/o seing the OOM output I can't tell what's
> > exhausting the memory.
> When fuzzing the serial port one probably should switch of sysreq. It seems 
> as if there is a break send somehow and then it selects the OOM option.
> So when switching of MAGIC_SYSRQ the OOMs are gone. So its a non issue.

Amazing that you get the break+oom combo out of that fuzzer!

Re: [PATCH] 3.4.28-rt40 tglx fix imx.c spinlock

[Paul Gortmaker]

[Adding dave jones to CC]

On Thu, Feb 14, 2013 at 5:35 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 14 Feb 2013, Tim Sander wrote:
>> > That's true, but w/o seing the OOM output I can't tell what's
>> > exhausting the memory.
>> When fuzzing the serial port one probably should switch of sysreq. It seems
>> as if there is a break send somehow and then it selects the OOM option.
>> So when switching of MAGIC_SYSRQ the OOMs are gone. So its a non issue.
>
> Amazing that you get the break+oom combo out of that fuzzer!

Doing a basic "git whatchanged" and searching for "trinity" is rather
impressive, regardless of the kernel version and/or where "rogue states"
may currently be at with their "program"....   Kudos to davej for that.

P.
--

[PATHC] 3.6 spinlock fix

[Tim Sander]

Hi
> On Thu, Feb 14, 2013 at 5:35 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 14 Feb 2013, Tim Sander wrote:
> >> > That's true, but w/o seing the OOM output I can't tell what's
> >> > exhausting the memory.
> >> 
> >> When fuzzing the serial port one probably should switch of sysreq. It
> >> seems
> >> as if there is a break send somehow and then it selects the OOM option.
> >> So when switching of MAGIC_SYSRQ the OOMs are gone. So its a non issue.
> > 
> > Amazing that you get the break+oom combo out of that fuzzer!
That fuzzer is running at 57600Hz while the serial port of the fuzzed device is
running twice that rate. The break condition seems to be easy hit by the fuzzer
 i've sent in a previous mail. 
> Doing a basic "git whatchanged" and searching for "trinity" is rather
> impressive, regardless of the kernel version and/or where "rogue states"
> may currently be at with their "program"....   Kudos to davej for that.
Mh, but thats not trinity! Havn't tried that but well fuzzing at a different 
serial rate than the receiver might be a good idea even if it sounds pretty stupid.

Attached is the patch for the 3.6.9-rt kernel (but i think this should also apply 
to the "normal" 3.6 i guess).  But as Greg already took care of this patch i guess 
that only for convinience. Also it seems as if the patch sent to Greg is missing the
#include <linux/kdb.h>?

Best regards
Tim

    tglx: fix imx.c spinlock

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e309e8b..39820ea 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -48,6 +48,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/kdb.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -1213,8 +1214,12 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
        struct imx_port_ucrs old_ucr;
        unsigned int ucr1;
        unsigned long flags;
+       int locked = 1;
 
-       spin_lock_irqsave(&sport->port.lock, flags);
+       if (sport->port.sysrq || oops_in_progress || in_kdb_printk())
+               locked = spin_trylock_irqsave(&sport->port.lock, flags);
+       else
+               spin_lock_irqsave(&sport->port.lock, flags);
 
        /*
         *      First, save UCR1/2/3 and then disable interrupts
@@ -1241,7 +1246,8 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
 
        imx_port_ucrs_restore(&sport->port, &old_ucr);
 
-       spin_unlock_irqrestore(&sport->port.lock, flags);
+       if (locked)
+               spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
 /*

Re: [PATHC] 3.6 spinlock fix

[Thomas Gleixner]

On Fri, 15 Feb 2013, Tim Sander wrote:
> > On Thu, Feb 14, 2013 at 5:35 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > > On Thu, 14 Feb 2013, Tim Sander wrote:
> > >> > That's true, but w/o seing the OOM output I can't tell what's
> > >> > exhausting the memory.
> > >> 
> > >> When fuzzing the serial port one probably should switch of sysreq. It
> > >> seems
> > >> as if there is a break send somehow and then it selects the OOM option.
> > >> So when switching of MAGIC_SYSRQ the OOMs are gone. So its a non issue.
> > > 
> > > Amazing that you get the break+oom combo out of that fuzzer!
> That fuzzer is running at 57600Hz while the serial port of the fuzzed device is
> running twice that rate. The break condition seems to be easy hit by the fuzzer
>  i've sent in a previous mail. 
> > Doing a basic "git whatchanged" and searching for "trinity" is rather
> > impressive, regardless of the kernel version and/or where "rogue states"
> > may currently be at with their "program"....   Kudos to davej for that.
> Mh, but thats not trinity! Havn't tried that but well fuzzing at a different 
> serial rate than the receiver might be a good idea even if it sounds pretty stupid.
> 
> Attached is the patch for the 3.6.9-rt kernel (but i think this should also apply 
> to the "normal" 3.6 i guess).  But as Greg already took care of this patch i guess 
> that only for convinience. Also it seems as if the patch sent to Greg is missing the
> #include <linux/kdb.h>?

Errm, no. The in_kdb_printk() is an RT specific issue, so I left it
out for the mainline fix.

Thanks,

	tglx