linux-kernel - Re: [PATCH v4] printk: Add console owner and waiter logic to load balance console writes

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:   Thu, 14 Dec 2017 14:51:02 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.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>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        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>
Subject: Re: [PATCH v4] printk: Add console owner and waiter logic to load
 balance console writes

On Fri 2017-11-24 16:58:16, Petr Mladek wrote:
> On Fri 2017-11-24 16:54:16, Petr Mladek wrote:
> > On Wed 2017-11-08 10:27:23, Steven Rostedt wrote:
> > > If there is a waiter, then it breaks out of the loop, clears the waiter
> > > flag (because that will release the waiter from its spin), and exits.
> > > Note, it does *not* release the console semaphore. Because it is a
> > > semaphore, there is no owner.
> > 
> > > Index: linux-trace.git/kernel/printk/printk.c
> > > ===================================================================
> > > --- linux-trace.git.orig/kernel/printk/printk.c
> > > +++ linux-trace.git/kernel/printk/printk.c
> > > @@ -86,8 +86,15 @@ EXPORT_SYMBOL_GPL(console_drivers);
> > >  static struct lockdep_map console_lock_dep_map = {
> > >  	.name = "console_lock"
> > >  };
> > > +static struct lockdep_map console_owner_dep_map = {
> > > +	.name = "console_owner"
> > > +};
> > >  #endif
> > >  
> > > +static DEFINE_RAW_SPINLOCK(console_owner_lock);
> > > +static struct task_struct *console_owner;
> > > +static bool console_waiter;
> > > +
> > >  enum devkmsg_log_bits {
> > >  	__DEVKMSG_LOG_BIT_ON = 0,
> > >  	__DEVKMSG_LOG_BIT_OFF,
> > > @@ -1753,8 +1760,56 @@ asmlinkage int vprintk_emit(int facility
> > >  		 * semaphore.  The release will print out buffers and wake up
> > >  		 * /dev/kmsg and syslog() users.
> > >  		 */
> > > -		if (console_trylock())
> > > +		if (console_trylock()) {
> > >  			console_unlock();
> > > +		} else {
> > > +			struct task_struct *owner = NULL;
> > > +			bool waiter;
> > > +			bool spin = false;
> > > +
> > > +			printk_safe_enter_irqsave(flags);
> > > +
> > > +			raw_spin_lock(&console_owner_lock);
> > > +			owner = READ_ONCE(console_owner);
> > > +			waiter = READ_ONCE(console_waiter);
> > > +			if (!waiter && owner && owner != current) {
> > > +				WRITE_ONCE(console_waiter, true);
> > > +				spin = true;
> > > +			}
> > > +			raw_spin_unlock(&console_owner_lock);
> > > +
> > > +			/*
> > > +			 * If there is an active printk() writing to the
> > > +			 * consoles, instead of having it write our data too,
> > > +			 * see if we can offload that load from the active
> > > +			 * printer, and do some printing ourselves.
> > > +			 * Go into a spin only if there isn't already a waiter
> > > +			 * spinning, and there is an active printer, and
> > > +			 * that active printer isn't us (recursive printk?).
> > > +			 */
> > > +			if (spin) {
> > > +				/* We spin waiting for the owner to release us */
> > > +				spin_acquire(&console_owner_dep_map, 0, 0, _THIS_IP_);
> > > +				/* Owner will clear console_waiter on hand off */
> > > +				while (READ_ONCE(console_waiter))
> > > +					cpu_relax();
> > > +
> > > +				spin_release(&console_owner_dep_map, 1, _THIS_IP_);
> > > +				printk_safe_exit_irqrestore(flags);
> > > +
> > > +				/*
> > > +				 * The owner passed the console lock to us.
> > > +				 * Since we did not spin on console lock, annotate
> > > +				 * this as a trylock. Otherwise lockdep will
> > > +				 * complain.
> > > +				 */
> > > +				mutex_acquire(&console_lock_dep_map, 0, 1, _THIS_IP_);
> > 
> > I am not sure that this correctly imitates the real lock
> > dependency. The trylock flag means that we are able to skip
> > this section when the lock is taken elsewhere. But it is not
> > the whole truth. In fact, we are blocked in this code path
> > when console_sem is taken by another process.
> > 
> > The use of console_owner_lock is not enough. The other
> > console_sem calls a lot of code outside the section
> > guarded by console_owner_lock.

Ah, I confused here console_owner_lock and console_owner_dep_map.
The custom map covers all the code where console_owner is set.
It might be enough to catch a potential bug after all.


> > I think that we have actually entered the cross-release bunch
> > of problems, see https://lwn.net/Articles/709849/

Also I think that we do not need the cross-release stuff after all.
The thing is that we move console_sem only to printk() call
that normally calls console_unlock() as well. It means that
the transferred owner should not bring new type of dependencies.
As Steven said somewhere: "If there is a deadlock, it was
there even before."

We could look at it from this side. The possible deadlock would
look like:

CPU0				CPU1

console_unlock()

  console_owner = current;

				spin_lockA()
				  printk()
				    spin = true;
				    while (...)

    call_console_drivers()
      spin_lockA()

This would be a deadlock. CPU0 would wait for the lock A.
While CPU1 would own the lockA and would wait for CPU0
to finish calling the console drivers and pass the console_sem
owner.

But if the above is true than the following scenario was
already possible before:

CPU0

spin_lockA()
  printk()
    console_unlock()
      call_console_drivers()
	spin_lockA()

By other words, this deadlock was there even before. Such
deadlocks are prevented by using printk_deferred() in
the sections guarded by the lock A.

I am sorry for the noise and that it took me so long to
get over this. Well, nobody said that there was something
wrong with my fears and why. I hope that I did not simplified
it too much this time ;-)

Best Regards,
Petr