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

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

[Lai Jiangshan]

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 <laijs@cn.fujitsu.com>
---
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 13932ab..003912e 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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);

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

[Paul Menage]

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

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

[Lai Jiangshan]

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

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

[Li Zefan]

Lai Jiangshan wrote:
> 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;

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

>  			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

s/maybe be being/may be being/

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

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

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

[Paul Menage]

On Tue, Sep 9, 2008 at 7:18 PM, Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
>        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;

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

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

[Greg KH]

On Wed, Sep 10, 2008 at 10:18:36AM +0800, Lai Jiangshan wrote:
> 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:

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

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

[Paul Menage]

On Tue, Sep 9, 2008 at 10:01 PM, Greg KH <greg@kroah.com> wrote:
>
> 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

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

[Greg KH]

On Tue, Sep 09, 2008 at 10:31:24PM -0700, Paul Menage wrote:
> On Tue, Sep 9, 2008 at 10:01 PM, Greg KH <greg@kroah.com> wrote:
> >
> > 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()?

> 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

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

[Li Zefan]

Greg KH wrote:
> On Tue, Sep 09, 2008 at 10:31:24PM -0700, Paul Menage wrote:
>> On Tue, Sep 9, 2008 at 10:01 PM, Greg KH <greg@kroah.com> wrote:
>>> 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()?
> 

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.

>> 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 :)
> 

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

[Greg KH]

On Wed, Sep 10, 2008 at 02:25:57PM +0800, Li Zefan wrote:
> Greg KH wrote:
> > On Tue, Sep 09, 2008 at 10:31:24PM -0700, Paul Menage wrote:
> >> On Tue, Sep 9, 2008 at 10:01 PM, Greg KH <greg@kroah.com> wrote:
> >>> 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()?
> > 
> 
> 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

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

[Paul Menage]

On Tue, Sep 9, 2008 at 11:29 PM, Greg KH <greg@kroah.com> wrote:
>
> 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()

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

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

[Greg KH]

On Wed, Sep 10, 2008 at 08:03:18AM -0700, Paul Menage wrote:
> On Tue, Sep 9, 2008 at 11:29 PM, Greg KH <greg@kroah.com> wrote:
> >
> > 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()
> 
> >
> > 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.

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

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

[Paul Menage]

On Fri, Sep 12, 2008 at 8:58 AM, Greg KH <greg@kroah.com> wrote:
>> 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