linux-kernel - printk_ratelimited() && proc/sys/kernel/printk

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

Date:   Wed, 20 Mar 2019 18:03:42 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Steffen Froemer <sfroemer@...hat.com>, linux-kernel@...r.kernel.org
Subject: printk_ratelimited() && proc/sys/kernel/printk_ratelimit*

Sorry for spam, but I simply do not know whom to ask ;)

/proc/sys/kernel/printk_ratelimit and /proc/sys/kernel/printk_ratelimit_burst
affect printk_ratelimit_state which have a single user, printk_ratelimit() which
is "don't use" according to its comment.

At the same time, printk_ratelimited() uses a static ratelimit_state, so user-
space can't override the default DEFAULT_RATELIMIT_.* numbers.

Isn't it strange? Shouldn't printk_ratelimited() use printk_ratelimit_state ?

	--- x/include/linux/printk.h
	+++ x/include/linux/printk.h
	@@ -415,11 +415,7 @@ extern int kptr_restrict;
	 #ifdef CONFIG_PRINTK
	 #define printk_ratelimited(fmt, ...)					\
	 ({									\
	-	static DEFINE_RATELIMIT_STATE(_rs,				\
	-				      DEFAULT_RATELIMIT_INTERVAL,	\
	-				      DEFAULT_RATELIMIT_BURST);		\
	-									\
	-	if (__ratelimit(&_rs))						\
	+	if (__ratelimit(&printk_ratelimit_state))			\
			printk(fmt, ##__VA_ARGS__);				\
	 })
	 #else
	--- x/kernel/printk/printk.c
	+++ x/kernel/printk/printk.c
	@@ -2979,7 +2979,8 @@ int printk_deferred(const char *fmt, ...
	  * This enforces a rate limit: not more than 10 kernel messages
	  * every 5s to make a denial-of-service attack impossible.
	  */
	-DEFINE_RATELIMIT_STATE(printk_ratelimit_state, 5 * HZ, 10);
	+DEFINE_RATELIMIT_STATE(printk_ratelimit_state,
	+		DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
	 
	 int __printk_ratelimit(const char *func)
	 {

------------------------------------------------------------------------------

Another question is spin_trylock() in ___ratelimit(). This means that a message
can be silently dropped "for no reason", without rs->missed++. Doesn't look good.

The changelog for commit edaac8e3167 "ratelimit: Fix/allow use in atomic contexts"
says:

	I'd like to use printk_ratelimit() in NMI context, but it's not
	robust right now due to spinlock usage in lib/ratelimit.c. If an
	NMI is unlucky enough to hit just that spot we might lock up trying
	to take the spinlock again.

if NMI is the the only problem, why can't we do

	if (in_nmi()) {
		if (!raw_spin_trylock_irqsave(&rs->lock, flags))
			return 0;
	} else {
		raw_spin_lock_irqsave(&rs->lock, flags);
	}

?

Oleg.