linux-kernel - Re: linux.git: printk() problem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Oct 2016 13:06:17 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Joe Perches <joe@...ches.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Petr Mladek <pmladek@...e.cz>, Tejun Heo <tj@...nel.org>,
        Calvin Owens <calvinowens@...com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: linux.git: printk() problem

On (10/24/16 19:22), Linus Torvalds wrote:
> On Mon, Oct 24, 2016 at 7:06 PM, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
> > On Mon, Oct 24, 2016 at 6:55 PM, Sergey Senozhatsky
> > <sergey.senozhatsky.work@...il.com> wrote:
> >>
> >> I think cont_flush() should grab the logbuf_lock lock, because
> >> it does log_store() and touches the cont.len. so something like
> >> this perhaps
> >
> > Absolutely. Good catch.
> 
> Actually, you can't do it the way you did (inside cont_flush), because
> "cont_flush()" is already called with logbuf_lock held in most cases
> (see "cont_add()").

right. my bad, realized too late.

> So it's really just the timer function that needs to take the
> logbuf_lock before it calls cont_flush().

yes.

> So here's a new version. How does this look to you?

ok, looks much better.

there are several things that I want to mention here, just to make
sure we don't miss anything (just my 5 cents).

1) the way we dumpstack on x86 (at least on x86) is a spaghetti of
printk() and pr_cont() calls. for instance, arch/x86/kernel/dumpstack_64.c
show_regs() does pr_cont() to print out the registers, while the stack and
backtrace are printed with printk(). so, I assume, the backtrace now will
look a bit upside-down, because cont lines are printed with the delay.
correct?

2) flush on oops. not that panic printk is deadlock proof (not at all)
   but:
     a) rather unlikely, but what if BUG_ON() or panic() happens
      under lock_timer_base()?
     b) what if timer event never happens? (we are in panic, who knows)

  so how about skipping mod_timer in deferred_cont_flush() and just
  cont_flush() when we are in oops? here is probably one more thing we
  need to "fix" first. oops_in_progress is unreliable. x86 oops_end()
  does bust_spinlocks(0) before it calls panic(). panic() increments
  oops_in_progress but decrements it back to 0 (bust_spinlocks(0)) before
  it does console_flush_on_panic(). so there is (almost) no way
  console_flush_on_panic() can see oops_in_progress != 0.

diff --git a/kernel/panic.c b/kernel/panic.c
index e6480e2..8e17540 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -228,7 +228,6 @@ void panic(const char *fmt, ...)
        if (_crash_kexec_post_notifiers)
                __crash_kexec(NULL);

-       bust_spinlocks(0);

        /*
         * We may have ended up stopping the CPU holding the lock (in
@@ -240,6 +239,7 @@ void panic(const char *fmt, ...)
         */
        debug_locks_off();
        console_flush_on_panic();
+       bust_spinlocks(0);

        if (!panic_blink)
                panic_blink = no_blink;
---

	-ss