[PATCH -v3 0/2] printk.devkmsg: Ratelimit it by default

[PATCH -v3 0/2] printk.devkmsg: Ratelimit it by default

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Hi all,

here's v3 integrating Ingo's comments. The thing is called
printk.devkmsg= or printk_devkmsg now, depending on cmdline option or
sysctl.


Changelog:
----------

v2:

here's v2 with the requested sysctl option kernel.printk_kmsg and
locking of the setting when printk.kmsg= is supplied on the command
line.

Patch 1 is unchanged.

Patch 2 has grown the sysctl addition.

v1:

Rostedt is busy so I took Linus' old patch and Steven's last v2 and
split and extended them with the comments people had on the last thread:

https://lkml.kernel.org/r/20160425145606.598329f2@gandalf.local.home

I hope, at least.

So it is ratelimiting by default, with "on" and "off" cmdline options. I
called the option somewhat a bit shorter too: "printk.kmsg"

The current use cases of this and of which I'm aware are:

* debug the kernel and thus shut up all interfering input from
userspace, i.e. boot with "printk.kmsg=off"

* debug userspace (and by that I mean systemd) by booting with
"printk.kmsg=on" so that the ratelimiting is disabled and the kernel log
gets all the spew.

Thoughts?

Thanks.

Borislav Petkov (2):
  ratelimit: Extend to print suppressed messages on release
  printk: Add kernel parameter to control writes to /dev/kmsg

 Documentation/kernel-parameters.txt |  6 +++
 Documentation/sysctl/kernel.txt     | 14 +++++++
 include/linux/printk.h              |  7 ++++
 include/linux/ratelimit.h           | 36 +++++++++++++---
 kernel/printk/printk.c              | 83 +++++++++++++++++++++++++++++++++----
 kernel/sysctl.c                     |  9 ++++
 lib/ratelimit.c                     |  6 ++-
 7 files changed, 146 insertions(+), 15 deletions(-)

-- 
2.7.3

[PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Extend the ratelimiting facility to print the amount of suppressed lines
when it is being released.

Separated from a previous patch by Linus.

Also, make the ON_RELEASE image not use "callbacks" as it is misleading.

Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Franck Bui <fbui@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 include/linux/ratelimit.h | 36 +++++++++++++++++++++++++++++++-----
 lib/ratelimit.c           |  6 ++++--
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
index 18102529254e..1d8a17ee8395 100644
--- a/include/linux/ratelimit.h
+++ b/include/linux/ratelimit.h
@@ -2,11 +2,15 @@
 #define _LINUX_RATELIMIT_H
 
 #include <linux/param.h>
+#include <linux/sched.h>
 #include <linux/spinlock.h>
 
 #define DEFAULT_RATELIMIT_INTERVAL	(5 * HZ)
 #define DEFAULT_RATELIMIT_BURST		10
 
+/* issue num suppressed message on exit */
+#define RATELIMIT_MSG_ON_RELEASE	BIT(0)
+
 struct ratelimit_state {
 	raw_spinlock_t	lock;		/* protect the state */
 
@@ -15,6 +19,7 @@ struct ratelimit_state {
 	int		printed;
 	int		missed;
 	unsigned long	begin;
+	unsigned long	flags;
 };
 
 #define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {		\
@@ -34,12 +39,33 @@ struct ratelimit_state {
 static inline void ratelimit_state_init(struct ratelimit_state *rs,
 					int interval, int burst)
 {
+	memset(rs, 0, sizeof(*rs));
+
 	raw_spin_lock_init(&rs->lock);
-	rs->interval = interval;
-	rs->burst = burst;
-	rs->printed = 0;
-	rs->missed = 0;
-	rs->begin = 0;
+	rs->interval	= interval;
+	rs->burst	= burst;
+}
+
+static inline void ratelimit_default_init(struct ratelimit_state *rs)
+{
+	return ratelimit_state_init(rs, DEFAULT_RATELIMIT_INTERVAL,
+					DEFAULT_RATELIMIT_BURST);
+}
+
+static inline void ratelimit_state_exit(struct ratelimit_state *rs)
+{
+	if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
+		return;
+
+	if (rs->missed)
+		printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
+		       current->comm, rs->missed);
+}
+
+static inline void
+ratelimit_set_flags(struct ratelimit_state *rs, unsigned long flags)
+{
+	rs->flags = flags;
 }
 
 extern struct ratelimit_state printk_ratelimit_state;
diff --git a/lib/ratelimit.c b/lib/ratelimit.c
index 2c5de86460c5..b753f0cfb00b 100644
--- a/lib/ratelimit.c
+++ b/lib/ratelimit.c
@@ -46,12 +46,14 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func)
 		rs->begin = jiffies;
 
 	if (time_is_before_jiffies(rs->begin + rs->interval)) {
-		if (rs->missed)
+		if (rs->missed && !(rs->flags & RATELIMIT_MSG_ON_RELEASE))
 			printk(KERN_WARNING "%s: %d callbacks suppressed\n",
 				func, rs->missed);
 		rs->begin   = jiffies;
 		rs->printed = 0;
-		rs->missed  = 0;
+
+		if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
+			rs->missed = 0;
 	}
 	if (rs->burst && rs->burst > rs->printed) {
 		rs->printed++;
-- 
2.7.3

[PATCH -v3 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Add a "printk.devkmsg" kernel command line parameter which controls how
userspace writes into /dev/kmsg. It has three options:

* ratelimit - ratelimit logging from userspace.
* on  - unlimited logging from userspace
* off - logging from userspace gets ignored

The default setting is to ratelimit the messages written to it.

It additionally does not limit logging to /dev/kmsg while the system is
booting if we haven't disabled it on the command line.

This patch is based on previous patches from Linus and Steven.

In addition, we can control the logging from a lower priority
sysctl interface - kernel.printk_devkmsg={0,1,2} - with numeric values
corresponding to the options above.

That interface will succeed only if printk.devkmsg *hasn't* been supplied
on the command line. If it has, then printk.devkmsg is a one-time setting
which remains for the duration of the system lifetime.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Franck Bui <fbui@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 Documentation/kernel-parameters.txt |  6 +++
 Documentation/sysctl/kernel.txt     | 14 +++++++
 include/linux/printk.h              |  7 ++++
 kernel/printk/printk.c              | 83 +++++++++++++++++++++++++++++++++----
 kernel/sysctl.c                     |  9 ++++
 5 files changed, 111 insertions(+), 8 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c958d1c..0b1fea56dd49 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3150,6 +3150,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
 			default: disabled
 
+	printk.devkmsg={on,off}
+			Control writing to /dev/kmsg.
+			on - unlimited logging to /dev/kmsg from userspace
+			off - logging to /dev/kmsg disabled
+			Default: ratelimited logging.
+
 	printk.time=	Show timing data prefixed to each printk message line
 			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
 
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index a3683ce2a2f3..dec84d90061c 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -752,6 +752,20 @@ send before ratelimiting kicks in.
 
 ==============================================================
 
+printk_devkmsg:
+
+Control the logging to /dev/kmsg from userspace:
+
+0: default, ratelimited
+1: unlimited logging to /dev/kmsg from userspace
+2: logging to /dev/kmsg disabled
+
+The kernel command line parameter printk.devkmsg= overrides this and is
+a one-time setting until next reboot: once set, it cannot be changed by
+this sysctl interface anymore.
+
+==============================================================
+
 randomize_va_space:
 
 This option can be used to select the type of process address
diff --git a/include/linux/printk.h b/include/linux/printk.h
index f4da695fd615..e6bb50751504 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -171,6 +171,13 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 extern int printk_delay_msec;
 extern int dmesg_restrict;
 extern int kptr_restrict;
+extern unsigned int devkmsg_log;
+
+struct ctl_table;
+
+extern int
+devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, void __user *buf,
+			  size_t *lenp, loff_t *ppos);
 
 extern void wake_up_klogd(void);
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 60cdf6386763..9bc1d037340c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -86,6 +86,55 @@ static struct lockdep_map console_lock_dep_map = {
 };
 #endif
 
+enum devkmsg_log_bits {
+	__DEVKMSG_LOG_BIT_ON = 0,
+	__DEVKMSG_LOG_BIT_OFF,
+	__DEVKMSG_LOG_BIT_LOCK,
+};
+
+enum devkmsg_log_masks {
+	DEVKMSG_LOG_MASK_ON             = BIT(__DEVKMSG_LOG_BIT_ON),
+	DEVKMSG_LOG_MASK_OFF            = BIT(__DEVKMSG_LOG_BIT_OFF),
+	DEVKMSG_LOG_MASK_LOCK           = BIT(__DEVKMSG_LOG_BIT_LOCK),
+};
+
+/* Keep both the 'on' and 'off' bits clear, i.e. ratelimit by default: */
+#define DEVKMSG_LOG_MASK_DEFAULT	0
+
+unsigned int __read_mostly devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
+static int __init control_devkmsg(char *str)
+{
+	if (!str)
+		return -EINVAL;
+
+	if (!strncmp(str, "on", 2))
+		devkmsg_log = DEVKMSG_LOG_MASK_ON;
+	else if (!strncmp(str, "off", 3))
+		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
+	else if (!strncmp(str, "ratelimit", 9))
+		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
+	else
+		return -EINVAL;
+
+	/* Sysctl cannot change it anymore. */
+	devkmsg_log |= DEVKMSG_LOG_MASK_LOCK;
+
+	return 0;
+}
+__setup("printk.devkmsg=", control_devkmsg);
+
+
+int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
+			      void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK) {
+		if (write)
+			return -EINVAL;
+	}
+
+	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+}
+
 /*
  * Number of registered extended console drivers.
  *
@@ -614,6 +663,7 @@ struct devkmsg_user {
 	u64 seq;
 	u32 idx;
 	enum log_flags prev;
+	struct ratelimit_state rs;
 	struct mutex lock;
 	char buf[CONSOLE_EXT_LOG_MAX];
 };
@@ -623,11 +673,24 @@ static ssize_t devkmsg_write(struct kiocb *iocb, struct iov_iter *from)
 	char *buf, *line;
 	int level = default_message_loglevel;
 	int facility = 1;	/* LOG_USER */
