[PATCH -v2 0/2] printk.kmsg: Ratelimit it by default

[PATCH -v2 0/2] printk.kmsg: Ratelimit it by default

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Hi all,

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.

Changelog:

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              |  6 +++
 include/linux/ratelimit.h           | 36 ++++++++++++++---
 kernel/printk/printk.c              | 77 +++++++++++++++++++++++++++++++++----
 kernel/sysctl.c                     |  9 +++++
 lib/ratelimit.c                     |  6 ++-
 7 files changed, 139 insertions(+), 15 deletions(-)

-- 
2.7.3

[PATCH -v2 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.

Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 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..a68a56d223c2 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 callbacks suppressed\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

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

[Joe Perches]

On Wed, 2016-06-29 at 11:56 +0200, Borislav Petkov 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.
[]
> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
[]
> @@ -15,6 +19,7 @@ struct ratelimit_state {
>  	int		printed;
>  	int		missed;
>  	unsigned long	begin;
> +	unsigned long	flags;
>  };

There are a lot of ratelimit_state structs used in the kernel.
Every printk like <foo>_ratelimited uses a static one.

It'd likely be better to reduce the struct ratelimit_state
size rather than expand it.

burst and printed could probably become s16 or even s8.
flags could be u8 until necessary to expand.

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

[Borislav Petkov]

On Wed, Jun 29, 2016 at 03:24:44AM -0700, Joe Perches wrote:
> It'd likely be better to reduce the struct ratelimit_state
> size rather than expand it.

Something for a separate patchset when someone feels bored.

-- 
Regards/Gruss,
    Boris.

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

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> +/* issue num suppressed message on exit */
> +#define RATELIMIT_MSG_ON_RELEASE	BIT(0)

So this flag says that we should issue a ratelimit message when it occurs.

> +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 callbacks suppressed\n",
> +		       current->comm, rs->missed);

... here we print the message if the RATELIMIT_MSG_ON_RELEASE bit is set.

> +++ 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);

But here we print the message if the RATELIMIT_MSG_ON_RELEASE bit is zero.
Is that intentional?

Also, while we are changing it, I'd like to suggest a different message - it's 
talking about 'callbacks' but there's no callback here - we are skipping kernel 
log messages. So how about:

	pr_warn("%s: %d kernel log lines skipped, due to rate-limiting.\n"

Thanks,

	Ingo

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

[Borislav Petkov]

On Fri, Jul 01, 2016 at 10:22:47AM +0200, Ingo Molnar wrote:
> > +/* issue num suppressed message on exit */
> > +#define RATELIMIT_MSG_ON_RELEASE	BIT(0)
> 
> So this flag says that we should issue a ratelimit message when it occurs.

This flag says that we should print the ratelimit message when we
release the ratelimit state, i.e., devkmsg_release() for example.

> But here we print the message if the RATELIMIT_MSG_ON_RELEASE bit is zero.
> Is that intentional?

Sure, we want to dump the ratelimited messages only when we release the
ratelimit state.

> Also, while we are changing it, I'd like to suggest a different message - it's 
> talking about 'callbacks' but there's no callback here - we are skipping kernel 
> log messages. So how about:
> 
> 	pr_warn("%s: %d kernel log lines skipped, due to rate-limiting.\n"

Well, I'm not sure: even though the majority of the ratelimit usage is
printing stuff and it has been carved out from printk.c, it still is a
generic facility and you probably want to ratelimit other things too
with it, like polling for something or whatnot.

  [ Whether it is really usable for something else besides ratelimiting
    printk is a whole another question, of course. ]

And ___ratelimit() itself gets a function name as arg so there you
*really* *have* callbacks so %d callbacks suppressed\n" is actually
correct.

I guess I can make the RATELIMIT_MSG_ON_RELEASE message more generic:

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);
}

?

-- 
Regards/Gruss,
    Boris.

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

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jul 01, 2016 at 10:22:47AM +0200, Ingo Molnar wrote:
> > > +/* issue num suppressed message on exit */
> > > +#define RATELIMIT_MSG_ON_RELEASE	BIT(0)
> > 
> > So this flag says that we should issue a ratelimit message when it occurs.
> 
> This flag says that we should print the ratelimit message when we
> release the ratelimit state, i.e., devkmsg_release() for example.
> 
> > But here we print the message if the RATELIMIT_MSG_ON_RELEASE bit is zero.
> > Is that intentional?
> 
> Sure, we want to dump the ratelimited messages only when we release the
> ratelimit state.

