Re: [RFC][PATCH 2/4] memcg: fix error path ofmem_cgroup_move_parent

[Balbir Singh]

* Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx> [2009-01-08 19:14:45]:

> There is a bug in error path of mem_cgroup_move_parent.
> 
> Extra refcnt got from try_charge should be dropped, and usages incremented
> by try_charge should be decremented in both error paths:
> 
>     A: failure at get_page_unless_zero
>     B: failure at isolate_lru_page
> 
> This bug makes this parent directory unremovable.
> 
> In case of A, rmdir doesn't return, because res.usage doesn't go
> down to 0 at mem_cgroup_force_empty even after all the pc in
> lru are removed.
> In case of B, rmdir fails and returns -EBUSY, because it has
> extra ref counts even after res.usage goes down to 0.
> 
> 
> Signed-off-by: Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx>
> ---
>  mm/memcontrol.c |   23 +++++++++++++++--------
>  1 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 62e69d8..288e22c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -983,14 +983,15 @@ static int mem_cgroup_move_account(struct page_cgroup *pc,
>  	if (pc->mem_cgroup != from)
>  		goto out;
> 
> -	css_put(&from->css);
>  	res_counter_uncharge(&from->res, PAGE_SIZE);
>  	mem_cgroup_charge_statistics(from, pc, false);
>  	if (do_swap_account)
>  		res_counter_uncharge(&from->memsw, PAGE_SIZE);
> +	css_put(&from->css);
> +
> +	css_get(&to->css);
>  	pc->mem_cgroup = to;
>  	mem_cgroup_charge_statistics(to, pc, true);
> -	css_get(&to->css);
>  	ret = 0;
>  out:
>  	unlock_page_cgroup(pc);
> @@ -1023,8 +1024,10 @@ static int mem_cgroup_move_parent(struct page_cgroup *pc,
>  	if (ret || !parent)
>  		return ret;
> 
> -	if (!get_page_unless_zero(page))
> -		return -EBUSY;
> +	if (!get_page_unless_zero(page)) {
> +		ret = -EBUSY;
> +		goto uncharge;
> +	}
> 
>  	ret = isolate_lru_page(page);
> 
> @@ -1033,19 +1036,23 @@ static int mem_cgroup_move_parent(struct page_cgroup *pc,
> 
>  	ret = mem_cgroup_move_account(pc, child, parent);
> 
> -	/* drop extra refcnt by try_charge() (move_account increment one) */
> -	css_put(&parent->css);
>  	putback_lru_page(page);
>  	if (!ret) {
>  		put_page(page);
> +		/* drop extra refcnt by try_charge() */
> +		css_put(&parent->css);
>  		return 0;
>  	}
> -	/* uncharge if move fails */
> +
>  cancel:
> +	put_page(page);
> +uncharge:
> +	/* drop extra refcnt by try_charge() */
> +	css_put(&parent->css);
> +	/* uncharge if move fails */
>  	res_counter_uncharge(&parent->res, PAGE_SIZE);
>  	if (do_swap_account)
>  		res_counter_uncharge(&parent->memsw, PAGE_SIZE);
> -	put_page(page);
>  	return ret;
>  }
> 
>

Looks good to me, just out of curiousity how did you catch this error?
Through review or testing? 

-- 
	Balbir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/