linux-kernel - Re: printk: reset may_schedule if console_trylock() from console

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] [day] [month] [year] [list]

Message-ID: <20170105105618.GE480@jagdpanzerIV.localdomain>
Date:   Thu, 5 Jan 2017 19:56:18 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:     sergey.senozhatsky@...il.com, pmladek@...e.com, jack@...e.com,
        tj@...nel.org, kyle@...nel.org, davej@...emonkey.org.uk,
        calvinowens@...com, gregkh@...uxfoundation.org, jslaby@...e.cz,
        linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        sergey.senozhatsky.work@...il.com
Subject: Re: printk: reset may_schedule if console_trylock() from
 console_unlock()

On (01/05/17 19:27), Tetsuo Handa wrote:
[..]
> > the other thing is... do we really need to console_conditional_schedule()
> > from fbcon_*()? console_unlock() does cond_resched() after every line it
> > prints. wouldn't that be enough?
> 
> Every record, isn't it?

yes. after every call to console drivers.

> How many bytes can a record write to consoles?

1024 - PREFIX_MAX bytes at most.

> > so may be we can drop some of console_conditional_schedule()
> > call sites in fbcon. or update console_conditional_schedule()
> > function to always return the current preemption value, not the
> > one we saw in console_trylock().
> 
> Replacing console_may_schedule with get_console_may_schedule() will avoid
> this bug. I noticed that we forgot to reset console_may_schedule to 0
> because the "again:" label is located after the assignment line.

well...
we call cond_resched() from under the spin_lock(), which modifies the
preempt count and cond_resched() which we call from
console_conditional_schedule() checks that current->preempt count is
0 before it calls tif_need_resched().

there are configs/setups where spinlock does not modify preempt count,
but I suspect that on those setups cond_resched() does nothing.

so it looks to me that we just WARN from ___might_sleep() but, at least
in this particular case, I don't think we can actually schedule().
but I just had a very quick look, so I may be completely wrong. need
to double check.

what I tried to say:
-- I will send out a patch for printk() once we settle down the current
   work in progress.

why do I want to address this in printk? because who knows, may be there
is (or there will be) something out there that takes rcu_read_lock() and
then does the console_conditional_schedule(). rcu does not modify current
preempt count, it has its own preempt counter, and get_console_may_schedule()
takes that into account.

> What happened here was:
> 
> [...]

I need more time to walk through your analysis/proposal.

	-ss