ah, I see, I was being dim - never mind!

> > Also, while we are changing it, I'd like to suggest a different message - it's 
> > talking about 'callbacks' but there's no callback here - we are skipping kernel 
> > log messages. So how about:
> > 
> > 	pr_warn("%s: %d kernel log lines skipped, due to rate-limiting.\n"
> 
> Well, I'm not sure: even though the majority of the ratelimit usage is
> printing stuff and it has been carved out from printk.c, it still is a
> generic facility and you probably want to ratelimit other things too
> with it, like polling for something or whatnot.

Yeah, indeed, you are right.

Thanks,

	Ingo

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

[Borislav Petkov]

On Fri, Jul 01, 2016 at 12:17:43PM +0200, Ingo Molnar wrote:
> Yeah, indeed, you are right.

How about this though, still:

ratelimit_state_exit:

        if (rs->missed)
                printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
                       current->comm, rs->missed);


I still think it is better than the "callbacks" thing as we're issuing
it on release and there's not really a @func which callbacks we're
limiting...

-- 
Regards/Gruss,
    Boris.

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

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jul 01, 2016 at 12:17:43PM +0200, Ingo Molnar wrote:
> > Yeah, indeed, you are right.
> 
> How about this though, still:
> 
> ratelimit_state_exit:
> 
>         if (rs->missed)
>                 printk(KERN_WARNING "%s: %d output lines suppressed due to ratelimiting\n",
>                        current->comm, rs->missed);
> 
> 
> I still think it is better than the "callbacks" thing as we're issuing
> it on release and there's not really a @func which callbacks we're
> limiting...

Yeah, indeed!

Thanks,

	Ingo

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

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Add a "printk.kmsg" 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_kmsg={0,1,2} - with numeric values
corresponding to the options above.

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

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 Documentation/kernel-parameters.txt |  6 +++
 Documentation/sysctl/kernel.txt     | 14 +++++++
 include/linux/printk.h              |  6 +++
 kernel/printk/printk.c              | 77 +++++++++++++++++++++++++++++++++----
 kernel/sysctl.c                     |  9 +++++
 5 files changed, 104 insertions(+), 8 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c958d1c..4799c88b7258 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.kmsg={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..7823eb8c714f 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -752,6 +752,20 @@ send before ratelimiting kicks in.
 
 ==============================================================
 
+printk_kmsg:
+
+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.kmsg= overrides this and is a
+one-time setting for the duration of the system lifetime: 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..bcf72e756122 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -171,6 +171,12 @@ 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;
+
+int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
+			      void __user *buffer, 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..9f0a885c2718 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -86,6 +86,48 @@ static struct lockdep_map console_lock_dep_map = {
 };
 #endif
 
+#define DEVKMSG_LOG_RATELIMIT		0
+#define DEVKMSG_LOG_ON			1
+#define DEVKMSG_LOG_OFF			2
+#define DEVKMSG_LOCK			(1 << 8)
+#define DEVKMSG_LOG_MASK		(DEVKMSG_LOCK - 1)
+#define DEVKMSG_LOCKED_MASK		~DEVKMSG_LOG_MASK
+
+/* DEVKMSG_LOG_RATELIMIT by default */
+unsigned int __read_mostly devkmsg_log;
+static int __init control_devkmsg(char *str)
+{
+	if (!str)
+		return -EINVAL;
+
+	if (!strncmp(str, "on", 2))
+		devkmsg_log = DEVKMSG_LOG_ON;
+	else if (!strncmp(str, "off", 3))
+		devkmsg_log = DEVKMSG_LOG_OFF;
+	else if (!strncmp(str, "ratelimit", 9))
+		devkmsg_log = DEVKMSG_LOG_RATELIMIT;
+	else
+		return -EINVAL;
+
+	/* Sysctl cannot change it anymore. */
+	devkmsg_log |= DEVKMSG_LOCK;
+
+	return 0;
+}
+__setup("printk.kmsg=", 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_LOCKED_MASK) {
+		if (write)
+			return -EINVAL;
+	}
+
+	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+}
+
 /*
  * Number of registered extended console drivers.
  *
@@ -614,6 +656,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 +666,25 @@ 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) == DEVKMSG_LOG_OFF)
+		return len;
+
+	/* Ratelimit when not explicitly enabled or when we're not booting. */
+	if ((system_state != SYSTEM_BOOTING) &&
+	    ((devkmsg_log & DEVKMSG_LOG_MASK) != DEVKMSG_LOG_ON)) {
+		if (!___ratelimit(&user->rs, current->comm))
+			return ret;
+	}
+
 	buf = kmalloc(len+1, GFP_KERNEL);
 	if (buf == NULL)
 		return -ENOMEM;
