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

PATCH In review Lai Jiangshan 4 participants 13 messages Aug 19, 2008 – Sep 12, 2008 Library code

lore

Share & tools

lore

Lai Jiangshan

author

2008-08-19 06:31 UTC #0 lore raw

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

put_css_set_taskexit may be called when find_css_set is called on other cpu. And the race will occur:

put_css_set_taskexit side find_css_set side

                                        |
atomic_dec_and_test(&kref->refcount)    |
    /* kref->refcount = 0 */            |
....................................................................
                                        |  read_lock(&css_set_lock)
                                        |  find_existing_css_set
                                        |  get_css_set
                                        |  read_unlock(&css_set_lock);

....................................................................
__release_css_set |
....................................................................
| /* use a released css_set */ |

[put_css_set is the same. But in the current code, all put_css_set are put into cgroup mutex critical region as the same as find_css_set.]

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/cgroup.c +20 -3

index 13932ab..003912e 100644
@@ -241,7 +241,6 @@ static void unlink_css_set(struct css_set *cg)
 	struct cg_cgroup_link *link;
 	struct cg_cgroup_link *saved_link;
 
-	write_lock(&css_set_lock);
 	hlist_del(&cg->hlist);
 	css_set_count--;

@@ -251,8 +250,6 @@ static void unlink_css_set(struct css_set *cg)
 		list_del(&link->cgrp_link_list);
 		kfree(link);
 	}
-
-	write_unlock(&css_set_lock);
 }
 
 static void __release_css_set(struct kref *k, int taskexit)

@@ -260,7 +257,13 @@ static void __release_css_set(struct kref *k, int taskexit)
 	int i;
 	struct css_set *cg = container_of(k, struct css_set, ref);
 
+	write_lock(&css_set_lock);
+	if (atomic_read(&k->refcount) > 0) {
+		write_unlock(&css_set_lock);
+		return;
+	}
 	unlink_css_set(cg);
+	write_unlock(&css_set_lock);
 
 	rcu_read_lock();
 	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {

@@ -410,6 +413,20 @@ static struct css_set *find_css_set(
 	 * the desired set */
 	read_lock(&css_set_lock);
 	res = find_existing_css_set(oldcg, cgrp, template);
+	/*
+	 * put_css_set[_taskexit]() may race with find_css_set(), in that
+	 * find_css_set() got the css_set after put_css_set() had released it.
+	 *
+	 * We should put the whole put_css_set[_taskexit]() into css_set_lock's
+	 * write_lock critical setion to avoid this race. But it will increase
+	 * overhead for do_exit().
+	 *
+	 * So we do not avoid this race but put it under control:
+	 * __release_css_set() will re-check the refcount
+	 * with css_set_lock held.
+	 *
+	 * This race may trigger the warnning in kref_get().
+	 */
 	if (res)
 		get_css_set(res);
 	read_unlock(&css_set_lock);

Paul Menage

2008-09-10 00:28 UTC #1 lore raw

Quoted 3 lines · Lai Jiangshan

put_css_set_taskexit may be called when find_css_set is called on other cpu. And the race will occur:

Sorry I didn't respond to this when it originally came out - I was on vacation.

I agree that it's a race that needs to be fixed, but I'm not sure that I like the fix that can generate kref warnings.

I can see two possible fixes:

1) avoid the race entirely by introducing some new primitives:

atomic_dec_and_write_lock() (like atomic_dec_and_lock(), but for an rwlock)

and kref_put_and_write_lock() which would be something like:

int kref_put_and_write_lock(struct kref *kref, void (*release)(struct kref *kref), rwlock*lock)
{

  if(atomic_dec_and_write_lock(&kref->refcount, lock)) {
    release(kref);
    return 1;

}
return 0;
}

We'd then use kref_put_and_write_lock(), and enter __release_css_set() with the lock already held

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)

Paul

Lai Jiangshan

author

2008-09-10 02:21 UTC #2 lore raw

Quoted 7 lines · Paul Menage

On Mon, Aug 18, 2008 at 11:29 PM, Lai Jiangshan <[email protected]> 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 <[email protected]>]

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 <[email protected]>

---

include/linux/kref.h +1 -0

index 0cef6ba..400ffab 100644
@@ -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_ */

kernel/cgroup.c +2 -2

index 13932ab..0bbb98d 100644
@@ -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)

lib/kref.c +19 -0

index 9ecd6e8..b8c1ce6 100644
@@ -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

Li Zefan

2008-09-10 02:41 UTC #3 lore raw

Quoted 40 lines · Lai Jiangshan

