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

From: Steven Rostedt <rostedt@goodmis.org>
To: Borislav Petkov <bp@alien8.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
	"Andrew Morton" <akpm@linux-foundation.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>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Subject: Re: [PATCH -v3 2/2] printk: Add kernel parameter to control writes to /dev/kmsg
Date: Tue, 5 Jul 2016 17:47:49 -0400	[thread overview]
Message-ID: <20160705174749.351d77d1@gandalf.local.home> (raw)
In-Reply-To: <1467642292-15671-3-git-send-email-bp@alien8.de>

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