RFC: Containerized syslog (Take II)

Containerized syslog is now part of nsproxy. A new flag CLONE_SYSLOG allow to unshare syslog area.
Main containerized syslog purpose is to allow full container not to leak or compromise hosts syslog data.

include/linux/init_task.h +1 -0

index abec69b..30b479e 100644
@@ -11,6 +11,7 @@
 #include <linux/user_namespace.h>
 #include <linux/securebits.h>
 #include <net/net_namespace.h>
+#include <linux/syslog.h>
 
 extern struct files_struct init_files;
 extern struct fs_struct init_fs;

include/linux/user_namespace.h +0 -1

index 3d0c73e..cc4f453 100644
@@ -14,7 +14,6 @@ struct user_namespace {
 	struct hlist_head	uidhash_table[UIDHASH_SZ];
 	struct user_struct	*creator;
 	struct work_struct	destroyer;
-	struct syslog_ns        *syslog;
 };
 
 extern struct user_namespace init_user_ns;

kernel/fork.c +0 -0

index f88bd98..38c8d8c 100644

kernel/fork.c +0 -0

 		goto bad_unshare_out;
 
 	/*

kernel/nsproxy.c +12 -2

index 09b4ff9..ff968db 100644
@@ -44,6 +44,8 @@ static inline struct nsproxy *create_nsproxy(void)
 static struct nsproxy *create_new_namespaces(unsigned long flags,
 			struct task_struct *tsk, struct fs_struct *new_fs)
 {
+#define	CONTAINER_BUF_LEN	4096	/*should be enough for container syslog	*/
+
 	struct nsproxy *new_nsp;
 	int err;
 

@@ -80,9 +82,17 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
 		err = PTR_ERR(new_nsp->net_ns);
 		goto out_net;
 	}
-
+	new_nsp->syslog_ns = copy_syslog_ns(flags, tsk->nsproxy->syslog_ns);
+	if (IS_ERR(new_nsp->syslog_ns)) {
+		err = PTR_ERR(new_nsp->syslog_ns);
+		goto out_syslog;
+	}
+	
 	return new_nsp;
 
+out_syslog:
+	if (new_nsp->net_ns) 
+		put_net(new_nsp->net_ns);
 out_net:
 	if (new_nsp->pid_ns)
 		put_pid_ns(new_nsp->pid_ns);

@@ -116,7 +126,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
 	get_nsproxy(old_ns);
 
 	if (!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
-				CLONE_NEWPID | CLONE_NEWNET)))
+				CLONE_NEWPID | CLONE_NEWNET | CLONE_SYSLOG)))
 		return 0;
 
 	if (!capable(CAP_SYS_ADMIN)) {

kernel/nsproxy.c +0 -0

 		return 0;
 
 	if (!capable(CAP_SYS_ADMIN))

kernel/printk.c +1 -1

index fd0a05c..3c7f213 100644
@@ -148,7 +148,7 @@ static int saved_console_loglevel = -1;
  */
 void log_buf_kexec_setup(void)
 {
-	struct syslog_ns *syslog_ns = syslog_get_current();
+	struct syslog_ns *syslog_ns = get_current_syslog_ns();
 
 	VMCOREINFO_SYMBOL(sys_log_buf);
 	VMCOREINFO_SYMBOL(sys_log_end);

kernel/printk.c +2 -2

 	}
 	return 1;
 }

@@ -244,7 +244,7 @@ int do_syslog(int type, char __user *buf, int len)
 	int do_clear = 0;
 	char c;
 	int error = 0;
-	struct syslog_ns *syslog_ns = syslog_get_current();
+	struct syslog_ns *syslog_ns = get_current_syslog_ns();
 
 	error = security_syslog(type);
 	if (error)

