[RFC PATCH] rlimit: Account nproc per-usernamespace/per-user

Nikolay Borisov kernel at kyup.com
Thu Oct 27 07:01:03 UTC 2016



On 10/26/2016 08:25 PM, Serge E. Hallyn wrote:
> On Wed, Oct 26, 2016 at 03:40:27PM +0300, Nikolay Borisov wrote:
>> There are container setups which map the same kuids to
>> different containers. In such situation what will happen is
>> that same uid's in different containers will map to the same
>> underlying user on the matchine (e.g. same struct user). One
>> implication of this is that the number of processes for that
>> particular user are going to be shared among all the same uids
>> in the container. This is problematic, as it means a user in
>> containerA can potentially exhaust the process limit such that
>> a user in containerB cannot spawn any processes.
> 
> Hi - thanks for the description.  Based on that, though, I worry
> that it is a feature we do not want.  Nothing explicitly prohibits
> sharing kuids in different containers, but it is is sharing.  If
> you want greater isolation between two containers, you must not share
> any kuids.
> 
> I'm not saying nack, but i am saying it seems a misguided feature
> which could lead people to think sharing uids is safer than it is.

I agree that in order for this to be considered "secure" it relies on
the assumption that there is no leakage between containers. However,
there are currently setups which rely on this behavior for whatever
(mis)guided reasons. Furthermore the current design of namespaces
doesn't do anything to prevent such uses. Given this I don't think it be
fair to completely disregard them, hence the patch.

