Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1176482 > unrolled thread

[RFC 0/8] Additional kmsg devices

Back to article view | Back to linux.kernel

Contents

  [RFC 0/8] Additional kmsg devices Marcin Niesluchowski <m.niesluchow@samsung.com> - 2015-07-03 13:00 +0200
    [RFC 4/8] kmsg: add function for adding and deleting additional buffers Marcin Niesluchowski <m.niesluchow@samsung.com> - 2015-07-03 13:00 +0200
    [RFC 8/8] kmsg: add ioctl for kmsg* devices operating on buffers Marcin Niesluchowski <m.niesluchow@samsung.com> - 2015-07-03 13:00 +0200
    Re: [RFC 0/8] Additional kmsg devices Marcin Niesluchowski <m.niesluchow@samsung.com> - 2015-07-03 17:10 +0200
      Re: [RFC 0/8] Additional kmsg devices Andy Lutomirski <luto@amacapital.net> - 2015-07-03 19:00 +0200
      Re: [RFC 0/8] Additional kmsg devices Karol Lewandowski <k.lewandowsk@samsung.com> - 2015-07-07 19:20 +0200

#1176482 — [RFC 0/8] Additional kmsg devices

Dear All,

This series of patches extends kmsg interface with ability to dynamicaly
create (and destroy) kmsg-like devices which can be used by user space
for logging. Logging to kernel has number of benefits, including but not
limited to - always available, requiring no userspace, automatically
rotating and low overhead.

User-space logging to kernel cyclic buffers was already successfully used
in android logger concept but it had certain flaws that this commits try
to address:
* drops hardcoded number of devices and static paths in favor for dynamic
  configuration by ioctl interface in userspace
* extends existing driver instead of creating completely new one

Those patches apply on branch 'master',
(commit 9bdc771f2c29a11920f477fba05a58e23ee42554):
  git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

Marcin Niesluchowski (8):
  printk: move code regarding log message storing format
  printk: add one function for storing log in proper format
  kmsg: introduce additional kmsg devices support
  kmsg: add function for adding and deleting additional buffers
  kmsg: device support in mem class
  kmsg: add predefined _PID, _TID, _COMM keywords to kmsg* log dict
  kmsg: add ioctl for adding and deleting kmsg* devices
  kmsg: add ioctl for kmsg* devices operating on buffers

 Documentation/ioctl/ioctl-number.txt |    1 +
 drivers/char/mem.c                   |  154 +++-
 fs/proc/kmsg.c                       |    4 +-
 include/linux/printk.h               |    6 +
 include/uapi/linux/Kbuild            |    1 +
 include/uapi/linux/kmsg_ioctl.h      |   45 ++
 kernel/printk/printk.c               | 1361 ++++++++++++++++++++++------------
 7 files changed, 1087 insertions(+), 485 deletions(-)
 create mode 100644 include/uapi/linux/kmsg_ioctl.h

-- 

Best Regards,
Marcin Niesluchowski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1176484 — [RFC 4/8] kmsg: add function for adding and deleting additional buffers

Additional kmsg buffers should be created and deleted dynamically.

Adding two functions
* kmsg_sys_buffer_add() creates additional kmsg buffer returning minor
* kmsg_sys_buffer_del() deletes one based on provided minor

Signed-off-by: Marcin Niesluchowski <m.niesluchow@samsung.com>
---
 include/linux/printk.h |   3 ++
 kernel/printk/printk.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 122 insertions(+), 3 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index d3b5f23..5806982 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -422,6 +422,9 @@ extern void init_kmsg_minor(int minor);
 
 extern int kmsg_sys_mode(int minor, umode_t *mode);
 