@@ -638,7 +638,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 {
 	int printed_len = 0;
 	int current_log_level = default_message_loglevel;
-	struct syslog_ns *syslog_ns = syslog_get_current();
+	struct syslog_ns *syslog_ns = get_current_syslog_ns();
 	unsigned long flags;
 	int this_cpu;
 	char *p;

kernel/printk.c +1 -1

 		spin_lock_irqsave(&sys_log_lock, flags);
 		sys_log_con_start = sys_log_start;

@@ -1462,7 +1462,7 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 	const char *s1, *s2;
 	unsigned long l1, l2;
 	unsigned long flags;
-	struct syslog_ns *syslog_ns = syslog_get_current();
+	struct syslog_ns *syslog_ns = get_current_syslog_ns();
 
 	/* Theoretically, the log could move on after we do this, but
 	   there's not a lot we can do about that. The new messages

kernel/syslog.c +0 -0

index 69d30a9..0088a85 100644
@@ -22,35 +22,66 @@
  *
  */

kernel/syslog.c +6 -0

 #define __LOG_BUF_LEN   (1 << CONFIG_LOG_BUF_SHIFT)
 
 static char __log_buf[__LOG_BUF_LEN];
 
 struct syslog_ns init_kernel_syslog_ns = {
+	.kref = {
+		.refcount	= ATOMIC_INIT(2),
+	},
         .wait = __WAIT_QUEUE_HEAD_INITIALIZER(init_kernel_syslog_ns.wait),
         .buf_len = __LOG_BUF_LEN,
         .buf = __log_buf
 };
+EXPORT_SYMBOL_GPL(init_kernel_syslog_ns);
 #endif
+/*
+ * Procedure to free all ressources tied to syslog

kernel/syslog.c +9 -0

+	return syslog;
+}
 
 /*
+ * Procedure to interface kref _put with syslog_free
+ *
+ */
+static void syslog_out(struct kref *kref)
+
+{
+	struct syslog_ns *sl;

kernel/syslog.c +2 -0

 {
 	struct syslog_ns *ns;
 

@@ -61,6 +92,8 @@ struct syslog_ns * syslog_malloc(unsigned container_buf_len)
 	if (!ns)
 		return ERR_PTR(-ENOMEM);
 
+	(void) kref_init(&(ns->kref));
+
 	ns->buf_len = container_buf_len;
 	ns->buf = kzalloc(container_buf_len, GFP_KERNEL);
 	if (!ns->buf) {

kernel/syslog.c +27 -9

 {
 	if ((syslog_ns == &init_kernel_syslog_ns ) && (container_buf_len > syslog_ns->buf_len)) {

@@ -102,7 +135,7 @@ struct syslog_ns * syslog_realloc(struct syslog_ns *syslog_ns, unsigned containe
 			(void) free_bootmem((unsigned long)old_buf, old_buf_len);
 		}
 	if (!syslog_ns)
-		return syslog_malloc(container_buf_len);
+		return malloc_syslog_ns(container_buf_len);
 	if (syslog_ns->buf_len > container_buf_len) {
 		(void) printk(KERN_WARNING "log_buf_len: Not allowed to decrease syslog buffer\n");
 		return ERR_PTR(-EINVAL);

@@ -126,32 +159,51 @@ struct syslog_ns * syslog_realloc(struct syslog_ns *syslog_ns, unsigned containe
 	(void) printk(KERN_NOTICE "log_buf_len: %u\n", syslog_ns->buf_len);
 	return syslog_ns;
 }
+
 /*
- * Procedure to free all ressources tied to syslog
+ * Procedure to use current syslog unless a CLONE_SYSLOG is set
+ * such a new syslog area is defined and used
  *
  */
-struct syslog_ns *syslog_free(struct syslog_ns *syslog)
+struct syslog_ns *copy_syslog_ns(unsigned long flags,struct syslog_ns *current_syslog_ns)
 
 {
-	if (syslog != (struct syslog_ns *)0) {
-		(void) kfree(syslog->buf);
-		(void) kfree(syslog);
-		syslog = (struct syslog_ns *)0;
-		}
-	return syslog;
+#define	CONTAINER_BUF_LEN	4096	/*should be enough for container syslog	*/
+
+	BUG_ON(!current_syslog_ns);
+	if ((flags & CLONE_SYSLOG) == 0) /*incrementing usage reference count	*/
+		(void) kref_get(&(current_syslog_ns->kref));
+	else
+		current_syslog_ns=malloc_syslog_ns(CONTAINER_BUF_LEN);
+	return current_syslog_ns;
+	
+}
+
+/*
+ * Procedure to decrement syslog usage count and free memory
+ * if syslog usage count reach zero.
+ *
+ */
+struct syslog_ns *release_syslog_ns(struct syslog_ns *current_syslog_ns)
+
+{
+	if (kref_put(&(current_syslog_ns->kref), syslog_out)==0)
+	   current_syslog_ns=(struct syslog_ns *)0;
+	return current_syslog_ns;
 }
 
 /*

I think this last point deserves significant elaboration. What
does it mean to "not leak or compromise hosts syslog data"? Is it possible to guarantee that we don't leak or is this just a best-effort sort of thing? My hunch is it's the latter. You really must respond to Eric's question of why this is useful. I know it might seem obvious to you but it's not actualy clear. I've found that carefuly explaining my reasoning when it seemed obvious often uncovers hidden assumptions I made and may even suggest problems and/or alternate approaches.

Kernel patch guidelines suggests you should split the patch up so it's one change at a time. One possible way to divide this into a series of patches is:

patch to add krefs
patch to move syslog_ns from user_namespace to nsproxy patch to rename syslog_current_ns() (or whatever) patch to rename X to Y

(There may be better ways to break up the patches -- these just came to mind)

Check with Suka on this one -- I seem to recall specific unused CLONE_ flags are, unfortunately, reserved. Otherwise yes, this is the right place and what Serge suggested.

You shouldn't be able to unshare syslog -- I think it's a security concern. I think just keeping CLONE_SYSLOG out of this set is sufficient for sys_unshare. There's a corresponding check in clone/clone2/eclone which would be similar to how CLONE_NEWPID is privileged.

kernel/nsproxy.c out: · line 165

Quoted 1 line

kernel/nsproxy.c +1 -0

+		ns->syslog_ns=release_syslog_ns(ns->syslog_ns);

style: space around the assignment operator.
See the kernel Documentation on contributing patches and the style of kernel code -- those are our standards too. scripts/checkpatch.pl will also find many of these things for you.

You shouldn't be able to unshare syslog. It should require using a privileged clone() call like CLONE_NEWPID. So CLONE_SYSLOG doesn't belong here I think.

In kernel code, especially namespaces, "get" implies taking an extra reference. So get_current_syslog_ns() needs a different name. I'd name it like the pid
namespace: anything without _ns on the end uses "current".

obj-$(CONFIG_PRINTK) += syslog.o

Then I think you can avoid this #ifdef entirely.

kernel/syslog.c line 37

Quoted 8 lines

/*
* Static memory definition, used to assign a syslog
* to the kernel itself
*
*/

kernel/syslog.c +0 -2

-
-#ifdef CONFIG_PRINTK

I suppose. Again, not a substantial change. To me the comment is unnecessary. This init_foo_ns pattern is quite recognizable to anyone familiar with how things are initialized in the kernel.

kernel/syslog.c line 84

Quoted 2 lines

kernel/syslog.c +1 -1

-struct syslog_ns * syslog_malloc(unsigned container_buf_len)
+static struct syslog_ns * malloc_syslog_ns(unsigned container_buf_len)

For namespaces we've been using "create" rather than foo_[m]alloc. create_pid_namespace, create_user_ns, create_uts_ns, etc.

Quoted 1 line

+struct syslog_ns * realloc_syslog_ns(struct syslog_ns *syslog_ns, unsigned container_buf_len)

Does this change the syslog_ns pointer, or just the buffer size? If the latter then resize might be a better word.