+	struct file *file = iocb->ki_filp;
+	struct devkmsg_user *user = file->private_data;
 	size_t len = iov_iter_count(from);
 	ssize_t ret = len;
 
-	if (len > LOG_LINE_MAX)
+	if (!user || len > LOG_LINE_MAX)
 		return -EINVAL;
+
+	/* Ignore when user logging is disabled. */
+	if (devkmsg_log & DEVKMSG_LOG_MASK_OFF)
+		return len;
+
+	/* Ratelimit when not explicitly enabled or when we're not booting. */
+	if ((system_state != SYSTEM_BOOTING) && !(devkmsg_log & DEVKMSG_LOG_MASK_ON)) {
+		if (!___ratelimit(&user->rs, current->comm))
+			return ret;
+	}
+
 	buf = kmalloc(len+1, GFP_KERNEL);
 	if (buf == NULL)
 		return -ENOMEM;
@@ -801,18 +864,20 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	int err;
 
 	/* write-only does not need any file context */
-	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
-		return 0;
-
-	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
-				       SYSLOG_FROM_READER);
-	if (err)
-		return err;
+	if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
+		err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
+					       SYSLOG_FROM_READER);
+		if (err)
+			return err;
+	}
 
 	user = kmalloc(sizeof(struct devkmsg_user), GFP_KERNEL);
 	if (!user)
 		return -ENOMEM;
 
+	ratelimit_default_init(&user->rs);
+	ratelimit_set_flags(&user->rs, RATELIMIT_MSG_ON_RELEASE);
+
 	mutex_init(&user->lock);
 
 	raw_spin_lock_irq(&logbuf_lock);
@@ -831,6 +896,8 @@ static int devkmsg_release(struct inode *inode, struct file *file)
 	if (!user)
 		return 0;
 
+	ratelimit_state_exit(&user->rs);
+
 	mutex_destroy(&user->lock);
 	kfree(user);
 	return 0;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 87b2fc38398b..013d5fe0636a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -814,6 +814,15 @@ static struct ctl_table kern_table[] = {
 		.extra2		= &ten_thousand,
 	},
 	{
+		.procname	= "printk_devkmsg",
+		.data		= &devkmsg_log,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= devkmsg_sysctl_set_loglvl,
+		.extra1		= &zero,
+		.extra2		= &two,
+	},
+	{
 		.procname	= "dmesg_restrict",
 		.data		= &dmesg_restrict,
 		.maxlen		= sizeof(int),
-- 
2.7.3

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Steven Rostedt]

