linux-kernel - Re: [PATCH] mm: don't warn about allocations which stall for too long

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20171102105513.50f8e29e@gandalf.local.home>
Date:   Thu, 2 Nov 2017 10:55:13 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Cong Wang <xiyou.wangcong@...il.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Mel Gorman <mgorman@...e.de>, Michal Hocko <mhocko@...nel.org>,
        Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "yuwang.yuwang" <yuwang.yuwang@...baba-inc.com>
Subject: Re: [PATCH] mm: don't warn about allocations which stall for too
 long

On Thu, 2 Nov 2017 17:53:13 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@...il.com> wrote:

> On (10/31/17 15:32), Steven Rostedt wrote:
> [..]
> > (new globals)
> > static DEFINE_SPIN_LOCK(console_owner_lock);
> > static struct task_struct console_owner;
> > static bool waiter;
> > 
> > console_unlock() {
> > 
> > [ Assumes this part can not preempt ]
> >
> > 	spin_lock(console_owner_lock);
> > 	console_owner = current;
> > 	spin_unlock(console_owner_lock);  
> 
>  + disables IRQs?

Yes, this was pseudo code, just to get an idea out. I'll have a patch
soon that will include all the nasty details.

> 
> > 	for each message
> > 		write message out to console
> > 
> > 		if (READ_ONCE(waiter))
> > 			break;
> > 
> > 	spin_lock(console_owner_lock);
> > 	console_owner = NULL;
> > 	spin_unlock(console_owner_lock);
> > 
> > [ preemption possible ]  
> 
> otherwise
> 
>      printk()
>       if (console_trylock())
>         console_unlock()
>          preempt_disable()
>           spin_lock(console_owner_lock);
>           console_owner = current;
>           spin_unlock(console_owner_lock);
>           .......
>           spin_lock(console_owner_lock);
> IRQ
>     printk()
>      console_trylock() // fails so we go to busy-loop part
>       spin_lock(console_owner_lock);       << deadlock

Yeah, I do disable interrupts. The pseudo code was just a way to
quickly convey the idea. I said "spin_lock" where I could have just
said "lock".

> 
> 
> even if we would replace spin_lock(console_owner_lock) with IRQ
> spin_lock, we still would need to protect against IRQs on the very
> same CPU. right? IOW, we need to store smp_processor_id() of a CPU
> currently doing console_unlock() and check it in vprintk_emit()?
> and we need to protect the entire console_unlock() function. not
> just the printing loop, otherwise the IRQ CPU will spin forever
> waiting for itself to up() the console_sem.

Yes and it will.

> 
> this somehow reminds me of "static unsigned int logbuf_cpu", which
> we used to have in vprintk_emit() and were happy to remove it...
> 
> 
> the whole "console_unlock() is non-preemptible" can bite, I'm
> afraid. it's not always printk()->console_unlock(), sometimes
> it's console_lock()->console_unlock() that has to flush the
> logbuf.
> 
> CPU0					CPU1  ~  CPU99
> console_lock();
> 					printk(); ... printk();
> console_unlock()
>  preempt_disable();
>   for (;;)
>     call_console_drivers();
>     <<lockup>>
> 
> 
> this pattern is not so unusual. _especially_ in the existing scheme
> of things.
> 
> not to mention the problem of "the last printk()", which will take
> over and do the flush.
> 
> CPU0					CPU1  ~  CPU99
> console_lock();
> 					printk(); ... printk();
> console_unlock();
> 					    IRQ on CPU2
> 					     printk()
> 					      // take over console_sem
> 					      console_unlock()
> 
> and so on.
> seems that there will be lots of if-s.


Let's wait for the patch and talk more after I post it.

-- Steve