Re: Possible regression with cgroups in 3.11

[Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>]

On Thu 07-11-13 18:53:01, Johannes Weiner wrote:
[...]
> Unfortunately, memory cgroups don't hold the rcu_read_lock() over both
> the tryget and the res_counter charge that would make it visible to
> offline_css(), which means that there is a possible race condition
> between cgroup teardown and an ongoing charge:
> 
> #0: destroy                #1: charge
> 
>                            rcu_read_lock()
>                            css_tryget()
>                            rcu_read_unlock()
> disable tryget()
> call_rcu()
>   offline_css()
>     reparent_charges()
>                            res_counter_charge()
>                            css_put()
>                              css_free()
>                            pc->mem_cgroup = deadcg
>                            add page to lru

AFAICS this might only happen when a charge is done by a group nonmember
(the group has to be empty at the destruction path, right?) and this is
done only for the swap accounting. What we can do instead is that we
might recheck after the charge has been done and cancel + fallback to
current if we see that the group went away. Not nice either but it
shouldn't be intrusive and should work:

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d1fded477ef6..3d69a3fe4c55 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4006,6 +4006,24 @@ static int __mem_cgroup_try_charge_swapin(struct mm_struct *mm,
 		goto charge_cur_mm;
 	*memcgp = memcg;
 	ret = __mem_cgroup_try_charge(NULL, mask, 1, memcgp, true);
+
+	/*
+	 * try_get_mem_cgroup_from_page and the actual charge are not
+	 * done in the same RCU read section which means that the memcg
+	 * might get offlined before res counter is charged so we have
+	 * to recheck the memcg status again here and revert that charge
+	 * as we cannot be sure it was accounted properly.
+	 */
+	if (!ret) {
+		/* XXX: can we have something like css_online() check? */
+		if (!css_tryget(&memcg->css)) {
+			__mem_cgroup_cancel_charge(memcg, 1);
+			css_put(&memcg->css);
+			*memcgp = NULL;
+			goto charge_cur_mm;
+		}
+		css_put(&memcg->css);
+	}
 	css_put(&memcg->css);
 	if (ret == -EINTR)
 		ret = 0;

What do you think? I guess, in the long term we somehow have to mark
alien charges and delay css_offlining until they are done to prevent
hacks like above.
-- 
Michal Hocko
SUSE Labs