linux-kernel - Re: [RFC PATCH -tip] cpuacct: per-cgroup utime/stime statistics

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]

Date:	Mon, 16 Mar 2009 14:01:46 +0900
From:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To:	bharata@...ux.vnet.ibm.com
Cc:	linux-kernel@...r.kernel.org, Balaji Rao <balajirrao@...il.com>,
	Dhaval Giani <dhaval@...ux.vnet.ibm.com>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Li Zefan <lizf@...fujitsu.com>,
	Paul Menage <menage@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-arch@...r.kernel.org
Subject: Re: [RFC PATCH -tip] cpuacct: per-cgroup utime/stime statistics -
 v2

On Mon, 16 Mar 2009 10:07:54 +0530
Bharata B Rao <bharata@...ux.vnet.ibm.com> wrote:

> > > +static void cpuacct_update_stats(struct task_struct *tsk,
> > > +		enum cpuacct_stat_index idx, cputime_t val)
> > > +{
> > > +	struct cpuacct *ca;
> > > +
> > > +	if (unlikely(!cpuacct_subsys.active))
> > > +		return;
> > > +
> > > +	ca = task_ca(tsk);
> > > +
> > > +	do {
> > > +		percpu_counter_add(&ca->cpustat[idx], val);
> > > +		ca = ca->parent;
> > > +	} while (ca);
> > > +}
> > > +
> > 
> > IIUC, to make sure accessing "ca" to be safe, we need some condition.
> > (task_lock() or some other.....
> 
> task_lock() protects tsk->cgroups->subsys[]. So can we hold task_lock()
> to protect this walk ?
If necessary (see below)

> But we do this cpuacct hierarchy walk for the
> current task here. So can a current task's ca or ca's parents disappear
> from under us ?

Ok, followings are correct, I think.
 1. All updates are called under preempt-disabled.
 2. If "ca" is pointed by task, "disappear" will not happen.
 3. if one of children is alive, the parent is alive.
 4. "attach" can happen and tsk may be moved to other cgroup,
     because we don't take task_lock.
     So, ca = task_ca() may not be correct one.

What we should explain here is How we consider "4", here.

> > > +	ca = task_ca(tsk);
> > > + ----------------------------------(*1)
> > > +	do {
> > > +		percpu_counter_add(&ca->cpustat[idx], val);
> > > +		ca = ca->parent;
> > > +	} while (ca);
        ----------------------------------(*2)

About accounting, we don't have any problem even if (*2) is different from (*1).
But, considering "ca" can be empty cgroup after (*1), in _theory_, we cannot
say "ca" is valid pointer after (*1).

Hmm, if necessary, adding rcu_read_lock() before (*1) is reasonable ?
(I say "if necessary" because I'm not sure.)

Thanks,
-Kame 






--
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/