@@ -801,18 +858,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 +890,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..a29d6c4fa86c 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -814,6 +814,15 @@ static struct ctl_table kern_table[] = {
 		.extra2		= &ten_thousand,
 	},
 	{
+		.procname	= "printk_kmsg",
+		.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 -v2 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> +printk_kmsg:
> +
> +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.kmsg= overrides this and is a
> +one-time setting for the duration of the system lifetime: once set, it
> +cannot be changed by this sysctl interface anymore.

Nit:

s/
 is a one-time setting for the duration of the system lifetime: once set, it ...
/
 is a one-time setting until the next reboot: once set, it ...

As I really hope this bit is not burned permanently into the system hardware! ;-)

> +++ b/include/linux/printk.h
> @@ -171,6 +171,12 @@ 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;
> +
> +int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> +			      void __user *buffer, size_t *lenp, loff_t *ppos);
>  
>  extern void wake_up_klogd(void);

Nit: I'd make devkmsg_sysctl_set_loglvl() extern as well, to stay consistent with 
the surrounding prototypes.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -86,6 +86,48 @@ static struct lockdep_map console_lock_dep_map = {
>  };
>  #endif
>  
> +#define DEVKMSG_LOG_RATELIMIT		0
> +#define DEVKMSG_LOG_ON			1
> +#define DEVKMSG_LOG_OFF			2
> +#define DEVKMSG_LOCK			(1 << 8)
> +#define DEVKMSG_LOG_MASK		(DEVKMSG_LOCK - 1)
> +#define DEVKMSG_LOCKED_MASK		~DEVKMSG_LOG_MASK

Hm, so the state definitions and names here look a bit confusing to me, got a 
headache trying to sort through them!

So from a UI point of view, what we want to have is 5 levels:

	permanent-off
	off
	ratelimit
	on
	permanent-on

Right?

You implemented the 'permanent' bit via an independent LOCK bit in the flags 
state. This leaves us:

	off
	ratelimit
	on

... which you implemented via giving two independent bits to 'off' and 'on', and 
if neither is set that means 'ratelimit', right?

So the most robust way to define such bitfields is via a pattern like this:

enum devkmsg_log_bits {
	__DEVKMSG_LOG_BIT_ON,
	__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

The double underscore prefixes are there to make sure we never use the bit numbers 
directly.

And then all the code becomes pretty self-explanatory IMHO:

> +/* DEVKMSG_LOG_RATELIMIT by default */
> +unsigned int __read_mostly devkmsg_log;

   unsigned int __read_mostly devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;

Note that this initialization would survive any future change of the default.

> +static int __init control_devkmsg(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +
> +	if (!strncmp(str, "on", 2))
> +		devkmsg_log = DEVKMSG_LOG_ON;
> +	else if (!strncmp(str, "off", 3))
> +		devkmsg_log = DEVKMSG_LOG_OFF;
> +	else if (!strncmp(str, "ratelimit", 9))
> +		devkmsg_log = DEVKMSG_LOG_RATELIMIT;
> +	else
> +		return -EINVAL;
> +
> +	/* Sysctl cannot change it anymore. */
> +	devkmsg_log |= DEVKMSG_LOCK;
> +
> +	return 0;
> +}
> +__setup("printk.kmsg=", control_devkmsg);

This would become something like:

	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 = 0;
	else
		return -EINVAL;

