printk_ratelimited() && proc/sys/kernel/printk_ratelimit*

[Oleg Nesterov]

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.