> 
>> Fix this by utilising the newly introduced UCOUNT infrastructure,
>> so that process counts are now being accounted per-userns/per-user.
>> In case when a machine is running in non-container setup then
>> the accounting semantics is virtually the same, given that there is
>> going to be one ucount-per-user struct.
>>
>> Signed-off-by: Nikolay Borisov <kernel at kyup.com>
>> ---
>> Hello Eric, 
>>
>> Now that ucount has been merged I took the liberty of 
>> experimenting what the nproc-per-userns approach would 
>> look like and here is the result. In my previous [0] you 
>> expressed concerns regarding not having hierarchical limits, 
>> essentially allowing users to circumvent their limits. So
>> this version doesn't do anything to rectify this issue. 
>> However, I do think the idea of the rlimit is not to 
>> forbid the user of spawning more processes, since currently
>> the limit is being set on a per-process basis, so a malicious 
>> process can in fact increase that limits and spawn a large number 
>> of processes. Given this I believe the current patch doesn't make
>> the situation any worse in that regard. 
>>
>> [0] https://lists.linux-foundation.org/pipermail/containers/2015-September/036229.html
>>
>>  fs/exec.c                      |  2 +-
>>  include/linux/cred.h           |  1 +
>>  include/linux/sched.h          |  1 -
>>  include/linux/user_namespace.h |  8 ++++++++
>>  kernel/cred.c                  | 18 +++++++++++-------
>>  kernel/exit.c                  |  2 +-
>>  kernel/fork.c                  |  7 +++++--
>>  kernel/sys.c                   |  2 +-
>>  kernel/ucount.c                | 16 ++++++++++++++++
>>  kernel/user.c                  |  1 -
>>  10 files changed, 44 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 6fcfb3f7b137..8126e00a8d3e 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1651,7 +1651,7 @@ static int do_execveat_common(int fd, struct filename *filename,
>>  	 * whether NPROC limit is still exceeded.
>>  	 */
>>  	if ((current->flags & PF_NPROC_EXCEEDED) &&
>> -	    atomic_read(&current_user()->processes) > rlimit(RLIMIT_NPROC)) {
>> +	    atomic_read(&current_cred()->ucounts->ulimit[ULIMIT_NPROC]) > rlimit(RLIMIT_NPROC)) {
>>  		retval = -EAGAIN;
>>  		goto out_ret;
>>  	}
>> diff --git a/include/linux/cred.h b/include/linux/cred.h
>> index f0e70a1bb3ac..c3bb4d20ff16 100644
>> --- a/include/linux/cred.h
>> +++ b/include/linux/cred.h
>> @@ -141,6 +141,7 @@ struct cred {
>>  	void		*security;	/* subjective LSM security */
>>  #endif
>>  	struct user_struct *user;	/* real user ID subscription */
>> +	struct ucounts *ucounts;		/* container for per-userns/per-counts */
>>  	struct user_namespace *user_ns; /* user_ns the caps and keyrings are relative to. */
>>  	struct group_info *group_info;	/* supplementary groups for euid/fsgid */
>>  	struct rcu_head	rcu;		/* RCU deletion hook */
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 348f51b0ec92..3709d70fe2de 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -842,7 +842,6 @@ static inline int signal_group_exit(const struct signal_struct *sig)
>>   */
>>  struct user_struct {
>>  	atomic_t __count;	/* reference count */
>> -	atomic_t processes;	/* How many processes does this user have? */
>>  	atomic_t sigpending;	/* How many pending signals does this user have? */
>>  #ifdef CONFIG_INOTIFY_USER
>>  	atomic_t inotify_watches; /* How many inotify watches does this user have? */
>> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
>> index eb209d4523f5..d4fc31ae9d85 100644
>> --- a/include/linux/user_namespace.h
>> +++ b/include/linux/user_namespace.h
>> @@ -35,6 +35,11 @@ enum ucount_type {
>>  	UCOUNT_COUNTS,
>>  };
>>  
>> +enum ulimit_type {
>> +	ULIMIT_NPROC,
>> +	ULIMIT_COUNTS,
>> +};
>> +
>>  struct user_namespace {
>>  	struct uid_gid_map	uid_map;
>>  	struct uid_gid_map	gid_map;
>> @@ -67,6 +72,7 @@ struct ucounts {
>>  	kuid_t uid;
>>  	atomic_t count;
>>  	atomic_t ucount[UCOUNT_COUNTS];
>> +	atomic_t ulimit[ULIMIT_COUNTS];
>>  };
>>  
>>  extern struct user_namespace init_user_ns;
>> @@ -75,6 +81,8 @@ bool setup_userns_sysctls(struct user_namespace *ns);
>>  void retire_userns_sysctls(struct user_namespace *ns);
>>  struct ucounts *inc_ucount(struct user_namespace *ns, kuid_t uid, enum ucount_type type);
>>  void dec_ucount(struct ucounts *ucounts, enum ucount_type type);
>> +struct ucounts *inc_ulimit(struct user_namespace *ns, kuid_t uid, enum ulimit_type type);
>> +void dec_ulimit(struct ucounts *ucounts, enum ulimit_type type);
>>  
>>  #ifdef CONFIG_USER_NS
>>  
>> diff --git a/kernel/cred.c b/kernel/cred.c
>> index 5f264fb5737d..dd9ffc0396bb 100644
>> --- a/kernel/cred.c
>> +++ b/kernel/cred.c
>> @@ -330,13 +330,16 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
>>  #endif
>>  		clone_flags & CLONE_THREAD
>>  	    ) {
>> -		p->real_cred = get_cred(p->cred);
>> +		struct cred *c;
>> +		struct ucounts *uc = inc_ulimit(p->cred->user_ns, p->cred->uid,
>> +						ULIMIT_NPROC);
>> +		p->real_cred = c = get_cred(p->cred);
>>  		get_cred(p->cred);
>>  		alter_cred_subscribers(p->cred, 2);
>>  		kdebug("share_creds(%p{%d,%d})",
>>  		       p->cred, atomic_read(&p->cred->usage),
>>  		       read_cred_subscribers(p->cred));
>> -		atomic_inc(&p->cred->user->processes);
>> +		c->ucounts = uc;
>>  		return 0;
>>  	}
>>  
>> @@ -369,7 +372,8 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
>>  	}
>>  #endif
>>  
>> -	atomic_inc(&new->user->processes);
>> +	new->ucounts = inc_ulimit(new->user_ns, new->uid,
>> +				      ULIMIT_NPROC);
>>  	p->cred = p->real_cred = get_cred(new);
>>  	alter_cred_subscribers(new, 2);
>>  	validate_creds(new);
>> @@ -461,12 +465,12 @@ int commit_creds(struct cred *new)
>>  	 * in set_user().
>>  	 */
>>  	alter_cred_subscribers(new, 2);
>> -	if (new->user != old->user)
>> -		atomic_inc(&new->user->processes);
>> +	if (new->user != old->user || old->user_ns != new->user_ns)
>> +		new->ucounts = inc_ulimit(new->user_ns, new->uid, ULIMIT_NPROC);
>>  	rcu_assign_pointer(task->real_cred, new);
>>  	rcu_assign_pointer(task->cred, new);
>> -	if (new->user != old->user)
>> -		atomic_dec(&old->user->processes);
>> +	if (new->user != old->user || old->user_ns != new->user_ns)
>> +		dec_ulimit(old->ucounts, ULIMIT_NPROC);
>>  	alter_cred_subscribers(old, -2);
>>  
>>  	/* send notifications */
>> diff --git a/kernel/exit.c b/kernel/exit.c
>> index 9d68c45ebbe3..7ac8954230bd 100644
>> --- a/kernel/exit.c
>> +++ b/kernel/exit.c
>> @@ -173,7 +173,7 @@ void release_task(struct task_struct *p)
>>  	/* 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);
>> +	dec_ulimit(__task_cred(p)->ucounts, ULIMIT_NPROC);
>>  	rcu_read_unlock();
>>  
>>  	proc_flush_task(p);
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 623259fc794d..0b16c9d920b1 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -425,6 +425,7 @@ int arch_task_struct_size __read_mostly;
>>  void __init fork_init(void)
>>  {
>>  	int i;
>> +	struct cred *c = init_task.cred;
>>  #ifndef CONFIG_ARCH_TASK_STRUCT_ALLOCATOR
>>  #ifndef ARCH_MIN_TASKALIGN
>>  #define ARCH_MIN_TASKALIGN	L1_CACHE_BYTES
>> @@ -448,6 +449,8 @@ void __init fork_init(void)
>>  	for (i = 0; i < UCOUNT_COUNTS; i++) {
>>  		init_user_ns.ucount_max[i] = max_threads/2;
>>  	}
>> +
>> +	c->ucounts = inc_ulimit(&init_user_ns, make_kuid(&init_user_ns, 0), ULIMIT_NPROC);
>>  }
>>  
>>  int __weak arch_dup_task_struct(struct task_struct *dst,
>> @@ -1515,7 +1518,7 @@ static __latent_entropy struct task_struct *copy_process(
>>  	DEBUG_LOCKS_WARN_ON(!p->softirqs_enabled);
>>  #endif
>>  	retval = -EAGAIN;
>> -	if (atomic_read(&p->real_cred->user->processes) >=
>> +	if (atomic_read(&p->real_cred->ucounts->ulimit[ULIMIT_NPROC]) >=
>>  			task_rlimit(p, RLIMIT_NPROC)) {
>>  		if (p->real_cred->user != INIT_USER &&
>>  		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
>> @@ -1859,7 +1862,7 @@ static __latent_entropy struct task_struct *copy_process(
>>  #endif
>>  	delayacct_tsk_free(p);
>>  bad_fork_cleanup_count:
>> -	atomic_dec(&p->cred->user->processes);
>> +	dec_ulimit(p->cred->ucounts, ULIMIT_NPROC);
>>  	exit_creds(p);
>>  bad_fork_free:
>>  	put_task_stack(p);
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index 89d5be418157..143d04ed9da5 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -433,7 +433,7 @@ static int set_user(struct cred *new)
>>  	 * for programs doing set*uid()+execve() by harmlessly deferring the
>>  	 * failure to the execve() stage.
>>  	 */
>> -	if (atomic_read(&new_user->processes) >= rlimit(RLIMIT_NPROC) &&
>> +	if (atomic_read(&new->ucounts->ulimit[ULIMIT_NPROC]) >= rlimit(RLIMIT_NPROC) &&
>>  			new_user != INIT_USER)
>>  		current->flags |= PF_NPROC_EXCEEDED;
>>  	else
>> diff --git a/kernel/ucount.c b/kernel/ucount.c
>> index 9d20d5dd298a..d8f5362f6e17 100644
>> --- a/kernel/ucount.c
>> +++ b/kernel/ucount.c
>> @@ -181,6 +181,22 @@ static inline bool atomic_inc_below(atomic_t *v, int u)
>>  	}
>>  }
>>  
>> +struct ucounts *inc_ulimit(struct user_namespace *ns, kuid_t uid,
>> +			   enum ulimit_type type)
>> +{
>> +	struct ucounts *ucounts = get_ucounts(ns, uid);
>> +	atomic_inc(&ucounts->ulimit[type]);
>> +
>> +	return ucounts;
>> +}
>> +
>> +void dec_ulimit(struct ucounts *ucounts, enum ulimit_type type)
>> +{
>> +	atomic_dec(&ucounts->ulimit[type]);
>> +	WARN_ON(atomic_read(&ucounts->ulimit[type]) < 0);
>> +	put_ucounts(ucounts);
>> +}
>> +
>>  struct ucounts *inc_ucount(struct user_namespace *ns, kuid_t uid,
>>  			   enum ucount_type type)
>>  {
>> diff --git a/kernel/user.c b/kernel/user.c
>> index b069ccbfb0b0..ce1ba1fb96c0 100644
>> --- a/kernel/user.c
>> +++ b/kernel/user.c
>> @@ -90,7 +90,6 @@ static DEFINE_SPINLOCK(uidhash_lock);
>>  /* root_user.__count is 1, for init task cred */
>>  struct user_struct root_user = {
>>  	.__count	= ATOMIC_INIT(1),
>> -	.processes	= ATOMIC_INIT(1),
>>  	.sigpending	= ATOMIC_INIT(0),
>>  	.locked_shm     = 0,
>>  	.uid		= GLOBAL_ROOT_UID,
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> Containers mailing list
>> Containers at lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/containers


More information about the Containers mailing list