[RFC PATCH 0/2] Containerise nproc count

[RFC PATCH 0/2] Containerise nproc count

[Nikolay Borisov]

From: Nikolay Borisov <n.borisov@siteground.com>

Hello, 

This is an initial try to have nproc count apply per-userns, 
rather than per the global user struct. The implementation is 
really simple - a hashtable holding uid->nproc mapping for each
id inside the respective namespace. In its current form I have also
left the debugging code so that people who want to have a play with 
it can easily see what's happening. 

Now, this is only an RFC and I'd like to gather your thoughts about
the semantics. Currently as it stands I have tested the patchset by 
invoking multiple LXC containers, with identical uid mappings and 
users with the same uid inside the containers and it was working
correctly. 

There is an issue however, when using the unshare syscall and then doing
the mappings e.g. using "unshare -r" util from util-linux the initial process 
(the one which have done the unsharing) is accounted to the overflowuid but 
then again when exiting from the resulting shell the UID for user 0 is being 
freed which causes the BUG_ON in nsuser_nproc_dec to trigger. My initial idea 
for fixing this was to add code which upon writing to /proc/[pid]/uid_map  
would map all current processes from overflowuid to the 'ns->uid_map.extent[0].first'. 
This was working correctly but it was breaking the use case of lxc, since lxc is 
changing the uids after creating the uid_mapping (maybe this is a deficiency in the 
unshare util implementation?)

Another thing that needs improving is the locking occuring on the nsuser_nproc_hash, 
since in its current coarse-grained form it is serialisign process/thread creation on 
a per-usernamespace basis. 

I'm happy to discuss any concerns and improvements that people might have 
regarding this patchset. 


Nikolay Borisov (2):
  userns: Implement per-userns nproc infrastructure
  userns/nproc: Add hooks for userns nproc management

 include/linux/user_namespace.h |  15 +++++-
 kernel/cred.c                  |  36 +++++++++++++-
 kernel/exit.c                  |   9 ++++
 kernel/fork.c                  |  33 ++++++++++---
 kernel/user.c                  |   3 ++
 kernel/user_namespace.c        | 105 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 192 insertions(+), 9 deletions(-)

-- 
2.5.0

[RFC PATCH 1/2] userns: Implement per-userns nproc infrastructure

[Nikolay Borisov]

From: Nikolay Borisov <n.borisov@siteground.com>

This patch add a simple hashtable to the user_namespace structure and
the necessary functions to work with it. The idea is to keep a
uid->nproc counts per-namespace.

Signed-off-by: Nikolay Borisov <kernel@kyup.com>
---
 include/linux/user_namespace.h |  15 +++++-
 kernel/user.c                  |   3 ++
 kernel/user_namespace.c        | 105 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+), 1 deletion(-)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 8297e5b..6eb9414 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -5,6 +5,9 @@
 #include <linux/nsproxy.h>
 #include <linux/ns_common.h>
 #include <linux/sched.h>
+#include <linux/hashtable.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
 #include <linux/err.h>
 
 #define UID_GID_MAP_MAX_EXTENTS 5