On Mon,  4 Jul 2016 16:24:51 +0200
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Extend the ratelimiting facility to print the amount of suppressed lines
> when it is being released.
> 
> Separated from a previous patch by Linus.
> 
> Also, make the ON_RELEASE image not use "callbacks" as it is misleading.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Franck Bui <fbui@suse.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  include/linux/ratelimit.h | 36 +++++++++++++++++++++++++++++++-----
>  lib/ratelimit.c           |  6 ++++--
>  2 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
> index 18102529254e..1d8a17ee8395 100644
> --- a/include/linux/ratelimit.h
> +++ b/include/linux/ratelimit.h
> @@ -2,11 +2,15 @@
>  #define _LINUX_RATELIMIT_H
>  
>  #include <linux/param.h>
> +#include <linux/sched.h>
>  #include <linux/spinlock.h>
>  
>  #define DEFAULT_RATELIMIT_INTERVAL	(5 * HZ)
>  #define DEFAULT_RATELIMIT_BURST		10
>  
> +/* issue num suppressed message on exit */
> +#define RATELIMIT_MSG_ON_RELEASE	BIT(0)
> +
>  struct ratelimit_state {
>  	raw_spinlock_t	lock;		/* protect the state */
>  
> @@ -15,6 +19,7 @@ struct ratelimit_state {
>  	int		printed;
>  	int		missed;
>  	unsigned long	begin;
> +	unsigned long	flags;
>  };
>  
>  #define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {		\
> @@ -34,12 +39,33 @@ struct ratelimit_state {
>  static inline void ratelimit_state_init(struct ratelimit_state *rs,
>  					int interval, int burst)
>  {
> +	memset(rs, 0, sizeof(*rs));
> +
>  	raw_spin_lock_init(&rs->lock);
> -	rs->interval = interval;
> -	rs->burst = burst;
> -	rs->printed = 0;
> -	rs->missed = 0;
> -	rs->begin = 0;
> +	rs->interval	= interval;
> +	rs->burst	= burst;
> +}
> +
> +static inline void ratelimit_default_init(struct ratelimit_state *rs)
> +{
> +	return ratelimit_state_init(rs, DEFAULT_RATELIMIT_INTERVAL,
> +					DEFAULT_RATELIMIT_BURST);
> +}
> +
> +static inline void ratelimit_state_exit(struct ratelimit_state *rs)
> +{
> +	if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
> +		return;
> +
> +	if (rs->missed)
> +		printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
> +		       current->comm, rs->missed);

Is the comm important? Maybe add the function that called it?

    "%pS", _THIS_IP_

Perhaps add __always_inline, as _THIS_IP_ will point into the function
that calls this?

-- Steve


> +}
> +
> +static inline void
> +ratelimit_set_flags(struct ratelimit_state *rs, unsigned long flags)
> +{
> +	rs->flags = flags;
>  }
>  
>  extern struct ratelimit_state printk_ratelimit_state;
> diff --git a/lib/ratelimit.c b/lib/ratelimit.c
> index 2c5de86460c5..b753f0cfb00b 100644
> --- a/lib/ratelimit.c
> +++ b/lib/ratelimit.c
> @@ -46,12 +46,14 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func)
>  		rs->begin = jiffies;
>  
>  	if (time_is_before_jiffies(rs->begin + rs->interval)) {
> -		if (rs->missed)
> +		if (rs->missed && !(rs->flags & RATELIMIT_MSG_ON_RELEASE))
>  			printk(KERN_WARNING "%s: %d callbacks suppressed\n",
>  				func, rs->missed);
>  		rs->begin   = jiffies;
>  		rs->printed = 0;
> -		rs->missed  = 0;
> +
> +		if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
> +			rs->missed = 0;
>  	}
>  	if (rs->burst && rs->burst > rs->printed) {
>  		rs->printed++;

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Borislav Petkov]

On Tue, Jul 05, 2016 at 02:26:48PM -0400, Steven Rostedt wrote:
> > +	if (rs->missed)
> > +		printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
> > +		       current->comm, rs->missed);
> 
> Is the comm important?

Yes, we wanna dump the task name which called devkmsg_release().

> Maybe add the function that called it?
> 
>     "%pS", _THIS_IP_
> 
> Perhaps add __always_inline, as _THIS_IP_ will point into the function
> that calls this?

That would inadvertently be devkmsg_release() in this use case. But
we want to dump the task name which opened and spat so much crap into
/dev/kmsg so as to cause the ratelimiting to hit.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Steven Rostedt]

On Tue, 5 Jul 2016 20:45:17 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Tue, Jul 05, 2016 at 02:26:48PM -0400, Steven Rostedt wrote:
> > > +	if (rs->missed)
> > > +		printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
> > > +		       current->comm, rs->missed);  
> > 
> > Is the comm important?  
> 
> Yes, we wanna dump the task name which called devkmsg_release().
> 
> > Maybe add the function that called it?
> > 
> >     "%pS", _THIS_IP_
> > 
> > Perhaps add __always_inline, as _THIS_IP_ will point into the function
> > that calls this?  
> 
> That would inadvertently be devkmsg_release() in this use case. But
> we want to dump the task name which opened and spat so much crap into
> /dev/kmsg so as to cause the ratelimiting to hit.
> 

Perhaps we should show both, unless you don't think this will ever be
used by anything other than devkmsg?

-- Steve

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Borislav Petkov]

On Tue, Jul 05, 2016 at 02:57:32PM -0400, Steven Rostedt wrote:
> Perhaps we should show both, unless you don't think this will ever be
> used by anything other than devkmsg?

I'd say let's do it only when we go down that road and start using it
for something else.

Because, for example, the ratelimiting thing is, in fact, generic but it
is used primarily to ratelimit printks. Even though it could be used for
something else, theoretically...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Steven Rostedt]

On Tue, 5 Jul 2016 21:42:45 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Tue, Jul 05, 2016 at 02:57:32PM -0400, Steven Rostedt wrote:
> > Perhaps we should show both, unless you don't think this will ever be
> > used by anything other than devkmsg?  
> 
> I'd say let's do it only when we go down that road and start using it
> for something else.
> 
> Because, for example, the ratelimiting thing is, in fact, generic but it
> is used primarily to ratelimit printks. Even though it could be used for
> something else, theoretically...
> 

But you know... Build it and they will come.

OK, I'm fine then.

-- Steve

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Joe Perches]

On Tue, 2016-07-05 at 15:49 -0400, Steven Rostedt wrote:
> On Tue, 5 Jul 2016 21:42:45 +0200 Borislav Petkov <bp@alien8.de> wrote:
> > On Tue, Jul 05, 2016 at 02:57:32PM -0400, Steven Rostedt wrote:
> > > Perhaps we should show both, unless you don't think this will ever be
> > > used by anything other than devkmsg?  
> > I'd say let's do it only when we go down that road and start using it
> > for something else.
> > 
> > Because, for example, the ratelimiting thing is, in fact, generic but it
> > is used primarily to ratelimit printks. Even though it could be used for
> > something else, theoretically...
> > 
> But you know... Build it and they will come.

