linux-kernel - Re: [PATCH 7/8] cgroups: Add a task counter subsystem

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110809175708.GA27866@redhat.com>
Date:	Tue, 9 Aug 2011 19:57:08 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Paul Menage <menage@...gle.com>,
	Li Zefan <lizf@...fujitsu.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Aditya Kali <adityakali@...gle.com>
Subject: Re: [PATCH 7/8] cgroups: Add a task counter subsystem

On 08/09, Frederic Weisbecker wrote:
>
> On Tue, Aug 09, 2011 at 05:11:55PM +0200, Oleg Nesterov wrote:
> >
> > This doesn't look right or I missed something.
> >
> > What if tsk exits in between? Afaics this can happen with both
> > cgroup_attach_task() and cgroup_attach_proc().
> >
> > Let's look at cgroup_attach_task(). Suppose that
> > task_counter_can_attach_task() succeeds and charges the new cgrp,
> > Then cgroup_task_migrate() returns -ESRCH. Who will uncharge the
> > new cgrp?
> >
>
> I may totally be missing something but that looks safe to me.
> If the task has exited then cgroup_task_migrate() fails then we
> rollback with ->cancel_attach_task().

cgroup_attach_task() doesn't call ->cancel_attach_task() ;)

> > cgroup_attach_proc() is different, it calls cgroup_task_migrate()
> > after ->attach_task(). Cough.
>
> That's bad. I need to fix that.
>
> So if it returns -ESRCH, I shall not call attach_task() on it
> but cancel_attach_task().

Afaics this can't help, or I misunderstood. probably attach_task()
can check PF_EXITING...

> > ->attach_task() can be skipped if cgrp == oldcgrp... Probably this
> > is fine, in this case can_attach_task() shouldn't actually charge.
>
> In fact in this case it simply doesn't charge. res_counter_common_ancestor()
> returns the res_counter for cgrp as a limit and thus charging stops as soon
> as it starts.

Yes, this is what I meant. Just it wasn't immediately obvious for me,
initially I thought this is buggy.

> @@ -1295,6 +1295,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
> > >  	p->group_leader = p;
> > >  	INIT_LIST_HEAD(&p->thread_group);
> > >
> > > +	retval = cgroup_task_counter_fork(p);
> > > +	if (retval)
> > > +		goto bad_fork_free_pid;
> > > +
> >
> > Well, imho this is not good. You are adding yet another cgroup hook.
> > Why you can not reuse cgroup_fork_callbacks() ?
> >
> > Yes, it returns void. Can't we chane ->fork() to return the error and
> > make it boolean?
>
> That was my first proposition (minus the rollback with exit() that I forgot)
> but Paul Menage said that added unnecessary complexity in the fork callbacks.

Hmm. Yes, this adds some complexity in the fork callbacks.

But yet another cgroup_task_counter_fork() hook complicates the core kernel
code. And since I personally do not care about cgroups at all, I think this
is much worse ;)

> I would personally prefer that.

I strongly agree.

OK. Lets do it this way. Perhaps we can convince Paul later and cleanup.

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/