Re: [PATCH][rc1] cgroup: catch bad css refcnt at css_put

[Paul Menage <menage@google.com>]

On Mon, Sep 28, 2009 at 2:13 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> This is a patch for checking css->refcnt's sanity at css_put().
>
> BTW, I noticed that...css->refcnt may overflow if used with memcg...
> Now, refcnt is incremented per a page. Paul, do you have any idea ?
> (Ah, yes. "don't use css->refcnt per page" is maybe reasonable but
>  it will be big change..)
>
> ==
> __css_put() doesn't check a bug as refcnt goes to minus.
> I think it should be caught. This patch adds a check for it.
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Paul Menage <menage@google.com>

Looks reasonable, although there's no guarantee that it will warn on a
buggy release rather than a correct release that occur after a buggy
release.

> ---
>  kernel/cgroup.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.32-rc1/kernel/cgroup.c
> ===================================================================
> --- linux-2.6.32-rc1.orig/kernel/cgroup.c
> +++ linux-2.6.32-rc1/kernel/cgroup.c
> @@ -3708,8 +3708,10 @@ static void check_for_release(struct cgr
>  void __css_put(struct cgroup_subsys_state *css)
>  {
>        struct cgroup *cgrp = css->cgroup;
> +       int val;
>        rcu_read_lock();
> -       if (atomic_dec_return(&css->refcnt) == 1) {
> +       val = atomic_dec_return(&css->refcnt);
> +       if (val == 1) {
>                if (notify_on_release(cgrp)) {
>                        set_bit(CGRP_RELEASABLE, &cgrp->flags);
>                        check_for_release(cgrp);
> @@ -3717,6 +3719,7 @@ void __css_put(struct cgroup_subsys_stat
>                cgroup_wakeup_rmdir_waiter(cgrp);
>        }
>        rcu_read_unlock();
> +       WARN_ON(val < 1);
>  }
>
>  /*
>
>