arch/s390/kvm/kvm-s390.c:       ratelimit_state_init(&kvm->arch.sthyi_limit, 5 * HZ, 500);

As far as I know, _ratelimit is used for non-printk purposes in
arch/s390/kvm/sthyi.c

int handle_sthyi(struct kvm_vcpu *vcpu)
{
	int reg1, reg2, r = 0;
	u64 code, addr, cc = 0;
	struct sthyi_sctns *sctns = NULL;

	/*
	 * STHYI requires extensive locking in the higher hypervisors
	 * and is very computational/memory expensive. Therefore we
	 * ratelimit the executions per VM.
	 */
	if (!__ratelimit(&vcpu->kvm->arch.sthyi_limit)) {
		kvm_s390_retry_instr(vcpu);
		return 0;
	}

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Christian Borntraeger]

On 07/05/2016 10:08 PM, Joe Perches wrote:
> On Tue, 2016-07-05 at 15:49 -0400, Steven Rostedt wrote:
>> On Tue, 5 Jul 2016 21:42:45 +0200 Borislav Petkov <bp@alien8.de> wrote:
>>> On Tue, Jul 05, 2016 at 02:57:32PM -0400, Steven Rostedt wrote:
>>>> Perhaps we should show both, unless you don't think this will ever be
>>>> used by anything other than devkmsg?  
>>> I'd say let's do it only when we go down that road and start using it
>>> for something else.
>>>
>>> Because, for example, the ratelimiting thing is, in fact, generic but it
>>> is used primarily to ratelimit printks. Even though it could be used for
>>> something else, theoretically...
>>>
>> But you know... Build it and they will come.
> 
> arch/s390/kvm/kvm-s390.c:       ratelimit_state_init(&kvm->arch.sthyi_limit, 5 * HZ, 500);
> 
> As far as I know, _ratelimit is used for non-printk purposes in
> arch/s390/kvm/sthyi.c
> 
> int handle_sthyi(struct kvm_vcpu *vcpu)
> {
> 	int reg1, reg2, r = 0;
> 	u64 code, addr, cc = 0;
> 	struct sthyi_sctns *sctns = NULL;
> 
> 	/*
> 	 * STHYI requires extensive locking in the higher hypervisors
> 	 * and is very computational/memory expensive. Therefore we
> 	 * ratelimit the executions per VM.
> 	 */
> 	if (!__ratelimit(&vcpu->kvm->arch.sthyi_limit)) {
> 		kvm_s390_retry_instr(vcpu);
> 		return 0;
> 	}

Yes, this is new in next. As far as I can see, the new message would only
appear if we would call ratelimit_state_exit. Correct? We do not call this -
I assume this is ok?

We really only want to reuse the rate limit base code (to avoid writing the same
code twice) and being in lib indicated that this can indeed be used outside
printk.
Now: your patch 1 would allow me to get rid of the messages completely
by setting the flag and by not calling ratelimit_state_exit. Which is probably
what we should do in our code.


Christian

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Paolo Bonzini]

On 05/07/2016 22:53, Christian Borntraeger wrote:
> Yes, this is new in next. As far as I can see, the new message would only
> appear if we would call ratelimit_state_exit. Correct? We do not call this -
> I assume this is ok?
> 
> We really only want to reuse the rate limit base code (to avoid writing the same
> code twice) and being in lib indicated that this can indeed be used outside
> printk.
> Now: your patch 1 would allow me to get rid of the messages completely
> by setting the flag and by not calling ratelimit_state_exit. Which is probably
> what we should do in our code.

Can we delay fixing this after the code is merged in Linus's tree?

Paolo

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Christian Borntraeger]

On 07/05/2016 11:14 PM, Paolo Bonzini wrote:
> 
> 
> On 05/07/2016 22:53, Christian Borntraeger wrote:
>> Yes, this is new in next. As far as I can see, the new message would only
>> appear if we would call ratelimit_state_exit. Correct? We do not call this -
>> I assume this is ok?
>>
>> We really only want to reuse the rate limit base code (to avoid writing the same
>> code twice) and being in lib indicated that this can indeed be used outside
>> printk.
>> Now: your patch 1 would allow me to get rid of the messages completely
>> by setting the flag and by not calling ratelimit_state_exit. Which is probably
>> what we should do in our code.
> 
> Can we delay fixing this after the code is merged in Linus's tree?

Absolutely. We already have 2 smaller conflicts in next and I certainly do not want to add
another one.

The current ratelimit print does not hurt - it is just not necessary for us.
so my statement was just a "statement of direction" to write some IBM speak ;-)

Re: [PATCH -v3 1/2] ratelimit: Extend to print suppressed messages on release

[Borislav Petkov]

On Tue, Jul 05, 2016 at 10:53:55PM +0200, Christian Borntraeger wrote:
> Yes, this is new in next. As far as I can see, the new message would only
> appear if we would call ratelimit_state_exit. Correct? We do not call this -
> I assume this is ok?

Right, the idea for the /dev/kmsg use case was to issue the suppressed
count only when we release the ratelimit state.

> We really only want to reuse the rate limit base code (to avoid writing the same
> code twice) and being in lib indicated that this can indeed be used outside
> printk.
> Now: your patch 1 would allow me to get rid of the messages completely
> by setting the flag and by not calling ratelimit_state_exit. Which is probably
> what we should do in our code.

Yeah, that should work for your usecase.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH -v3 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Steven Rostedt]

On Mon,  4 Jul 2016 16:24:52 +0200
Borislav Petkov <bp@alien8.de> wrote:

> @@ -614,6 +663,7 @@ struct devkmsg_user {
>  	u64 seq;
>  	u32 idx;
>  	enum log_flags prev;
> +	struct ratelimit_state rs;
>  	struct mutex lock;
>  	char buf[CONSOLE_EXT_LOG_MAX];
>  };
> @@ -623,11 +673,24 @@ static ssize_t devkmsg_write(struct kiocb *iocb, struct iov_iter *from)
>  	char *buf, *line;
>  	int level = default_message_loglevel;
>  	int facility = 1;	/* LOG_USER */
> +	struct file *file = iocb->ki_filp;
> +	struct devkmsg_user *user = file->private_data;
>  	size_t len = iov_iter_count(from);
>  	ssize_t ret = len;
>  
> -	if (len > LOG_LINE_MAX)
> +	if (!user || len > LOG_LINE_MAX)
>  		return -EINVAL;
> +
> +	/* Ignore when user logging is disabled. */
> +	if (devkmsg_log & DEVKMSG_LOG_MASK_OFF)
> +		return len;