+extern int kmsg_sys_buffer_add(size_t size, umode_t mode);
+extern void kmsg_sys_buffer_del(int minor);
+
 enum {
 	DUMP_PREFIX_NONE,
 	DUMP_PREFIX_ADDRESS,
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 7f30c8b..abe78c1 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -46,6 +46,9 @@
 #include <linux/utsname.h>
 #include <linux/ctype.h>
 #include <linux/uio.h>
+#include <linux/slab.h>
+#include <linux/kref.h>
+#include <linux/kdev_t.h>
 
 #include <asm/uaccess.h>
 
@@ -240,6 +243,7 @@ struct log_buffer {
 	char *buf;		/* cyclic log buffer */
 	u32 len;		/* buffer length */
 	wait_queue_head_t wait;	/* wait queue for kmsg buffer */
+	struct kref refcount;	/* refcount for kmsg_sys buffers */
 #endif
 /*
  * The lock protects kmsg buffer, indices, counters. This can be taken within
@@ -292,6 +296,7 @@ static struct log_buffer log_buf = {
 	.len		= __LOG_BUF_K_LEN,
 	.lock		= __RAW_SPIN_LOCK_UNLOCKED(log_buf.lock),
 	.wait		= __WAIT_QUEUE_HEAD_INITIALIZER(log_buf.wait),
+	.refcount	= { .refcount = { .counter = 0 } },
 	.first_seq	= 0,
 	.first_idx	= 0,
 	.next_seq	= 0,
@@ -862,6 +867,15 @@ struct devkmsg_user {
 	char buf[CONSOLE_EXT_LOG_MAX];
 };
 
+void log_buf_release(struct kref *ref)
+{
+	struct log_buffer *log_b = container_of(ref, struct log_buffer,
+						refcount);
+
+	kfree(log_b->buf);
+	kfree(log_b);
+}
+
 static int kmsg_sys_write(int minor, int level, const char *fmt, ...)
 {
 	va_list args;
@@ -969,10 +983,24 @@ static ssize_t kmsg_read(struct log_buffer *log_b, struct file *file,
 		}
 
 		raw_spin_unlock_irq(&log_b->lock);
-		ret = wait_event_interruptible(log_b->wait,
-					       user->seq != log_b->next_seq);
+
+		if (log_b == &log_buf) {
+			ret = wait_event_interruptible(log_b->wait,
+						user->seq != log_b->next_seq);
+		} else {
+			rcu_read_unlock();
+			kref_get(&log_b->refcount);
+			ret = wait_event_interruptible(log_b->wait,
+						user->seq != log_b->next_seq);
+			if (log_b->minor == -1)
+				ret = -ENXIO;
+			if (kref_put(&log_b->refcount, log_buf_release))
+				ret = -ENXIO;
+			rcu_read_lock();
+		}
 		if (ret)
 			goto out;
+
 		raw_spin_lock_irq(&log_b->lock);
 	}
 
@@ -1140,8 +1168,14 @@ static unsigned int devkmsg_poll(struct file *file, poll_table *wait)
 	rcu_read_lock();
 	list_for_each_entry_rcu(log_b, &log_buf.list, list) {
 		if (log_b->minor == minor) {
+			kref_get(&log_b->refcount);
+			rcu_read_unlock();
+
 			ret = kmsg_poll(log_b, file, wait);
-			break;
+
+			if (kref_put(&log_b->refcount, log_buf_release))
+				return POLLERR|POLLNVAL;
+			return ret;
 		}
 	}
 	rcu_read_unlock();
@@ -1242,6 +1276,88 @@ int kmsg_sys_mode(int minor, umode_t *mode)
 	return ret;
 }
 
+static DEFINE_SPINLOCK(kmsg_sys_list_lock);
+
+int kmsg_sys_buffer_add(size_t size, umode_t mode)
+{
+	unsigned long flags;
+	int minor = log_buf.minor;
+	struct log_buffer *log_b;
+	struct log_buffer *log_b_new;
+
+	if (size < LOG_LINE_MAX + PREFIX_MAX)
+		return -EINVAL;
+
+	log_b_new = kzalloc(sizeof(struct log_buffer), GFP_KERNEL);
+	if (!log_b_new)
+		return -ENOMEM;
+
+	log_b_new->buf = kmalloc(size, GFP_KERNEL);
+	if (!log_b_new->buf) {
+		kfree(log_b_new);
+		return -ENOMEM;
+	}
+
+	log_b_new->len = size;
+	log_b_new->lock = __RAW_SPIN_LOCK_UNLOCKED(log_b_new->lock);
+	init_waitqueue_head(&log_b_new->wait);
+	kref_init(&log_b_new->refcount);
+	log_b_new->mode = mode;
+
+	kref_get(&log_b_new->refcount);
+
+	spin_lock_irqsave(&kmsg_sys_list_lock, flags);
+
+	list_for_each_entry(log_b, &log_buf.list, list) {
+		if (log_b->minor - minor > 1)
+			break;
+
+		minor = log_b->minor;
+	}
+
+	if (!(minor & MINORMASK)) {
+		kref_put(&log_b->refcount, log_buf_release);
+		spin_unlock_irqrestore(&kmsg_sys_list_lock, flags);
+		return -ERANGE;
+	}
+
+	minor += 1;
+	log_b_new->minor = minor;
+
+	list_add_tail_rcu(&log_b_new->list, &log_b->list);
+
+	spin_unlock_irqrestore(&kmsg_sys_list_lock, flags);
+
+	return minor;
+}
+
+void kmsg_sys_buffer_del(int minor)
+{
+	unsigned long flags;
+	struct log_buffer *log_b;
+
+	spin_lock_irqsave(&kmsg_sys_list_lock, flags);
+
+	list_for_each_entry(log_b, &log_buf.list, list) {
+		if (log_b->minor == minor)
+			break;
+	}
+
+	if (log_b == &log_buf) {
+		spin_unlock_irqrestore(&kmsg_sys_list_lock, flags);
+		return;
+	}
+
+	list_del_rcu(&log_b->list);
+
+	spin_unlock_irqrestore(&kmsg_sys_list_lock, flags);
+
+	log_b->minor = -1;
+	wake_up_interruptible(&log_b->wait);
+
+	kref_put(&log_b->refcount, log_buf_release);
+}
+
 #ifdef CONFIG_KEXEC
 /*
  * This appends the listed symbols to /proc/vmcore
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176488 — [RFC 8/8] kmsg: add ioctl for kmsg* devices operating on buffers

There is no possibility to clear additional kmsg buffers,
get size of them or know what size should be passed to read
file operation (too small size causes it to retrun -EINVAL).

Add following ioctls which solve those issues:
* KMSG_CMD_GET_BUF_SIZE
* KMSG_CMD_GET_READ_SIZE_MAX
* KMSG_CMD_CLEAR

Signed-off-by: Marcin Niesluchowski <m.niesluchow@samsung.com>
---
 Documentation/ioctl/ioctl-number.txt |   2 +-
 include/uapi/linux/kmsg_ioctl.h      |  15 +++++
 kernel/printk/printk.c               | 103 ++++++++++++++++++++++++++---------
 3 files changed, 93 insertions(+), 27 deletions(-)

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 26c0e53..4b5f715 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -312,7 +312,7 @@ Code  Seq#(hex)	Include File		Comments
 					<mailto:vgo@ratio.de>
 0xB1	00-1F	PPPoX			<mailto:mostrows@styx.uwaterloo.ca>
 0xB3	00	linux/mmc/ioctl.h
-0xBB	00-02	uapi/linux/kmsg_ioctl.h
+0xBB	00-83	uapi/linux/kmsg_ioctl.h
 0xC0	00-0F	linux/usb/iowarrior.h
 0xCA	00-0F	uapi/misc/cxl.h
 0xCB	00-1F	CBM serial IEC bus	in development:
diff --git a/include/uapi/linux/kmsg_ioctl.h b/include/uapi/linux/kmsg_ioctl.h
index 89c0c61..2389d9f 100644
--- a/include/uapi/linux/kmsg_ioctl.h
+++ b/include/uapi/linux/kmsg_ioctl.h
@@ -27,4 +27,19 @@ struct kmsg_cmd_buffer_add {
 					      struct kmsg_cmd_buffer_add)
 #define KMSG_CMD_BUFFER_DEL		_IOW(KMSG_IOCTL_MAGIC, 0x01, int)
 
+/*
+ * A ioctl interface for kmsg* devices.
+ *
+ * KMSG_CMD_GET_BUF_SIZE:	Retrieve cyclic log buffer size associated with
+ *				device.
+ * KMSG_CMD_GET_READ_SIZE_MAX:	Retrieve max size of data read by kmsg read
+ *				operation.
+ * KMSG_CMD_CLEAR:		Clears cyclic log buffer. After that operation
+ *				there is no data to read from buffer unless
+ *				logs are written.
+ */
+#define KMSG_CMD_GET_BUF_SIZE		_IOR(KMSG_IOCTL_MAGIC, 0x80, __u32)
+#define KMSG_CMD_GET_READ_SIZE_MAX	_IOR(KMSG_IOCTL_MAGIC, 0x81, __u32)
+#define KMSG_CMD_CLEAR			_IO(KMSG_IOCTL_MAGIC, 0x82)
+
 #endif
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2a7f6a4..740ba79 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -49,6 +49,7 @@
 #include <linux/slab.h>
 #include <linux/kref.h>
 #include <linux/kdev_t.h>
+#include <linux/kmsg_ioctl.h>
 
 #include <asm/uaccess.h>
 
@@ -257,6 +258,10 @@ struct log_buffer {
 	u64 next_seq;
 #ifdef CONFIG_PRINTK
 	u32 next_idx;		/* index of the next record to store */
+/* sequence number of the next record to read after last 'clear' command */
+	u64 clear_seq;
+/* index of the next record to read after last 'clear' command */
+	u32 clear_idx;
 	int mode;		/* mode of device (kmsg_sys only) */
 	int minor;		/* minor representing buffer device */
 #endif
@@ -274,10 +279,6 @@ static u64 console_seq;
 static u32 console_idx;
 static enum log_flags console_prev;
 
-/* the next printk record to read after the last 'clear' command */
-static u64 clear_seq;
-static u32 clear_idx;
-
 #define PREFIX_MAX		32
 #define LOG_LINE_MAX		(1024 - PREFIX_MAX)
 
@@ -301,6 +302,8 @@ static struct log_buffer log_buf = {
 	.first_idx	= 0,
 	.next_seq	= 0,
 	.next_idx	= 0,
+	.clear_seq	= 0,
+	.clear_idx	= 0,
 	.mode		= 0,
 	.minor		= 0,
 };
@@ -1112,18 +1115,14 @@ static loff_t kmsg_llseek(struct log_buffer *log_b, struct file *file,
 		user->seq = log_b->first_seq;
 		break;
 	case SEEK_DATA:
-		/* no clear index for kmsg_sys buffers */
-		if (log_b != &log_buf) {
-			ret = -EINVAL;
-			break;
-		}
 		/*
 		 * The first record after the last SYSLOG_ACTION_CLEAR,
-		 * like issued by 'dmesg -c'. Reading /dev/kmsg itself
-		 * changes no global state, and does not clear anything.
+		 * like issued by 'dmesg -c' or KMSG_CMD_CLEAR ioctl
+		 * command. Reading /dev/kmsg itself changes no global
+		 * state, and does not clear anything.
 		 */
-		user->idx = clear_idx;
-		user->seq = clear_seq;
+		user->idx = log_b->clear_idx;
+		user->seq = log_b->clear_seq;
 		break;
 	case SEEK_END:
 		/* after the last record */
@@ -1263,6 +1262,56 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	return ret;
 }
 
+static long kmsg_ioctl(struct log_buffer *log_b, unsigned int cmd,
+		       unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	static const u32 read_size_max = CONSOLE_EXT_LOG_MAX;
+
+	switch (cmd) {
+	case KMSG_CMD_GET_BUF_SIZE:
+		if (copy_to_user(argp, &log_b->len, sizeof(u32)))
+			return -EFAULT;
+		break;
+	case KMSG_CMD_GET_READ_SIZE_MAX:
+		if (copy_to_user(argp, &read_size_max, sizeof(u32)))
+			return -EFAULT;
+		break;
+	case KMSG_CMD_CLEAR:
+		if (!capable(CAP_SYSLOG))
+			return -EPERM;
+		raw_spin_lock_irq(&log_b->lock);
+		log_b->clear_seq = log_b->next_seq;
+		log_b->clear_idx = log_b->next_idx;
+		raw_spin_unlock_irq(&log_b->lock);
+		break;
+	default:
+		return -ENOTTY;
+	}
+	return 0;
+}
+
+static long devkmsg_ioctl(struct file *file, unsigned int cmd,
+			  unsigned long arg)
+{
+	long ret = -ENXIO;
+	int minor = iminor(file->f_inode);
+	struct log_buffer *log_b;
+
+	if (minor == log_buf.minor)
+		return kmsg_ioctl(&log_buf, cmd, arg);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(log_b, &log_buf.list, list) {
+		if (log_b->minor == minor) {
+			ret = kmsg_ioctl(log_b, cmd, arg);
+			break;
+		}
+	}
+	rcu_read_unlock();
+	return ret;
+}
+
 static int devkmsg_release(struct inode *inode, struct file *file)
 {
 	struct devkmsg_user *user = file->private_data;
@@ -1281,6 +1330,8 @@ const struct file_operations kmsg_fops = {
 	.write_iter = devkmsg_write,
 	.llseek = devkmsg_llseek,
 	.poll = devkmsg_poll,
+	.unlocked_ioctl = devkmsg_ioctl,
+	.compat_ioctl = devkmsg_ioctl,
 	.release = devkmsg_release,
 };
 
@@ -1747,18 +1798,18 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
 		u32 idx;
 		enum log_flags prev;
 
-		if (clear_seq < log_buf.first_seq) {
+		if (log_buf.clear_seq < log_buf.first_seq) {
 			/* messages are gone, move to first available one */
-			clear_seq = log_buf.first_seq;
-			clear_idx = log_buf.first_idx;
+			log_buf.clear_seq = log_buf.first_seq;
+			log_buf.clear_idx = log_buf.first_idx;
 		}
 
 		/*
 		 * Find first record that fits, including all following records,
 		 * into the user-provided buffer for this dump.
 		 */
-		seq = clear_seq;
-		idx = clear_idx;
+		seq = log_buf.clear_seq;
+		idx = log_buf.clear_idx;
 		prev = 0;
 		while (seq < log_buf.next_seq) {
 			struct printk_log *msg = log_from_idx(&log_buf, idx);
@@ -1770,8 +1821,8 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
 		}
 
 		/* move first record forward until length fits into the buffer */
-		seq = clear_seq;
-		idx = clear_idx;
+		seq = log_buf.clear_seq;
+		idx = log_buf.clear_idx;
 		prev = 0;
 		while (len > size && seq < log_buf.next_seq) {
 			struct printk_log *msg = log_from_idx(&log_buf, idx);
@@ -1817,8 +1868,8 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
 	}
 
 	if (clear) {
-		clear_seq = log_buf.next_seq;
-		clear_idx = log_buf.next_idx;
+		log_buf.clear_seq = log_buf.next_seq;
+		log_buf.clear_idx = log_buf.next_idx;
 	}
 	raw_spin_unlock_irq(&log_buf.lock);
 
@@ -3199,8 +3250,8 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 		dumper->active = true;
 
 		raw_spin_lock_irqsave(&log_buf.lock, flags);
-		dumper->cur_seq = clear_seq;
-		dumper->cur_idx = clear_idx;
+		dumper->cur_seq = log_buf.clear_seq;
+		dumper->cur_idx = log_buf.clear_idx;
 		dumper->next_seq = log_buf.next_seq;
 		dumper->next_idx = log_buf.next_idx;
 		raw_spin_unlock_irqrestore(&log_buf.lock, flags);
@@ -3406,8 +3457,8 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer);
  */
 void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper)
 {
-	dumper->cur_seq = clear_seq;
-	dumper->cur_idx = clear_idx;
+	dumper->cur_seq = log_buf.clear_seq;
+	dumper->cur_idx = log_buf.clear_idx;
 	dumper->next_seq = log_buf.next_seq;
 	dumper->next_idx = log_buf.next_idx;
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176649

On 07/03/2015 01:21 PM, Richard Weinberger wrote:
> On Fri, Jul 3, 2015 at 12:49 PM, Marcin Niesluchowski
> <m.niesluchow@samsung.com> wrote:
>> Dear All,
>>
>> This series of patches extends kmsg interface with ability to dynamicaly
>> create (and destroy) kmsg-like devices which can be used by user space
>> for logging. Logging to kernel has number of benefits, including but not
>> limited to - always available, requiring no userspace, automatically
>> rotating and low overhead.
>>
>> User-space logging to kernel cyclic buffers was already successfully used
>> in android logger concept but it had certain flaws that this commits try
>> to address:
>> * drops hardcoded number of devices and static paths in favor for dynamic
>>    configuration by ioctl interface in userspace
>> * extends existing driver instead of creating completely new one
> So, now we start moving syslogd into kernel land because userspace is
> too broken to provide
> decent logging?
>
> I can understand the systemd is using kmsg if no other logging service
> is available
> but I really don't think we should encourage other programs to do so.
>
> Why can't you just make sure that your target has a working
> syslogd/rsyslogd/journald/whatever?
> All can be done perfectly fine in userspace.
* Message credibility: Lets imagine simple service which collects logs 
via unix sockets. There is no reliable way of identifying logging 
process. getsockopt() with SO_PEERCRED option would give pid form cred 
structure, but according to manual it may not be of actual logging process:
   "The returned credentials are those that were in effect at the time 
of the call to connect(2) or socketpair(2)."
       - select(7)

* Early userspace tool: Helpful especially for embeded systems.

* Reliability: Userspace service may be killed due to out of memory 
(OOM). This is kernel cyclic buffer, which size can be specified 
differently according to situation.

* Possibility of using it with pstore: This code could be extended to 
log additional buffers to persistent storage same way main (kmsg) log 
buffer is.

* Use case of attaching file descriptor to stdout/stderr: Especially in 
early userspace.

* Performance: Those services mentioned by You are weeker solutions in 
that case. Especially systemd-journald is much too heavy soulution.

-- 

Best Regards,
Marcin Niesluchowski

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176721

On Fri, Jul 3, 2015 at 8:09 AM, Marcin Niesluchowski
<m.niesluchow@samsung.com> wrote:
>
> * Message credibility: Lets imagine simple service which collects logs via
> unix sockets. There is no reliable way of identifying logging process.
> getsockopt() with SO_PEERCRED option would give pid form cred structure, but
> according to manual it may not be of actual logging process:
>   "The returned credentials are those that were in effect at the time of the
> call to connect(2) or socketpair(2)."
>       - select(7)

There's SCM_CREDENTIALS, which is dangerous, but it's dangerous in
exactly the same way that your patches are dangerous.  You're
collecting PID/TID when write(2) is called, and it's very easy to get
another process to call write(2) on your behalf, because write(2)
isn't supposed to collect credentials.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1178757

On 2015-07-07 15:11, Petr Mladek wrote:
> On Fri 2015-07-03 17:09:03, Marcin Niesluchowski wrote:
>> On 07/03/2015 01:21 PM, Richard Weinberger wrote:
>>> On Fri, Jul 3, 2015 at 12:49 PM, Marcin Niesluchowski
>>> <m.niesluchow@samsung.com> wrote:
>>>> Dear All,
>>>>
>>>> This series of patches extends kmsg interface with ability to dynamicaly
>>>> create (and destroy) kmsg-like devices which can be used by user space
>>>> for logging. Logging to kernel has number of benefits, including but not
>>>> limited to - always available, requiring no userspace, automatically
>>>> rotating and low overhead.
>>>>
>>>> User-space logging to kernel cyclic buffers was already successfully used
>>>> in android logger concept but it had certain flaws that this commits try
>>>> to address:
>>>> * drops hardcoded number of devices and static paths in favor for dynamic
>>>>   configuration by ioctl interface in userspace
>>>> * extends existing driver instead of creating completely new one
>>> So, now we start moving syslogd into kernel land because userspace is
>>> too broken to provide
>>> decent logging?
>>>
>>> I can understand the systemd is using kmsg if no other logging service
>>> is available
>>> but I really don't think we should encourage other programs to do so.
>>>
>>> Why can't you just make sure that your target has a working
>>> syslogd/rsyslogd/journald/whatever?
>>> All can be done perfectly fine in userspace.
>> * Message credibility: Lets imagine simple service which collects
>> logs via unix sockets. There is no reliable way of identifying
>> logging process. getsockopt() with SO_PEERCRED option would give pid
>> form cred structure, but according to manual it may not be of actual
>> logging process:
>>   "The returned credentials are those that were in effect at the
>> time of the call to connect(2) or socketpair(2)."
>>       - select(7)
>>
>> * Early userspace tool: Helpful especially for embeded systems.
>>
>> * Reliability: Userspace service may be killed due to out of memory
>> (OOM). This is kernel cyclic buffer, which size can be specified
>> differently according to situation.
> But then many services will fight for the space in the kernel ring
> buffer.

Yes.   Please note however that problems you describe are also valid for
/dev/kmsg today.

User space has used (one) writeable kmsg for some time already - which
has caused number of interesting problems caused by the fact that
messages from different domains (kernel, userspace) are written to one
place (ie. systemd "debug" problem).  One of the goals is to avoid this
particular problem and let userspace create, destroy and user their own
buffers at will.

> We will need a mechanism to guarantee a space for each service.

We preserve semantics of kmsg, so I don't see why we would need to give
any more guarantees that what was provided there.

> We will need priorities to throttle various services various ways. 

Appropriate buffer size will throttle messages automatically -  I don't
think we need anything more than that.  See also below.

> It will be easier to lost messages. 

Ability to lose messages is one of the goals -  if we exceed buffer size
and there are no one actively reading buffers this is what we want to
happen.

Say we have one buffer where debug messages go - we are not at all
interested in the content and are very happy to lose these... unless in
the case of crash, where we will have all of it dumped to persistent
storage by pstore (this is when content might be interesting and helpful).

This is just one of many possible scenarios.

> It might be harder to get the important messages on the console when
> the system is going down.

Messages from additional buffers are not intended to be written to console.

> It will be harder to handle continuous lines.

I don't see how it would be different from what we have today.

> I am not sure that we want to go this way.

This is why this thread has RFC tag anyway :^)


Thanks

-- 
Karol Lewandowski, Samsung R&D Institute Poland

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web