linux-kernel - Re: + cgroups-fix-ordering-of-calls-in-cgroup_attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110826151245.GA16243@redhat.com>
Date:	Fri, 26 Aug 2011 17:12:45 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	akpm@...ux-foundation.org
Cc:	linux-kernel@...r.kernel.org, bblum@...rew.cmu.edu,
	fweisbec@...il.com, lizf@...fujitsu.com, paul@...lmenage.org,
	tj@...nel.org
Subject: Re: + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch
	added to -mm tree

On 08/25, Andrew Morton wrote:
>
> From: Ben Blum <bblum@...rew.cmu.edu>
>
> @@ -2135,14 +2135,17 @@ int cgroup_attach_proc(struct cgroup *cg
>  		oldcgrp = task_cgroup_from_root(tsk, root);
>  		if (cgrp == oldcgrp)
>  			continue;
> -		/* attach each task to each subsystem */
> -		for_each_subsys(root, ss) {
> -			if (ss->attach_task)
> -				ss->attach_task(cgrp, tsk);
> -		}
>  		/* if the thread is PF_EXITING, it can just get skipped. */
>  		retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
> -		BUG_ON(retval != 0 && retval != -ESRCH);
> +		if (retval == 0) {
> +			/* attach each task to each subsystem */
> +			for_each_subsys(root, ss) {
> +				if (ss->attach_task)
> +					ss->attach_task(cgrp, tsk);
> +			}

Yes, I think this is what we need, the patch itself looks fine.

But this doesn't answer my another question. After that the code does

	 * step 4: do expensive, non-thread-specific subsystem callbacks.

		ss->attach(ss, cgrp, oldcgrp, leader);

OK, non-thread-specific is nice, but how can this "leader" represent
the process?

It can be zombie (but still group_leader) even without any races.
Say, cpuset_attach() and mem_cgroup_move_task() need get_task_mm(p).
How this can work if the leader is dead?

Also. Even if we add the locking around while_each_thread() (btw,
we need this in any case), we can race with exec which can change
the leader. In this case this task_struct has nothing to do with
the process we are going to attach, at all.

And, ss->can_attach(leader) has the same problems, it seems.

And. Say, devcgroup_can_attach() checks CAP_SYS_ADMIN. This is
security check. Why it is enough to check the leader only? We are
going to attach all threads. OK, this is probably fine, and I never
understood why capable/creds are not per-process, but this looks
so strange.

Oleg.

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