From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932084AbcGNUXd (ORCPT <rfc822;w@1wt.eu>);
	Thu, 14 Jul 2016 16:23:33 -0400
Received: from mail.linuxfoundation.org ([140.211.169.12]:41622 "EHLO
	mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751880AbcGNUX3 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 14 Jul 2016 16:23:29 -0400
Date: Thu, 14 Jul 2016 13:23:23 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Borislav Petkov <bp@alien8.de>
Cc: LKML <linux-kernel@vger.kernel.org>, Franck Bui <fbui@suse.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Ingo Molnar <mingo@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= 
	<u.kleine-koenig@pengutronix.de>
Subject: Re: [PATCH -v4 2/2] printk: Add kernel parameter to control writes
 to /dev/kmsg
Message-Id: <20160714132323.25e6140ade78fbce622a6381@linux-foundation.org>
In-Reply-To: <1467969530-5215-3-git-send-email-bp@alien8.de>
References: <1467969530-5215-1-git-send-email-bp@alien8.de>
	<1467969530-5215-3-git-send-email-bp@alien8.de>
X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri,  8 Jul 2016 11:18:50 +0200 Borislav Petkov <bp@alien8.de> wrote:

> 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.

Which changes current kernel behaviour.  Can we please get some
discussion into this changelog which justifies that decision?  What's
the reasoning and how do we know it won't break existing stuff?

> 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.

Please also include a description of the reasoning behind this design
decision.

> 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>

Did nobody ack any of this?

>  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.devkmsg=ratelimit is undocumented?

>  	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

hm.  If we're going to make this a pain old 0/1/2 then perhaps the boot
parameter should also simply accept 0/1/2.

> +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.

Why?

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

Please put these forward declarations near top-of-file.  Otherwise we
can later get duplicates.

> +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);
>
> ...
>
> +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. */

Please enhance the comment to describe why this is being done.  The
reasoning behind it.

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