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

[Li Zefan <lizf@cn.fujitsu.com>]

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