I wonder if we should return some sort of error message here? ENODEV?

> +
> +	/* Ratelimit when not explicitly enabled or when we're not booting. */
> +	if ((system_state != SYSTEM_BOOTING) && !(devkmsg_log & DEVKMSG_LOG_MASK_ON)) {
> +		if (!___ratelimit(&user->rs, current->comm))
> +			return ret;
> +	}
> +
>  	buf = kmalloc(len+1, GFP_KERNEL);
>  	if (buf == NULL)
>  		return -ENOMEM;
> @@ -801,18 +864,20 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	int err;
>  
>  	/* write-only does not need any file context */
> -	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
> -		return 0;
> -
> -	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> -				       SYSLOG_FROM_READER);
> -	if (err)
> -		return err;
> +	if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> +		err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> +					       SYSLOG_FROM_READER);
> +		if (err)
> +			return err;
> +	}

Hmm, there's no error message when it is disabled? I'm not sure that is
what we want. I specifically had the return be an error on open if it
was disabled, because (surprisingly) systemd does the right thing and
uses another utility for syslogging.

If you silently fail here, then we lose all logging because systemd
thinks this is working when it is not. That's not what I want.

-- Steve

>  
>  	user = kmalloc(sizeof(struct devkmsg_user), GFP_KERNEL);
>  	if (!user)
>  		return -ENOMEM;
>  
> +	ratelimit_default_init(&user->rs);
> +	ratelimit_set_flags(&user->rs, RATELIMIT_MSG_ON_RELEASE);
> +
>  	mutex_init(&user->lock);
>  
>  	raw_spin_lock_irq(&logbuf_lock);
> @@ -831,6 +896,8 @@ static int devkmsg_release(struct inode *inode, struct file *file)
>  	if (!user)
>  		return 0;
>  
> +	ratelimit_state_exit(&user->rs);
> +
>  	mutex_destroy(&user->lock);
>  	kfree(user);
>  	return 0;
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 87b2fc38398b..013d5fe0636a 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -814,6 +814,15 @@ static struct ctl_table kern_table[] = {
>  		.extra2		= &ten_thousand,
>  	},
>  	{
> +		.procname	= "printk_devkmsg",
> +		.data		= &devkmsg_log,
> +		.maxlen		= sizeof(unsigned int),
> +		.mode		= 0644,
> +		.proc_handler	= devkmsg_sysctl_set_loglvl,
> +		.extra1		= &zero,
> +		.extra2		= &two,
> +	},
> +	{
>  		.procname	= "dmesg_restrict",
>  		.data		= &dmesg_restrict,
>  		.maxlen		= sizeof(int),

Re: [PATCH -v3 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Borislav Petkov]

On Tue, Jul 05, 2016 at 05:47:49PM -0400, Steven Rostedt wrote:
> I wonder if we should return some sort of error message here? ENODEV?

What for?

We're silently ignoring it. If we start returning an error here, we
might break doofus if it checks the write retval.

And it's not like we care - we're ignoring all writes whatsoever.

> If you silently fail here, then we lose all logging because systemd
> thinks this is working when it is not. That's not what I want.

Hmm, ok, you're making sense to me.

Do you want an error message too or only an -ENODEV returned or
somesuch?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH -v3 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Steven Rostedt]

On Wed, 6 Jul 2016 00:02:21 +0200
Borislav Petkov <bp@alien8.de> wrote:

> > If you silently fail here, then we lose all logging because systemd
> > thinks this is working when it is not. That's not what I want.  
> 
> Hmm, ok, you're making sense to me.
> 
> Do you want an error message too or only an -ENODEV returned or
> somesuch?
> 

My patch returned -EPERM and it works on my box. That is, systemd finds
something else to log to.

-- Steve

[PATCH -v3.1 1/2] ratelimit: Extend to print suppressed messages on release

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Extend the ratelimiting facility to print the amount of suppressed lines
when it is being released.

Separated from a previous patch by Linus.

Also, make the ON_RELEASE image not use "callbacks" as it is misleading.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Franck Bui <fbui@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---

v3.1: Simplify testing of !RATELIMIT_MSG_ON_RELEASE in ___ratelimit()

 include/linux/ratelimit.h | 36 +++++++++++++++++++++++++++++++-----
 lib/ratelimit.c           | 15 +++++++++------
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
index 18102529254e..1d8a17ee8395 100644
--- a/include/linux/ratelimit.h
+++ b/include/linux/ratelimit.h
@@ -2,11 +2,15 @@
 #define _LINUX_RATELIMIT_H
 
 #include <linux/param.h>
+#include <linux/sched.h>
 #include <linux/spinlock.h>
 
 #define DEFAULT_RATELIMIT_INTERVAL	(5 * HZ)
 #define DEFAULT_RATELIMIT_BURST		10
 
