netdev - Re: [PATCH v1 3/5] cgroup: Protect access to task_cls

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: <503220A7.2010807@parallels.com>
Date:	Mon, 20 Aug 2012 15:33:59 +0400
From:	Glauber Costa <glommer@...allels.com>
To:	Daniel Wagner <wagi@...om.org>
CC:	Neil Horman <nhorman@...driver.com>, <netdev@...r.kernel.org>,
	<cgroups@...r.kernel.org>,
	Daniel Wagner <daniel.wagner@...-carit.de>,
	"David S. Miller" <davem@...emloft.net>,
	Gao feng <gaofeng@...fujitsu.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	John Fastabend <john.r.fastabend@...el.com>,
	Li Zefan <lizefan@...wei.com>, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH v1 3/5] cgroup: Protect access to task_cls_classid() when
 built as module

On 08/20/2012 03:29 PM, Daniel Wagner wrote:
>> This is racy I think.  The read of the static key is atomic with other reads,
>> > but the entire conditional is not atomic.  If two cpus were creating cgroups in
>> > parallel, it would be possible for both to read the static key as being zero
>> > (the second cpu would read the key before the first cpu could increment it).
> D'oh, That is racy.
> 

Take a look at how we solve this particular problem in memcg. By using a
pair of bits meaning "ever active" and "currently active", we can avoid
problems with the static key increments.

Nothing bad can't happen by increasing the static key more than once;
the problem is that you need to somehow take note of that to make sure
you decrement the as many times you incremented when you release it.

Two other important things to keep in mind while dealing with static
branches:

* You can't increase/decrease while holding the cgroup_lock. lockdep may
trigger, because the cgroup_lock holds the cpu_hotplug lock, that the
static branches update path will also take. (due to cpusets). Doing a
logical hotplug will stress this path, and make the problem visible.

Take a look at disarm_sock_keys() (mm/memcontrol.c) to see how we solve
this particular problem.

* If you have code in more than one call site, the update among them is
not atomic. Not sure if this one applies to your case, but it can lead
you to some very unpleasant to debug problems. We use one of those two
bits I mentioned ("currently active") to make sure objects are not
marked before all sites are already patched.

Hope our previous experience with this can help you.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html