linux-kernel - Re: [RFC][PATCH v8 1/2] printk: Make printk() completely async

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:	Fri, 1 Apr 2016 10:59:12 +0200
From:	Petr Mladek <pmladek@...e.com>
To:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jan Kara <jack@...e.com>, Tejun Heo <tj@...nel.org>,
	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	linux-kernel@...r.kernel.org,
	Byungchul Park <byungchul.park@....com>,
	Jan Kara <jack@...e.cz>
Subject: Re: [RFC][PATCH v8 1/2] printk: Make printk() completely async

On Fri 2016-04-01 10:08:03, Sergey Senozhatsky wrote:
> Hello Petr,
> 
> On (03/31/16 13:12), Petr Mladek wrote:
> > > +	 * Set printing kthread sleep condition early, under the
> > > +	 * logbuf_lock, so it (if RUNNING) will go to console_lock()
> > > +	 * and spin on logbuf_lock.
> > > +	 */
> > > +	if (!in_panic && printk_kthread && !need_flush_console)
> > > +		need_flush_console = true;
> > 
> > I would remove the if-statement and always set it:
> > 
> >   + It does not matter if we set it in panic. It will not affect
> >     anything.
> 
> hm... yes, you're right.
> 
> >   + The check for printk_kthread is racy. It might be false here
> >     and it might be true later when check whether to wakeup
> >     the kthread or try to get console directly.
> 
> which is fine, isn't it? we will wakeup the printing kthread, it will
> console_lock()/console_unlock() (regardless the state of need_flush_console).
> printing thread checks need_flush_console only when it decides whether
> it shall schedule.

You almost persuaded me :-) But what about this?

CPU0					CPU1

printk()

  if (printk_kthread)
  # fails and need_flush_console
  # stays false

					init_printk_kthread()
					  # put printk_thread into
					  # run queue
					  printk_kthread = ...;

  if (!in_panic && printk_kthread)
    wake_up_process(printk_kthread);

					# printk kthread finally gets
					# scheduled
					printk_kthread_func()

					set_current_state(TASK_INTERRUPTIBLE);
					if (!need_flush_console)
					  schedule();

=> printk_kthread is happily sleeping without calling console.

We would rearrange printk_kthread_func() to call console
first before going to sleep. But I feel that it still might be
racy some other way because the operations are not synchronized.
For example, printk_kthread_func() might get scheduled on yet
another CPU and might go call consoles before printk_kthread
gets assigned but it might get rescheduled before it goes
into a sleep on preemptive kernel.

I think that it does not matter how need_flush_console if
the kthread is not running. The only drawback is that it will
most likely call consoles once without any real work but
it is only once on start up and we might need it anyway to
handle the above race.

> >   + We might set it true even when it was true before.
> > 
> > I think that this was an attempt to avoid a spurious wake up.
> > But we solve it better way now.
> 
> we also may have 'printk.synchronous = 1', which will purposelessly
> dirty need_flush_console from various CPUs from every printk /* and
> upon every return from console_unlock() */; that's why I added both
> printk_kthread and !need_flush_console (re-dirty already dirtied)
> checks.

I do not see any code that will modify need_flush_console when
printk.synchronous is modified at runtime. If it is set during boot
the kthread will never run and it does not matter how
need_console_flush is set.

I know that all this is rather theoretical. My main point is to remove
unnecessary checks that make the code harder to read and does not bring
any big advantage.

> > >  	raw_spin_lock(&logbuf_lock);
> > >  	retry = console_seq != log_next_seq;
> > > +	if (!retry && printk_kthread)
> > > +		need_flush_console = false;
> > 
> > Similar here. I remove the if-statement and always set it. We will
> > either retry or it should be false anyway.
> 
> well, 'printk.synchronous = 1'. seems that `!retry' check can be
> dropped, I agree.
> 
> a side nano-note,
> apart from 'printk.synchronous = 1', we also can have !printk_kthread
> because kthread_run(printk_kthread_func) failed. it's quite unlikely,
> but still.

If the kthread does not run, it does not matter how need_flush_console
is set. It is similar to the above check. It adds extra logic to the
code that does not bring any big win.

Sigh, I never know when it is the right time to stop wrangling about
details. Well, the easier code helps me to see real problems.

Best Regards,
Petr