linux-kernel - Re: [PATCH] cgroups: fix probable race with put_css_set[_taskexit] and find_css

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:	Wed, 10 Sep 2008 10:18:36 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	Paul Menage <menage@...gle.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <greg@...ah.com>
Subject: Re: [PATCH] cgroups: fix probable race with put_css_set[_taskexit]
 and find_css_set

Paul Menage wrote:
> On Mon, Aug 18, 2008 at 11:29 PM, Lai Jiangshan <laijs@...fujitsu.com> wrote:
> 
> 2) Use atomic_inc_not_zero() in find_existing_css_set(), to ensure
> that we only return a referenced css, and remove the get_css_set()
> call from find_css_set(). (Possibly wrapping this in a new
> kref_get_not_zero() function)
> 

[CC: Greg Kroah-Hartman <greg@...ah.com>]

There are indeed several ways fix this race by Using the
atomic-functions directly. I prefer the second one, i makes all
code clearly. And put_css_set[_taskexit] do not need to be changed.

I don't think adding kref_get_not_zero() API is a good idea.
It will bring kref APIs to a little chaos, kref_get_not_zero() is
hard to be used, for this function needs a special lock held.

But I tried:

Signed-off-by: Lai Jiangshan <laijs@...fujitsu.com>
---
diff --git a/include/linux/kref.h b/include/linux/kref.h
index 0cef6ba..400ffab 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -25,6 +25,7 @@ struct kref {
 void kref_set(struct kref *kref, int num);
 void kref_init(struct kref *kref);
 void kref_get(struct kref *kref);
+int kref_get_not_zero(struct kref *kref);
 int kref_put(struct kref *kref, void (*release) (struct kref *kref));
 
 #endif /* _KREF_H_ */
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 13932ab..0bbb98d 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -347,6 +347,8 @@ static struct css_set *find_existing_css_set(
 	hlist_for_each_entry(cg, node, hhead, hlist) {
 		if (!memcmp(template, cg->subsys, sizeof(cg->subsys))) {
 			/* All subsystems matched */
+			if (!kref_get_not_zero(&cg->ref))
+				return NULL;
 			return cg;
 		}
 	}
@@ -410,8 +412,6 @@ static struct css_set *find_css_set(
 	 * the desired set */
 	read_lock(&css_set_lock);
 	res = find_existing_css_set(oldcg, cgrp, template);
-	if (res)
-		get_css_set(res);
 	read_unlock(&css_set_lock);
 
 	if (res)
diff --git a/lib/kref.c b/lib/kref.c
index 9ecd6e8..b8c1ce6 100644
--- a/lib/kref.c
+++ b/lib/kref.c
@@ -46,6 +46,25 @@ void kref_get(struct kref *kref)
 }
 
 /**
+ * kref_get_not_zero - increment refcount for object if current refcount
+ *                     is not zero.
+ * @kref: object.
+ *
+ * Beware, the object maybe be being released, so we need a special lock held
+ * to ensure the object's refcount is remaining access.
+ * 
+ * Return 0 if this refcount is 0, otherwise return 1.
+ */
+int kref_get_not_zero(struct kref *kref)
+{
+	if (atomic_inc_not_zero(&kref->refcount)) {
+		smp_mb__after_atomic_inc();
+		return 1;
+	}
+	return 0;
+}
+
+/**
  * kref_put - decrement refcount for object.
  * @kref: object.
  * @release: pointer to the function that will clean up the object when the



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