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]

Message-ID: <48C733A2.2050709@cn.fujitsu.com>
Date:	Wed, 10 Sep 2008 10:40:34 +0800
From:	Li Zefan <lizf@...fujitsu.com>
To:	Lai Jiangshan <laijs@...fujitsu.com>
CC:	Paul Menage <menage@...gle.com>,
	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

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

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

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