+/* issue num suppressed message on exit */
+#define RATELIMIT_MSG_ON_RELEASE	BIT(0)
+
 struct ratelimit_state {
 	raw_spinlock_t	lock;		/* protect the state */
 
@@ -15,6 +19,7 @@ struct ratelimit_state {
 	int		printed;
 	int		missed;
 	unsigned long	begin;
+	unsigned long	flags;
 };
 
 #define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {		\
@@ -34,12 +39,33 @@ struct ratelimit_state {
 static inline void ratelimit_state_init(struct ratelimit_state *rs,
 					int interval, int burst)
 {
+	memset(rs, 0, sizeof(*rs));
+
 	raw_spin_lock_init(&rs->lock);
-	rs->interval = interval;
-	rs->burst = burst;
-	rs->printed = 0;
-	rs->missed = 0;
-	rs->begin = 0;
+	rs->interval	= interval;
+	rs->burst	= burst;
+}
+
+static inline void ratelimit_default_init(struct ratelimit_state *rs)
+{
+	return ratelimit_state_init(rs, DEFAULT_RATELIMIT_INTERVAL,
+					DEFAULT_RATELIMIT_BURST);
+}
+
+static inline void ratelimit_state_exit(struct ratelimit_state *rs)
+{
+	if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
+		return;
+
+	if (rs->missed)
+		printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
+		       current->comm, rs->missed);
+}
+
+static inline void
+ratelimit_set_flags(struct ratelimit_state *rs, unsigned long flags)
+{
+	rs->flags = flags;
 }
 
 extern struct ratelimit_state printk_ratelimit_state;
diff --git a/lib/ratelimit.c b/lib/ratelimit.c
index 2c5de86460c5..734cba5c8842 100644
--- a/lib/ratelimit.c
+++ b/lib/ratelimit.c
@@ -46,12 +46,15 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func)
 		rs->begin = jiffies;
 
 	if (time_is_before_jiffies(rs->begin + rs->interval)) {
-		if (rs->missed)
-			printk(KERN_WARNING "%s: %d callbacks suppressed\n",
-				func, rs->missed);
-		rs->begin   = jiffies;
-		rs->printed = 0;
-		rs->missed  = 0;
+		if (rs->missed) {
+			if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
+				pr_warn("%s: %d callbacks suppressed\n", func, rs->missed);
+				rs->missed = 0;
+			}
+
+			rs->begin   = jiffies;
+			rs->printed = 0;
+		}
 	}
 	if (rs->burst && rs->burst > rs->printed) {
 		rs->printed++;
-- 
2.7.3

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCH -v3.1 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>
 
Add a "printk.devkmsg" kernel command line parameter which controls how
userspace writes into /dev/kmsg. It has three options:

* ratelimit - ratelimit logging from userspace.
* on  - unlimited logging from userspace
* off - logging from userspace gets ignored

The default setting is to ratelimit the messages written to it.

It additionally does not limit logging to /dev/kmsg while the system is
booting if we haven't disabled it on the command line.

This patch is based on previous patches from Linus and Steven.

In addition, we can control the logging from a lower priority
sysctl interface - kernel.printk_devkmsg={0,1,2} - with numeric values
corresponding to the options above.

That interface will succeed only if printk.devkmsg *hasn't* been supplied
on the command line. If it has, then printk.devkmsg is a one-time setting
which remains for the duration of the system lifetime.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Franck Bui <fbui@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---

v3.1: Return error (-EPERM) on open when devkmsg logging disabled, as
Steve requested.

 Documentation/kernel-parameters.txt |  6 +++
 Documentation/sysctl/kernel.txt     | 14 ++++++
 include/linux/printk.h              |  7 +++
 kernel/printk/printk.c              | 86 +++++++++++++++++++++++++++++++++----
 kernel/sysctl.c                     |  9 ++++
 5 files changed, 114 insertions(+), 8 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c958d1c..0b1fea56dd49 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3150,6 +3150,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
 			default: disabled
 
+	printk.devkmsg={on,off}
+			Control writing to /dev/kmsg.
+			on - unlimited logging to /dev/kmsg from userspace
+			off - logging to /dev/kmsg disabled
+			Default: ratelimited logging.
+
 	printk.time=	Show timing data prefixed to each printk message line
 			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
 
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index a3683ce2a2f3..dec84d90061c 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -752,6 +752,20 @@ send before ratelimiting kicks in.
 
 ==============================================================
 
+printk_devkmsg:
+
+Control the logging to /dev/kmsg from userspace:
+
+0: default, ratelimited
+1: unlimited logging to /dev/kmsg from userspace
+2: logging to /dev/kmsg disabled
+
+The kernel command line parameter printk.devkmsg= overrides this and is
+a one-time setting until next reboot: once set, it cannot be changed by
+this sysctl interface anymore.
+
+==============================================================
+
 randomize_va_space:
 
 This option can be used to select the type of process address
diff --git a/include/linux/printk.h b/include/linux/printk.h
index f4da695fd615..e6bb50751504 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -171,6 +171,13 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 extern int printk_delay_msec;
 extern int dmesg_restrict;
 extern int kptr_restrict;
+extern unsigned int devkmsg_log;
+
+struct ctl_table;
+
+extern int
+devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, void __user *buf,
+			  size_t *lenp, loff_t *ppos);
 
 extern void wake_up_klogd(void);
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 60cdf6386763..1e71369780f0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -86,6 +86,55 @@ static struct lockdep_map console_lock_dep_map = {
 };
 #endif
 
+enum devkmsg_log_bits {
+	__DEVKMSG_LOG_BIT_ON = 0,
+	__DEVKMSG_LOG_BIT_OFF,
+	__DEVKMSG_LOG_BIT_LOCK,
+};
+
+enum devkmsg_log_masks {
+	DEVKMSG_LOG_MASK_ON             = BIT(__DEVKMSG_LOG_BIT_ON),
+	DEVKMSG_LOG_MASK_OFF            = BIT(__DEVKMSG_LOG_BIT_OFF),
+	DEVKMSG_LOG_MASK_LOCK           = BIT(__DEVKMSG_LOG_BIT_LOCK),
+};
+
+/* Keep both the 'on' and 'off' bits clear, i.e. ratelimit by default: */
+#define DEVKMSG_LOG_MASK_DEFAULT	0
+
+unsigned int __read_mostly devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
+static int __init control_devkmsg(char *str)
+{
+	if (!str)
+		return -EINVAL;
+
+	if (!strncmp(str, "on", 2))
+		devkmsg_log = DEVKMSG_LOG_MASK_ON;
+	else if (!strncmp(str, "off", 3))
+		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
+	else if (!strncmp(str, "ratelimit", 9))
+		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
+	else
+		return -EINVAL;
+
+	/* Sysctl cannot change it anymore. */
+	devkmsg_log |= DEVKMSG_LOG_MASK_LOCK;
+
+	return 0;
+}
+__setup("printk.devkmsg=", control_devkmsg);
+
+
+int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
+			      void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK) {
+		if (write)
+			return -EINVAL;
+	}
+
+	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+}
+
 /*
  * Number of registered extended console drivers.
  *
@@ -614,6 +663,7 @@ struct devkmsg_user {
 	u64 seq;
 	u32 idx;
 	enum log_flags prev;
+	struct ratelimit_state rs;
 	struct mutex lock;
 	char buf[CONSOLE_EXT_LOG_MAX];
 };
@@ -623,11 +673,24 @@ static ssize_t devkmsg_write(struct kiocb *iocb, struct iov_iter *from)
 	char *buf, *line;
 	int level = default_message_loglevel;
 	int facility = 1;	/* LOG_USER */
+	struct file *file = iocb->ki_filp;
+	struct devkmsg_user *user = file->private_data;
 	size_t len = iov_iter_count(from);
 	ssize_t ret = len;
 
-	if (len > LOG_LINE_MAX)
+	if (!user || len > LOG_LINE_MAX)
 		return -EINVAL;
+
+	/* Ignore when user logging is disabled. */
+	if (devkmsg_log & DEVKMSG_LOG_MASK_OFF)
+		return len;
+
+	/* Ratelimit when not explicitly enabled or when we're not booting. */
+	if ((system_state != SYSTEM_BOOTING) && !(devkmsg_log & DEVKMSG_LOG_MASK_ON)) {
+		if (!___ratelimit(&user->rs, current->comm))
+			return ret;
+	}
+
 	buf = kmalloc(len+1, GFP_KERNEL);
 	if (buf == NULL)
 		return -ENOMEM;
