[PATCH] cgroup: make sure that decisions in __css_put are atomic

[PATCH] cgroup: make sure that decisions in __css_put are atomic

[Salman Qazi]

__css_put is using atomic_dec on the ref count, and then
looking at the ref count to make decisions.  This is prone
to races, as someone else may decrement ref count between
our decrement and our decision.  Instead, we should base our
decisions on the value that we decremented the ref count to.

(This results in an actual race on Google's kernel which I
haven't been able to reproduce on the upstream kernel.  Having
said that, it's still incorrect by inspection).

Signed-off-by: Salman Qazi <sqazi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 0f3527d..18dc8aa 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4973,8 +4973,7 @@ void __css_put(struct cgroup_subsys_state *css)
 	struct cgroup *cgrp = css->cgroup;
 
 	rcu_read_lock();
-	atomic_dec(&css->refcnt);
-	switch (css_refcnt(css)) {
+	switch (atomic_dec_return(&css->refcnt)) {
 	case 1:
 		if (notify_on_release(cgrp)) {
 			set_bit(CGRP_RELEASABLE, &cgrp->flags);

Re: [PATCH] cgroup: make sure that decisions in __css_put are atomic

[Li Zefan]

Salman Qazi wrote:

> __css_put is using atomic_dec on the ref count, and then
> looking at the ref count to make decisions.  This is prone
> to races, as someone else may decrement ref count between
> our decrement and our decision.  Instead, we should base our
> decisions on the value that we decremented the ref count to.
> 
> (This results in an actual race on Google's kernel which I
> haven't been able to reproduce on the upstream kernel.  Having
> said that, it's still incorrect by inspection).
> 
> Signed-off-by: Salman Qazi <sqazi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>


Acked-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Good catch! This patch should be backported for 3.4.

> ---
>  kernel/cgroup.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 0f3527d..18dc8aa 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -4973,8 +4973,7 @@ void __css_put(struct cgroup_subsys_state *css)
>  	struct cgroup *cgrp = css->cgroup;
>  
>  	rcu_read_lock();
> -	atomic_dec(&css->refcnt);
> -	switch (css_refcnt(css)) {
> +	switch (atomic_dec_return(&css->refcnt)) {
>  	case 1:
>  		if (notify_on_release(cgrp)) {
>  			set_bit(CGRP_RELEASABLE, &cgrp->flags);
> 

Re: [PATCH] cgroup: make sure that decisions in __css_put are atomic

[Tejun Heo]

On Wed, Jun 06, 2012 at 03:31:19PM +0800, Li Zefan wrote:
> Salman Qazi wrote:
> 
> > __css_put is using atomic_dec on the ref count, and then
> > looking at the ref count to make decisions.  This is prone
> > to races, as someone else may decrement ref count between
> > our decrement and our decision.  Instead, we should base our
> > decisions on the value that we decremented the ref count to.
> > 
> > (This results in an actual race on Google's kernel which I
> > haven't been able to reproduce on the upstream kernel.  Having
> > said that, it's still incorrect by inspection).
> > 
> > Signed-off-by: Salman Qazi <sqazi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> 
> 
> Acked-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> Good catch! This patch should be backported for 3.4.

Applied to for-3.5-fixes w/ stable@ added.

Thanks.

-- 
tejun