	/* Sysctl cannot change it anymore: */
	devkmsg_log |= DEVKMSG_LOG_MASK_LOCK;

> +
> +int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> +			      void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	if (devkmsg_log & DEVKMSG_LOCKED_MASK) {
> +		if (write)
> +			return -EINVAL;
> +	}
> +
> +	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> +}

This could be written as:

	if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK) {


> +	/* Ignore when user logging is disabled. */
> +	if ((devkmsg_log & DEVKMSG_LOG_MASK) == DEVKMSG_LOG_OFF)
> +		return len;

This could be written more clearly as:

	if (devkmsg_log & DEVKMSG_LOG_MASK_OFF)
		return len;

... note how we don't have to be careful about masking out the locked bit: we just 
query the 'off' state bit.

> +	/* Ratelimit when not explicitly enabled or when we're not booting. */
> +	if ((system_state != SYSTEM_BOOTING) &&
> +	    ((devkmsg_log & DEVKMSG_LOG_MASK) != DEVKMSG_LOG_ON)) {
> +		if (!___ratelimit(&user->rs, current->comm))
> +			return ret;
> +	}

... and this could be written as:

	if ((system_state != SYSTEM_BOOTING) &&
	    (!(devkmsg_log & DEVKMSG_LOG_MASK_ON)) {

which too is a bit more robust: we already know that user logging is not disabled, 
now we check whether it's always-on or ratelimited.

> +		.procname	= "printk_kmsg",
> +		.data		= &devkmsg_log,

So I liked the 'devkmsg_log' name, because it clearly tells us that this is about 
/dev/kmsg logging state. So I think the visible UI name should be similar:

		.procname	= "printk_devkmsg",
		.data		= &devkmsg_log,

... this way people who know the 'devkmsg' string would know what to grep for in 
the kernel source - or what flag to search for in /proc/sys/kernel/.

Thanks,

	Ingo

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

[Borislav Petkov]

On Fri, Jul 01, 2016 at 11:04:13AM +0200, Ingo Molnar wrote:

...

> So the most robust way to define such bitfields is via a pattern like this:
> 
> enum devkmsg_log_bits {
> 	__DEVKMSG_LOG_BIT_ON,
> 	__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),

Agreed with so far, I'd only drop the "_MASK" thing and make it even
easier on the eyes:

enum devkmsg_log_state {
	DEVKMSG_LOG_ON		= BIT(__DEVKMSG_LOG_BIT_ON),
	DEVKMSG_LOG_OFF		= BIT(__DEVKMSG_LOG_BIT_OFF),
	DEVKMSG_LOCK		= BIT(__DEVKMSG_LOG_BIT_LOCK),
};

Now this makes the checking code even more readable!

	if (devkmsg_log & DEVKMSG_LOCK) {

and

	if (devkmsg_log & DEVKMSG_LOG_OFF)

and

	if ((system_state != SYSTEM_BOOTING) && (!(devkmsg_log & DEVKMSG_LOG_ON)) {


And this way you can read it straight-away as english text.

:-)))

Thanks.

-- 
Regards/Gruss,
    Boris.

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

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jul 01, 2016 at 11:04:13AM +0200, Ingo Molnar wrote:
> 
> ...
> 
> > So the most robust way to define such bitfields is via a pattern like this:
> > 
> > enum devkmsg_log_bits {
> > 	__DEVKMSG_LOG_BIT_ON,
> > 	__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),
> 
> Agreed with so far, I'd only drop the "_MASK" thing and make it even
> easier on the eyes:
> 
> enum devkmsg_log_state {
> 	DEVKMSG_LOG_ON		= BIT(__DEVKMSG_LOG_BIT_ON),
> 	DEVKMSG_LOG_OFF		= BIT(__DEVKMSG_LOG_BIT_OFF),
> 	DEVKMSG_LOCK		= BIT(__DEVKMSG_LOG_BIT_LOCK),
> };

It's just a nit, but generally it's nice to know the character of such values - 
i.e. in case that it's a bit mask that has to be used with bit ops. That's more 
important IMHO than brevity. This means that possibly buggy code like this stands 
out immediately:

	if (devkmgs_log == DEVKMSG_LOG_MASK_ON)

while this one:

	if (devkmgs_log == DEVKMSG_LOG_ON)

might slip through.

But no strong feelings either way!

Thanks,

	Ingo