@@ -800,19 +863,24 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	struct devkmsg_user *user;
 	int err;
 
-	/* write-only does not need any file context */
-	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
-		return 0;
+	if (devkmsg_log & DEVKMSG_LOG_MASK_OFF)
+		return -EPERM;
 
-	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
-				       SYSLOG_FROM_READER);
-	if (err)
-		return err;
+	/* write-only does not need any file context */
+	if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
+		err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
+					       SYSLOG_FROM_READER);
+		if (err)
+			return err;
+	}
 
 	user = kmalloc(sizeof(struct devkmsg_user), GFP_KERNEL);
 	if (!user)
 		return -ENOMEM;
 
+	ratelimit_default_init(&user->rs);
+	ratelimit_set_flags(&user->rs, RATELIMIT_MSG_ON_RELEASE);
+
 	mutex_init(&user->lock);
 
 	raw_spin_lock_irq(&logbuf_lock);
@@ -831,6 +899,8 @@ static int devkmsg_release(struct inode *inode, struct file *file)
 	if (!user)
 		return 0;
 
+	ratelimit_state_exit(&user->rs);
+
 	mutex_destroy(&user->lock);
 	kfree(user);
 	return 0;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 87b2fc38398b..013d5fe0636a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -814,6 +814,15 @@ static struct ctl_table kern_table[] = {
 		.extra2		= &ten_thousand,
 	},
 	{
+		.procname	= "printk_devkmsg",
+		.data		= &devkmsg_log,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= devkmsg_sysctl_set_loglvl,
+		.extra1		= &zero,
+		.extra2		= &two,
+	},
+	{
 		.procname	= "dmesg_restrict",
 		.data		= &dmesg_restrict,
 		.maxlen		= sizeof(int),
-- 
2.7.3


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH -v3.1 1/2] ratelimit: Extend to print suppressed messages on release

[Steven Rostedt]

On Wed, 6 Jul 2016 15:28:00 +0200
Borislav Petkov <bp@alien8.de> wrote:

>  extern struct ratelimit_state printk_ratelimit_state;
> diff --git a/lib/ratelimit.c b/lib/ratelimit.c
> index 2c5de86460c5..734cba5c8842 100644
> --- a/lib/ratelimit.c
> +++ b/lib/ratelimit.c
> @@ -46,12 +46,15 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func)
>  		rs->begin = jiffies;
>  
>  	if (time_is_before_jiffies(rs->begin + rs->interval)) {
> -		if (rs->missed)
> -			printk(KERN_WARNING "%s: %d callbacks suppressed\n",
> -				func, rs->missed);
> -		rs->begin   = jiffies;
> -		rs->printed = 0;
> -		rs->missed  = 0;
> +		if (rs->missed) {
> +			if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
> +				pr_warn("%s: %d callbacks suppressed\n", func, rs->missed);
> +				rs->missed = 0;
> +			}
> +
> +			rs->begin   = jiffies;
> +			rs->printed = 0;
> +		}

Hmm, the above changes the previous logic. That seems wrong.

Before:

	if (rs->missed)
		[...]
	rs->begin = jiffies;
	rs->printed = 0;

after:

	if (rs->missed) {
		rs->begin = jiffies;
		rs->printed = 0;
	}

I think you wanted:

	if (rs->missed) {
		if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
			[..]
			rs->missed = 0;
		}
	}
	rs->begin = jiffies;
	rs->printed = 0;

-- Steve



>  	}
>  	if (rs->burst && rs->burst > rs->printed) {
>  		rs->printed++;

Re: [PATCH -v3.1 1/2] ratelimit: Extend to print suppressed messages on release

[Joe Perches]

On Wed, 2016-07-06 at 15:28 +0200, Borislav Petkov wrote:
> Extend the ratelimiting facility to print the amount of suppressed lines
> when it is being released.
[]
> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
[]
> +static inline void ratelimit_state_exit(struct ratelimit_state *rs)
> +{
> +	if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
> +		return;
> +
> +	if (rs->missed)
> +		printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
> +		       current->comm, rs->missed);

Please use pr_warn as it will use whatever
pr_fmt prefix is specified by the subsystem.

Maybe:
	if (rs->missed)
		pr_warn("ratelimit: %s: %d output lines suppressed\n",
			current->comm, rs->missed);

to be more similar to the callback suppressed message in
lib/ratelimit.c

				pr_warn("%s: %d callbacks suppressed\n", func, rs->missed);

[PATCH -v3.2 1/2] ratelimit: Extend to print suppressed messages on release

[Borislav Petkov]

On Wed, Jul 06, 2016 at 09:40:34AM -0400, Steven Rostedt wrote:
> 	if (rs->missed) {
> 		rs->begin = jiffies;
> 		rs->printed = 0;
> 	}

This is what I get when I'm trying to juggle 10 things at the same time
:-\

Ok, here's v3.2. Thanks dude!

---
From: Borislav Petkov <bp@suse.de>
Date: Tue, 14 Jun 2016 11:51:04 +0200
Subject: [PATCH -v3.2 1/2] ratelimit: Extend to print suppressed messages on release
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Extend the ratelimiting facility to print the amount of suppressed lines
when it is being released.

Separated from a previous patch by Linus.

Also, make the ON_RELEASE image not use "callbacks" as it is misleading.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Franck Bui <fbui@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---

v3.2: Simplify testing of !RATELIMIT_MSG_ON_RELEASE in ___ratelimit() correctly!

 include/linux/ratelimit.h | 36 +++++++++++++++++++++++++++++++-----
 lib/ratelimit.c           | 10 ++++++----
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
index 18102529254e..1d8a17ee8395 100644
--- a/include/linux/ratelimit.h
+++ b/include/linux/ratelimit.h
@@ -2,11 +2,15 @@
 #define _LINUX_RATELIMIT_H
 
 #include <linux/param.h>
+#include <linux/sched.h>
 #include <linux/spinlock.h>
 
 #define DEFAULT_RATELIMIT_INTERVAL	(5 * HZ)
 #define DEFAULT_RATELIMIT_BURST		10
 
+/* issue num suppressed message on exit */
+#define RATELIMIT_MSG_ON_RELEASE	BIT(0)
+
 struct ratelimit_state {
 	raw_spinlock_t	lock;		/* protect the state */
 
@@ -15,6 +19,7 @@ struct ratelimit_state {
 	int		printed;
 	int		missed;
 	unsigned long	begin;
+	unsigned long	flags;
 };
 
 #define RATELIMIT_STATE_INIT(name, interval_init, burst_init) {		\
@@ -34,12 +39,33 @@ struct ratelimit_state {
 static inline void ratelimit_state_init(struct ratelimit_state *rs,
 					int interval, int burst)
 {
+	memset(rs, 0, sizeof(*rs));
+
 	raw_spin_lock_init(&rs->lock);
-	rs->interval = interval;
-	rs->burst = burst;
-	rs->printed = 0;
-	rs->missed = 0;
-	rs->begin = 0;
+	rs->interval	= interval;
+	rs->burst	= burst;
+}
+
+static inline void ratelimit_default_init(struct ratelimit_state *rs)
+{
+	return ratelimit_state_init(rs, DEFAULT_RATELIMIT_INTERVAL,
+					DEFAULT_RATELIMIT_BURST);
+}
+
+static inline void ratelimit_state_exit(struct ratelimit_state *rs)
+{
+	if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
+		return;
+
+	if (rs->missed)
+		printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
+		       current->comm, rs->missed);
+}
+
+static inline void
+ratelimit_set_flags(struct ratelimit_state *rs, unsigned long flags)
+{
+	rs->flags = flags;
 }
 
 extern struct ratelimit_state printk_ratelimit_state;
diff --git a/lib/ratelimit.c b/lib/ratelimit.c
index 2c5de86460c5..08f8043cac61 100644
--- a/lib/ratelimit.c
+++ b/lib/ratelimit.c
@@ -46,12 +46,14 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func)
 		rs->begin = jiffies;
 
 	if (time_is_before_jiffies(rs->begin + rs->interval)) {
-		if (rs->missed)
-			printk(KERN_WARNING "%s: %d callbacks suppressed\n",
-				func, rs->missed);
+		if (rs->missed) {
+			if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
+				pr_warn("%s: %d callbacks suppressed\n", func, rs->missed);
+				rs->missed = 0;
+			}
+		}
 		rs->begin   = jiffies;
 		rs->printed = 0;
-		rs->missed  = 0;
 	}
 	if (rs->burst && rs->burst > rs->printed) {
 		rs->printed++;
-- 
2.7.3

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH -v3.1 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Steven Rostedt]

