Re: [PATCH] cgroups: fix probable race with put_css_set[_taskexit] and find_css_set

[Lai Jiangshan <laijs@cn.fujitsu.com>]

Paul Menage wrote:
> On Mon, Aug 18, 2008 at 11:29 PM, Lai Jiangshan <laijs@cn.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@kroah.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@cn.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