linux-kernel - Re: [PATCH v5 0/2] printk: Console owner and waiter logic cleanup

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]

Message-ID: <20180116101355.iy7q3pqxzzlpdiht@pathway.suse.cz>
Date:   Tue, 16 Jan 2018 11:13:55 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Tejun Heo <tj@...nel.org>, akpm@...ux-foundation.org,
        linux-mm@...ck.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>,
        Vlastimil Babka <vbabka@...e.cz>,
        Peter Zijlstra <peterz@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        rostedt@...e.goodmis.org, Byungchul Park <byungchul.park@....com>,
        Pavel Machek <pavel@....cz>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 0/2] printk: Console owner and waiter logic cleanup

On Tue 2018-01-16 11:23:49, Sergey Senozhatsky wrote:
> On (01/15/18 15:45), Petr Mladek wrote:
> > > I think adding the preempt_disable() would fix printk() but let non
> > > printk console_unlock() still preempt.
> > 
> > I would personally remove cond_resched() from console_unlock()
> > completely.
> 
> hmm, not so sure. I think it's there for !PREEMPT systems which have
> to print a lot of messages. the case I'm speaking about in particular
> is when we register a CON_PRINTBUFFER console and need to console_unlock()
> (flush) all of the messages we currently have in the logbuf. we better
> have that cond_resched() there, I think.

Good point. I agree that we should keep the cond_resched() there
at least for now.


> > Sleeping in console_unlock() increases the chance that more messages
> > would need to be handled. And more importantly it reduces the chance
> > of a successful handover.
> > 
> > As a result, the caller might spend there very long time, it might
> > be getting increasingly far behind. There is higher risk of lost
> > messages. Also the eventual taker might have too much to proceed
> > in preemption disabled context.
> 
> yes.
> 
> > Removing cond_resched() is in sync with printk() priorities.
> 
> hmm, not sure. we have sleeping console_lock()->console_unlock() path
> for PREEMPT kernels, that cond_resched() makes the !PREEMPT kernels to
> have the same sleeping console_lock()->console_unlock().
> 
> printk()->console_unlock() seems to be a pretty independent thing,
> unfortunately (!), yet sleeping console_lock()->console_unlock()
> messes up with it a lot.

IMHO, the problem here is that console_lock is used to synchronize
too many things. It would be great to separate printk() duties
into a separate lock in the long term.

Anyway, I see it the following way. Most console_lock() callers
do the following things:

void foo()
{
	console_lock()
	foo_specific_work();
	console_unlock();
}

where console_unlock() flushes the printk buffer before actually
releasing the lock.

IMHO, it would make sense if flushing the printk buffer behaves
the same when called either from printk() or from any other path.
I mean that it should be aggressive and allow an effective
hand off.

It should be safe as long as foo_specific_work() does not take
too much time.

>From other side. The cond_resched() in console_unlock() should
be obsoleted by the hand-shake code.


> > The highest one is to get the messages out.
> > 
> > Finally, removing cond_resched() should make the behavior more
> > predictable (never preempted)
> 
> but we are always preempted in PREEMPT kernels when the current
> console_sem owner acquired the lock via console_lock(), not via
> console_trylock(). cond_resched() does the same, but for !PREEMPT.

I agree that the situation is more complicated for cond_resched()
called after console_lock(). I do not resist on removing it now.

Just one more thing. The time axe looks like:

+ cond_resched added into console_unlock in v4.5-rc1, Jan 15, 2016
     (commit 8d91f8b15361dfb438ab6)

+ preemtion enabled in printk in, v4.6-rc1, Mar 17, 2016
     (commit 6b97a20d3a7909daa0662)

They both were obvious solutions that helped to reduce the risk
of soft-lockups. The first one handled evidently safe scenarios.
The second one was even more aggressive. I would say that
they both were more or less add-hoc solutions that did not
take into account the other side effects (delaying output,
even loosing messages).

I would not say that one is a diametric difference between them.
Therefore if we remove one for a reason, we should think about
reverting the other as well. But again. I am fine if we remove
only one now.

Does this make any sense?

Best Regard,
Petr