On Wed, 6 Jul 2016 15:29:24 +0200
Borislav Petkov <bp@alien8.de> wrote:


> @@ -800,19 +863,24 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	struct devkmsg_user *user;
>  	int err;
>  
> -	/* write-only does not need any file context */
> -	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
> -		return 0;
> +	if (devkmsg_log & DEVKMSG_LOG_MASK_OFF)
> +		return -EPERM;
>  
> -	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> -				       SYSLOG_FROM_READER);
> -	if (err)
> -		return err;
> +	/* write-only does not need any file context */
> +	if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> +		err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> +					       SYSLOG_FROM_READER);
> +		if (err)
> +			return err;
> +	}
>  
>  	user = kmalloc(sizeof(struct devkmsg_user), GFP_KERNEL);
>  	if (!user)
>  		return -ENOMEM;
>  
> +	ratelimit_default_init(&user->rs);
> +	ratelimit_set_flags(&user->rs, RATELIMIT_MSG_ON_RELEASE);
> +
>  	mutex_init(&user->lock);
>  
>  	raw_spin_lock_irq(&logbuf_lock);
> @@ -831,6 +899,8 @@ static int devkmsg_release(struct inode *inode, struct file *file)
>  	if (!user)
>  		return 0;
>  
> +	ratelimit_state_exit(&user->rs);
> +
>  	mutex_destroy(&user->lock);
>  	kfree(user);
>  	return 0;
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c

Hmm, this does nothing to stop user space from doing the following:

while :; do echo '5,0,0,-;hello' > /dev/kmsg; done

But at least it's a start.

-- Steve

Re: [PATCH -v3.1 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Borislav Petkov]

On Wed, Jul 06, 2016 at 01:52:11PM -0400, Steven Rostedt wrote:
> Hmm, this does nothing to stop user space from doing the following:
> 
> while :; do echo '5,0,0,-;hello' > /dev/kmsg; done

I wouldn't worry too much if /dev/kmsg has root:root perms elsewhere
too, like it is the case in my guest here:

crw-r--r-- 1 root root 1, 11 Jul  6 22:28 /dev/kmsg

root can shoot herself in the foot in countless ways anyway.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH -v3.2 1/2] ratelimit: Extend to print suppressed messages on release

[Steven Rostedt]

On Wed, 6 Jul 2016 16:59:29 +0200
Borislav Petkov <bp@alien8.de> wrote:

> +static inline void ratelimit_state_exit(struct ratelimit_state *rs)
> +{
> +	if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
> +		return;
> +
> +	if (rs->missed)
> +		printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
> +		       current->comm, rs->missed);

Hmm, should this clear the missed flag? Especially since it isn't
cleared below.

-- Steve

> +}
> +
> +static inline void
> +ratelimit_set_flags(struct ratelimit_state *rs, unsigned long flags)
> +{
> +	rs->flags = flags;
>  }
>  
>  extern struct ratelimit_state printk_ratelimit_state;
> diff --git a/lib/ratelimit.c b/lib/ratelimit.c
> index 2c5de86460c5..08f8043cac61 100644
> --- a/lib/ratelimit.c
> +++ b/lib/ratelimit.c
> @@ -46,12 +46,14 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func)
>  		rs->begin = jiffies;
>  
>  	if (time_is_before_jiffies(rs->begin + rs->interval)) {
> -		if (rs->missed)
> -			printk(KERN_WARNING "%s: %d callbacks suppressed\n",
> -				func, rs->missed);
> +		if (rs->missed) {
> +			if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
> +				pr_warn("%s: %d callbacks suppressed\n", func, rs->missed);
> +				rs->missed = 0;
> +			}
> +		}
>  		rs->begin   = jiffies;
>  		rs->printed = 0;
> -		rs->missed  = 0;
>  	}
>  	if (rs->burst && rs->burst > rs->printed) {
>  		rs->printed++;

Re: [PATCH -v3.2 1/2] ratelimit: Extend to print suppressed messages on release

[Borislav Petkov]

On Wed, Jul 06, 2016 at 09:17:52PM -0400, Steven Rostedt wrote:
> Hmm, should this clear the missed flag? Especially since it isn't
> cleared below.

The expectation is that after you call exit on something, you don't need
it anymore. But I know exactly why you're asking for this so I'll do the
change. :-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.