Quoted 7 lines · Paul Menage
On Mon, Aug 18, 2008 at 11:29 PM, Lai Jiangshan <[email protected]> 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 <[email protected]>]
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 <[email protected]>
---
include/linux/kref.h +1 -0
index 0cef6ba..400ffab 100644
@@ -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_ */
kernel/cgroup.c +0 -0
index 13932ab..0bbb98d 100644

kernel/cgroup.c static struct css_set *find_existing_css_set( · line 351

Quoted 5 lines

kernel/cgroup.c +2 -0

@@ -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;

Better add comments to explain why we might get 0 refcount here.

Quoted 17 lines

kernel/cgroup.c +0 -2

 			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)

lib/kref.c +0 -0

index 9ecd6e8..b8c1ce6 100644

lib/kref.c void kref_get(struct kref *kref) · line 53

Quoted 8 lines

lib/kref.c +5 -0

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

s/maybe be being/may be being/

lib/kref.c void kref_get(struct kref *kref) · line 55

Quoted 2 lines

lib/kref.c +2 -0

+ * to ensure the object's refcount is remaining access.
+ *

need better wording... what do you mean by "remaining access"? to ensure the object is remaining valid?

Quoted 17 lines

lib/kref.c +12 -0

+ * 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

Paul Menage

2008-09-10 03:12 UTC #4 lore raw

In reply to Lai Jiangshan

kernel/cgroup.c static struct css_set *find_existing_css_set( · line 351

Quoted 5 lines · Lai Jiangshan

       hlist_for_each_entry(cg, node, hhead, hlist) {
               if (!memcmp(template, cg->subsys, sizeof(cg->subsys))) {
                       /* All subsystems matched */

kernel/cgroup.c +2 -0

+                       if (!kref_get_not_zero(&cg->ref))
+                               return NULL;

Maybe it would be better to do:

if (!memcmp(...) && kref_get_not_zero(&cg->ref))
return cg;

so that in the event of a race, we carry on looking in the array.

Quoted 3 lines

+ * Return 0 if this refcount is 0, otherwise return 1.
+ */
+int kref_get_not_zero(struct kref *kref)

Probably clearer to have a bool return type.

Paul

Greg KH

2008-09-10 05:03 UTC #5 lore raw

In reply to Lai Jiangshan

Quoted 15 lines · Lai Jiangshan

Quoted 7 lines · Paul Menage
On Mon, Aug 18, 2008 at 11:29 PM, Lai Jiangshan <[email protected]> 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 <[email protected]>]
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.

Quoted 5 lines

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:

What are you trying to solve here with this change? I agree, it does seem a bit "chaotic" :)

I thought we used to have something like this for kref in the past, but I must be mistaken as it's no longer there...

thanks,

greg k-h

Paul Menage

2008-09-10 05:31 UTC #6 lore raw

Quoted 3 lines · Greg KH

What are you trying to solve here with this change? I agree, it does seem a bit "chaotic" :)

There's a place in cgroups that uses kref_put() to release an object; the release function *then* takes a write-lock and removes the object from a lookup table; it could race with another thread that searches the lookup table (while holding a read-lock) and does kref_get() on the same object.

The current fix is for the release function to recheck inside the lock that the object's refcount is still zero, and only actually unlink/free it if so. And actually I've just realised that this isn't actually even safe, since the thread that just acquired the object could kref_put() it almost immediately, which would leave two threads both trying to unlink/free the object.

The two solutions being considered are:

- add a kref_put_and_write_lock(), similar to atomic_dec_and_lock(), which would ensure that the final refcount on the object was only released inside the lock

- add a kref_get_if_not_zero(), which would prevent a lookup from succeeding if another thread had just dropped the last reference on the object.

Paul

Greg KH

2008-09-10 06:18 UTC #7 lore raw

Quoted 10 lines · Paul Menage

Quoted 3 lines · Greg KH
What are you trying to solve here with this change? I agree, it does seem a bit "chaotic" :)
There's a place in cgroups that uses kref_put() to release an object; the release function *then* takes a write-lock and removes the object from a lookup table; it could race with another thread that searches the lookup table (while holding a read-lock) and does kref_get() on the same object.

Ick, yeah that's not good.

What about the way everyone else solves this, grab the lock before you call kref_put()?

Quoted 6 lines

The current fix is for the release function to recheck inside the lock that the object's refcount is still zero, and only actually unlink/free it if so. And actually I've just realised that this isn't actually even safe, since the thread that just acquired the object could kref_put() it almost immediately, which would leave two threads both trying to unlink/free the object.

Yeah, don't do that :)

thanks,