@@ -21,7 +24,7 @@ struct uid_gid_map {	/* 64 bytes -- 1 cache line */
 #define USERNS_SETGROUPS_ALLOWED 1UL
 
 #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED
-
+#define NPROC_HASH_ORDER 7
 struct user_namespace {
 	struct uid_gid_map	uid_map;
 	struct uid_gid_map	gid_map;
@@ -33,6 +36,8 @@ struct user_namespace {
 	kgid_t			group;
 	struct ns_common	ns;
 	unsigned long		flags;
+	struct spinlock		nproc_hash_lock;
+	DECLARE_HASHTABLE(nproc_hash, NPROC_HASH_ORDER);
 
 	/* Register of per-UID persistent keyrings for this namespace */
 #ifdef CONFIG_PERSISTENT_KEYRINGS
@@ -72,6 +77,9 @@ extern ssize_t proc_projid_map_write(struct file *, const char __user *, size_t,
 extern ssize_t proc_setgroups_write(struct file *, const char __user *, size_t, loff_t *);
 extern int proc_setgroups_show(struct seq_file *m, void *v);
 extern bool userns_may_setgroups(const struct user_namespace *ns);
+extern void userns_nproc_inc(struct user_namespace *ns, uid_t uid);
+extern void userns_nproc_dec(struct user_namespace *ns, uid_t uid);
+extern uint32_t get_userns_nproc(struct user_namespace *ns, uid_t uid);
 #else
 
 static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
@@ -100,6 +108,11 @@ static inline bool userns_may_setgroups(const struct user_namespace *ns)
 {
 	return true;
 }
+
+
+static void userns_nproc_inc(ustruct user_namespace *ns, id_t uid) {}
+static void userns_nproc_dec(ustruct user_namespace *ns, id_t uid) {}
+static uint32_t get_userns_nproc(ustruct user_namespace *ns, id_t uid) { return 0; }
 #endif
 
 #endif /* _LINUX_USER_H */
diff --git a/kernel/user.c b/kernel/user.c
index b2a552f..3a0e36e 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -54,6 +54,9 @@ struct user_namespace init_user_ns = {
 #ifdef CONFIG_USER_NS
 	.ns.ops = &userns_operations,
 #endif
+	//Intentially leave the nproc_hash_lock uninitialised
+	//as it shouldn't be used in the init namespace
+
 	.flags = USERNS_INIT_FLAGS,
 #ifdef CONFIG_PERSISTENT_KEYRINGS
 	.persistent_keyring_register_sem =
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 2f919cc..654ece7 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -9,6 +9,7 @@
 #include <linux/nsproxy.h>
 #include <linux/slab.h>
 #include <linux/user_namespace.h>
+#include <linux/hashtable.h>
 #include <linux/proc_ns.h>
 #include <linux/highuid.h>
 #include <linux/cred.h>
@@ -26,6 +27,13 @@
 static struct kmem_cache *user_ns_cachep __read_mostly;
 static DEFINE_MUTEX(userns_state_mutex);
 
+struct userns_nproc_count {
+	uint32_t key;
+	atomic_t processes;
+	struct hlist_node node;
+	struct rcu_head rcu_head;
+};
+
 static bool new_idmap_permitted(const struct file *file,
 				struct user_namespace *ns, int cap_setid,
 				struct uid_gid_map *map);
@@ -129,6 +137,10 @@ int create_user_ns(struct cred *new)
 	ns->flags = parent_ns->flags;
 	mutex_unlock(&userns_state_mutex);
 
+	spin_lock_init(&ns->nproc_hash_lock);
+	hash_init(ns->nproc_hash);
+
+	pr_info("new user_ns created: %p\n", ns);
 	set_cred_user_ns(new, ns);
 
 #ifdef CONFIG_PERSISTENT_KEYRINGS
@@ -168,6 +180,9 @@ void free_user_ns(struct user_namespace *ns)
 		key_put(ns->persistent_keyring_register);
 #endif
 		ns_free_inum(&ns->ns);
+
+		BUG_ON(!hash_empty(ns->nproc_hash));
+
 		kmem_cache_free(user_ns_cachep, ns);
 		if (user)
 			atomic_dec(&user->user_namespaces);
@@ -248,6 +263,96 @@ static u32 map_id_up(struct uid_gid_map *map, u32 id)
 	return id;
 }
 
+static struct userns_nproc_count *find_nproc_count(struct user_namespace *ns,
+						   uid_t uid)
+{
+	struct userns_nproc_count *found = NULL;
+
+	hash_for_each_possible(ns->nproc_hash, found, node, uid)
+		if (found->key == uid)
+			return found;
+
+	return NULL;
+}
+
+void userns_nproc_inc(struct user_namespace *ns, uid_t uid)
+{
+	struct userns_nproc_count *nproc_count;
+
+	BUG_ON(!ns);
+
+	spin_lock(&ns->nproc_hash_lock);
+	nproc_count = find_nproc_count(ns, uid);
+
+	if (nproc_count) {
+		atomic_inc(&nproc_count->processes);
+		spin_unlock(&ns->nproc_hash_lock);
+	} else {
+		spin_unlock(&ns->nproc_hash_lock);
+
+		nproc_count = kzalloc(sizeof(struct userns_nproc_count), GFP_KERNEL);
+		if (!nproc_count)
+			return; //silent failure
+
+		atomic_set(&nproc_count->processes, 1);
+		nproc_count->key = uid;
+
+		spin_lock(&ns->nproc_hash_lock);
+		hash_add(ns->nproc_hash, &nproc_count->node, uid);
+		spin_unlock(&ns->nproc_hash_lock);
+	}
+
+	pr_info("\t%s:incrementing count in user_ns: %p for uid: %u old:%d new:%d\n", __func__,
+		ns, uid, atomic_read(&nproc_count->processes)-1, atomic_read(&nproc_count->processes));
+}
+
+void userns_nproc_dec(struct user_namespace *ns, uid_t uid)
+{
+	struct userns_nproc_count *nproc_count;
+	char comm[TASK_COMM_LEN];
+
+	BUG_ON(!ns);
+
+	spin_lock(&ns->nproc_hash_lock);
+	nproc_count = find_nproc_count(ns, uid);
+
+	if (!nproc_count || atomic_read(&nproc_count->processes) <= 0)
+		pr_info("Will bug on %s/%d for uid: %d\n", get_task_comm(comm, current), task_pid_nr(current), uid);
+
+	BUG_ON(!nproc_count);
+
+	pr_info("\t%s:decrementing count in user_ns: %p for uid: %u old:%d new:%d\n", __func__,
+		ns, uid, atomic_read(&nproc_count->processes)+1, atomic_read(&nproc_count->processes));
+
+	if (atomic_dec_and_test(&nproc_count->processes)) {
+		pr_info("\t%s:Freeing nproc_count node for uid %s/%d\n", __func__, get_task_comm(comm, current), task_pid_nr(current));
+
+		hash_del(&nproc_count->node);
+		kfree(nproc_count);
+	}
+
+	spin_unlock(&ns->nproc_hash_lock);
+}
+
+uint32_t get_userns_nproc(struct user_namespace *ns, uid_t uid)
+{
+	struct userns_nproc_count *nproc_count;
+	uint32_t ret;
+
+	BUG_ON(!ns);
+
+	spin_lock(&ns->nproc_hash_lock);
+	nproc_count = find_nproc_count(ns, uid);
+
+	BUG_ON(!nproc_count);
+
+	ret = atomic_read(&nproc_count->processes);
+	spin_unlock(&ns->nproc_hash_lock);
+
+	return ret;
+}
+
+
 /**
  *	make_kuid - Map a user-namespace uid pair into a kuid.
  *	@ns:  User namespace that the uid is in
-- 
2.5.0

[RFC PATCH 2/2] userns/nproc: Add hooks for userns nproc management

[Nikolay Borisov]

From: Nikolay Borisov <n.borisov@siteground.com>

This patch introduce the usage of the userns_nproc_* functions
where necessary to have correct accounting of the processes.

Signed-off-by: Nikolay Borisov <kernel@kyup.com>
---
 kernel/cred.c | 36 ++++++++++++++++++++++++++++++++++--
 kernel/exit.c |  9 +++++++++
 kernel/fork.c | 33 +++++++++++++++++++++++++++------
 3 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/kernel/cred.c b/kernel/cred.c
index b7581dc..79565b8 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -320,6 +320,7 @@ struct cred *prepare_exec_creds(void)
 int copy_creds(struct task_struct *p, unsigned long clone_flags)
 {
 	struct cred *new;
+	struct user_namespace *ns;
 	int ret;
 
 	if (
@@ -331,10 +332,15 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
 		p->real_cred = get_cred(p->cred);
 		get_cred(p->cred);
 		alter_cred_subscribers(p->cred, 2);
+		ns = p->real_cred->user_ns;
 		kdebug("share_creds(%p{%d,%d})",
 		       p->cred, atomic_read(&p->cred->usage),
 		       read_cred_subscribers(p->cred));
 		atomic_inc(&p->cred->user->processes);
+		if (ns != &init_user_ns) {
+			pr_info ("%s: incrementing nproc from due copy_process (CLONE_THREAD)\n", __func__);
+			userns_nproc_inc(ns, from_kuid_munged(ns, p->real_cred->uid));
+		}
 		return 0;
 	}
 
@@ -343,6 +349,7 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
 		return -ENOMEM;
 
 	if (clone_flags & CLONE_NEWUSER) {
+		pr_debug("%s: Creating new usernamespace\n", __func__);
 		ret = create_user_ns(new);
 		if (ret < 0)
 			goto error_put;
@@ -369,6 +376,11 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
 
 	atomic_inc(&new->user->processes);
 	p->cred = p->real_cred = get_cred(new);
+	ns = p->real_cred->user_ns;
+	if (ns != &init_user_ns) {
+		pr_info("%s: Incrementing due to not-being a thread\n",	__func__);
+		userns_nproc_inc(ns, from_kuid_munged(ns, p->real_cred->uid));
+	}
 	alter_cred_subscribers(new, 2);
 	validate_creds(new);
 	return 0;
@@ -454,17 +466,37 @@ int commit_creds(struct cred *new)
 	if (!gid_eq(new->fsgid, old->fsgid))
 		key_fsgid_changed(task);
 
+	/* Handle cases when a process is moving from one userns to another */
+	if (old->user_ns != new->user_ns) {
+		if (new->user_ns != &init_user_ns) {
+			pr_info ("\t%s: incrementing user count in %p\n", __func__, new->user_ns);
+			userns_nproc_inc(new->user_ns, from_kuid_munged(new->user_ns, new->uid));
+		}
+		if (old->user_ns != &init_user_ns) {
+			pr_info ("\t%s: decrementing user_count in %p\n", __func__, old->user_ns);
+			userns_nproc_dec(old->user_ns, from_kuid_munged(old->user_ns, old->uid));
+		}
+	}
+
 	/* do it
 	 * RLIMIT_NPROC limits on user->processes have already been checked
 	 * in set_user().
 	 */
 	alter_cred_subscribers(new, 2);
-	if (new->user != old->user)
+	if (new->user != old->user) {
 		atomic_inc(&new->user->processes);
+		if (new->user_ns != &init_user_ns)
+			userns_nproc_inc(new->user_ns,
+				 from_kuid_munged(new->user_ns, new->uid));
+	}
 	rcu_assign_pointer(task->real_cred, new);
 	rcu_assign_pointer(task->cred, new);
-	if (new->user != old->user)
+	if (new->user != old->user) {
 		atomic_dec(&old->user->processes);
+		if (old->user_ns != &init_user_ns)
+			userns_nproc_dec(old->user_ns,
+				 from_kuid_munged(old->user_ns, old->uid));
+	}
 	alter_cred_subscribers(old, -2);
 
 	/* send notifications */
diff --git a/kernel/exit.c b/kernel/exit.c
index 22fcc05..dde172b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -170,13 +170,22 @@ void release_task(struct task_struct *p)
 {
 	struct task_struct *leader;
 	int zap_leader;
+	struct user_namespace *ns;
+	kuid_t uid;
 repeat:
 	/* don't need to get the RCU readlock here - the process is dead and
 	 * can't be modifying its own credentials. But shut RCU-lockdep up */
 	rcu_read_lock();
 	atomic_dec(&__task_cred(p)->user->processes);
+	ns = get_user_ns(__task_cred(p)->user_ns);
+	uid = __task_cred(p)->uid;
 	rcu_read_unlock();
 
+	if (ns != &init_user_ns)
+		userns_nproc_dec(ns, from_kuid_munged(ns, uid));
+
+	put_user_ns(ns);
+
 	proc_flush_task(p);
 
 	write_lock_irq(&tasklist_lock);
diff --git a/kernel/fork.c b/kernel/fork.c
index f9826a3..c537b6a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1308,18 +1308,34 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	DEBUG_LOCKS_WARN_ON(!p->softirqs_enabled);
 #endif
 	retval = -EAGAIN;
-	if (atomic_read(&p->real_cred->user->processes) >=
-			task_rlimit(p, RLIMIT_NPROC)) {
-		if (p->real_cred->user != INIT_USER &&
-		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
-			goto bad_fork_free;
-	}
+	//If we are in the root namespace use this check
+	if (p->real_cred->user_ns == &init_user_ns) {
+		if (atomic_read(&p->real_cred->user->processes) >=
+				task_rlimit(p, RLIMIT_NPROC)) {
+			if (p->real_cred->user != INIT_USER &&
+			    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
+				goto bad_fork_free;
+		}
 	current->flags &= ~PF_NPROC_EXCEEDED;
+	}
 
 	retval = copy_creds(p, clone_flags);
 	if (retval < 0)
 		goto bad_fork_free;
 
+	//Otherwise perform the non-root userns check here
+	//since we want the stuff in copy_cred to have already happened
+	if (p->real_cred->user_ns != &init_user_ns) {
+		struct user_namespace *ns = p->real_cred->user_ns;
+		int32_t processes = get_userns_nproc(ns, from_kuid_munged(ns, p->real_cred->uid));
+		retval = -EAGAIN;
+
+		if (processes >= task_rlimit(p, RLIMIT_NPROC))
+			goto bad_fork_cleanup_userns_count;
+		else
+			current->flags &= ~PF_NPROC_EXCEEDED;
+	}
+
 	/*
 	 * If multiple threads are within copy_process(), then this check
 	 * triggers too late. This doesn't hurt, the check is only there
@@ -1652,6 +1668,9 @@ bad_fork_cleanup_threadgroup_lock:
 	delayacct_tsk_free(p);
 bad_fork_cleanup_count:
 	atomic_dec(&p->cred->user->processes);
+bad_fork_cleanup_userns_count:
+	if (p->cred->user_ns != &init_user_ns)
+		userns_nproc_dec(p->cred->user_ns, from_kuid_munged(p->cred->user_ns, p->cred->uid));
 	exit_creds(p);
 bad_fork_free:
 	free_task(p);
@@ -1936,6 +1955,7 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
 	int do_sysvsem = 0;
 	int err;
 
+	pr_info("%s begin\n", __func__);
 	/*
 	 * If unsharing a user namespace must also unshare the thread.
 	 */
@@ -2037,6 +2057,7 @@ bad_unshare_cleanup_fs:
 		free_fs_struct(new_fs);
 
 bad_unshare_out:
+	pr_info("%s end\n", __func__);
 	return err;
 }
 
-- 
2.5.0

Re: [RFC PATCH 0/2] Containerise nproc count

[Eric W. Biederman]

Nikolay Borisov <kernel@kyup.com> writes:

> From: Nikolay Borisov <n.borisov@siteground.com>
>
> Hello, 
>
> This is an initial try to have nproc count apply per-userns, 
> rather than per the global user struct. The implementation is 
> really simple - a hashtable holding uid->nproc mapping for each
> id inside the respective namespace. In its current form I have also
> left the debugging code so that people who want to have a play with 
> it can easily see what's happening. 
>
> Now, this is only an RFC and I'd like to gather your thoughts about
> the semantics. Currently as it stands I have tested the patchset by 
> invoking multiple LXC containers, with identical uid mappings and 
> users with the same uid inside the containers and it was working
> correctly. 
>
> There is an issue however, when using the unshare syscall and then doing
> the mappings e.g. using "unshare -r" util from util-linux the initial process 
> (the one which have done the unsharing) is accounted to the overflowuid but 
> then again when exiting from the resulting shell the UID for user 0 is being 
> freed which causes the BUG_ON in nsuser_nproc_dec to trigger. My initial idea 
> for fixing this was to add code which upon writing to /proc/[pid]/uid_map  
> would map all current processes from overflowuid to the 'ns->uid_map.extent[0].first'. 
> This was working correctly but it was breaking the use case of lxc, since lxc is 
> changing the uids after creating the uid_mapping (maybe this is a deficiency in the 
> unshare util implementation?)
>
> Another thing that needs improving is the locking occuring on the nsuser_nproc_hash, 
> since in its current coarse-grained form it is serialisign process/thread creation on 
> a per-usernamespace basis. 
>
> I'm happy to discuss any concerns and improvements that people might have 
> regarding this patchset. 

So. no.

Changing rlimit nproc this way breaks per user process accounting.
Effectively this allows any user to escape their NPROC limit by creating
a new user namespace.  Which means that to even consider anything like
this we need hierarchical limits.

I am not particularly convinced that reusing uids between containers is
all that smart.  Certainly it is bad to assume that there is no leakage
and containers can never interact.  So something like this also needs a
description of why this new set of semantics is a good direction to go
in.

Eric