greg k-h

Li Zefan

2008-09-10 06:27 UTC #8 lore raw

Quoted 10 lines · Greg KH

Quoted 8 lines · Paul Menage
Quoted 2 lines · Greg KH
What are you trying to solve here with this change? I agree, it does seem a bit "chaotic" :)
There's a place in cgroups that uses kref_put() to release an object; the release function *then* takes a write-lock and removes the object from a lookup table; it could race with another thread that searches the lookup table (while holding a read-lock) and does kref_get() on the same object.

Quoted 5 lines

Ick, yeah that's not good.
What about the way everyone else solves this, grab the lock before you call kref_put()?

do_exit()
cgroup_exit()

    put_css_set_taskexit()
      kref_put()

If we grab the lock before kref_put(), we add overhead to do_exit(), which is what we are trying to avoid here.

Quoted 9 lines

Quoted 6 lines
The current fix is for the release function to recheck inside the lock that the object's refcount is still zero, and only actually unlink/free it if so. And actually I've just realised that this isn't actually even safe, since the thread that just acquired the object could kref_put() it almost immediately, which would leave two threads both trying to unlink/free the object.
Yeah, don't do that :)

Greg KH

2008-09-10 06:30 UTC #9 lore raw

Quoted 17 lines · Li Zefan

Quoted 10 lines · Greg KH
Quoted 8 lines · Paul Menage
Quoted 2 lines · Greg KH
What are you trying to solve here with this change? I agree, it does seem a bit "chaotic" :)
There's a place in cgroups that uses kref_put() to release an object; the release function *then* takes a write-lock and removes the object from a lookup table; it could race with another thread that searches the lookup table (while holding a read-lock) and does kref_get() on the same object.
Quoted 5 lines
Ick, yeah that's not good.
What about the way everyone else solves this, grab the lock before you call kref_put()?

Quoted 7 lines

do_exit()
cgroup_exit()
    put_css_set_taskexit()
      kref_put()
If we grab the lock before kref_put(), we add overhead to do_exit(), which is what we are trying to avoid here.

But you can't put such logic in the release() function as you are finding out, that's not going to work either.

Maybe you need to just "open-code" an atomic counter here and not use kref as it sounds like you are needing to do something very "special" here.

thanks,

greg k-h

Paul Menage

2008-09-10 15:03 UTC #10 lore raw

Quoted 3 lines · Greg KH

But you can't put such logic in the release() function as you are finding out, that's not going to work either.

Right - that's why a kref_put_and_write_lock() function would be handy
- it would take the lock only when necessary. It would delegate most of its work to the (as-yet nonexistant) atomic_dec_and_write_lock() function, which would be an rwlock equivalent of atomic_dec_and_lock()

Quoted 5 lines

Maybe you need to just "open-code" an atomic counter here and not use kref as it sounds like you are needing to do something very "special" here.

It's basically the same situation as the dentry cache - ref-counted objects that are also referenced from a hash table protected by a global lock.

If you're opposed to the addition of kref_put_and_write_lock() then yes, I'll replace kref with a custom refcount.

Paul

Greg KH

2008-09-12 19:09 UTC #11 lore raw

Quoted 16 lines · Paul Menage

Quoted 3 lines · Greg KH
But you can't put such logic in the release() function as you are finding out, that's not going to work either.
Right - that's why a kref_put_and_write_lock() function would be handy
- it would take the lock only when necessary. It would delegate most of its work to the (as-yet nonexistant) atomic_dec_and_write_lock() function, which would be an rwlock equivalent of atomic_dec_and_lock()
Quoted 5 lines
Maybe you need to just "open-code" an atomic counter here and not use kref as it sounds like you are needing to do something very "special" here.

Quoted 6 lines

It's basically the same situation as the dentry cache - ref-counted objects that are also referenced from a hash table protected by a global lock.
If you're opposed to the addition of kref_put_and_write_lock() then yes, I'll replace kref with a custom refcount.

It just seems messy, but if you want to try it, I'll be glad to look at the code. Oh wait, was that the patch that you sent out last time?

thanks,

greg k-h

Paul Menage

2008-09-12 19:33 UTC #12 lore raw

Quoted 5 lines · Greg KH

Quoted 2 lines
If you're opposed to the addition of kref_put_and_write_lock() then yes, I'll replace kref with a custom refcount.
It just seems messy, but if you want to try it, I'll be glad to look at the code. Oh wait, was that the patch that you sent out last time?

Yes - that patch replaced kref with a plain refcount. We weren't really using kref as anything other than an atomic_dec_and_test() wrapper anyway.

Paul