On Tue, 16 Jul 2013 15:54:02 -0700 Kamal Mostafa <kamal@xxxxxxxxxxxxx> wrote:
> This is a note to let you know that I have just added a patch titled
>
> memcg, kmem: fix reference count handling on the error path
>
> to the linux-3.8.y-queue branch of the 3.8.y.z extended stable tree
> which can be found at:
hm, why.
> From: Michal Hocko <mhocko@xxxxxxx>
> Date: Mon, 8 Jul 2013 16:00:29 -0700
> Subject: memcg, kmem: fix reference count handling on the error path
>
> commit f37a96914d1aea10fed8d9af10251f0b9caea31b upstream.
>
> mem_cgroup_css_online calls mem_cgroup_put if memcg_init_kmem fails.
> This is not correct because only memcg_propagate_kmem takes an
> additional reference while mem_cgroup_sockets_init is allowed to fail as
> well (although no current implementation fails) but it doesn't take any
> reference. This all suggests that it should be memcg_propagate_kmem
> that should clean up after itself so this patch moves mem_cgroup_put
> over there.
>
> Unfortunately this is not that easy (as pointed out by Li Zefan) because
> memcg_kmem_mark_dead marks the group dead (KMEM_ACCOUNTED_DEAD) if it is
> marked active (KMEM_ACCOUNTED_ACTIVE) which is the case even if
> memcg_propagate_kmem fails so the additional reference is dropped in
> that case in kmem_cgroup_destroy which means that the reference would be
> dropped two times.
>
> The easiest way then would be to simply remove mem_cgrroup_put from
> mem_cgroup_css_online and rely on kmem_cgroup_destroy doing the right
> thing.
We were bad. This changelog failed to describe the userspace-visible
effects of the bug (geeze, how often have I typed that?). Here we see
a consequence of that failure.
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6143,15 +6143,8 @@ mem_cgroup_css_alloc(struct cgroup *cont)
> spin_lock_init(&memcg->move_lock);
>
> error = memcg_init_kmem(memcg, &mem_cgroup_subsys);
> - if (error) {
> - /*
> - * We call put now because our (and parent's) refcnts
> - * are already in place. mem_cgroup_put() will internally
> - * call __mem_cgroup_free, so return directly
> - */
> - mem_cgroup_put(memcg);
> - return ERR_PTR(error);
> - }
> + if (error)
> + goto free_out;
> return &memcg->css;
> free_out:
> __mem_cgroup_free(memcg);
This fix only fixes things if memcg_init_kmem() fails. I expect it's
very unlikely that people will see memcg_init_kmem() failures in
practice.
Note to stable tree maintainers: I carefully evaluate every patch I
handle to decide whether or not it should be backported. Every single
one.
Hence if you decide to backport a patch which I merged, you are
overriding an earlier decision of mine.
Now, I will freely admit that I may have made a mistake. But please be
aware that you are taking a path which I have already considered and
rejected. So a little extra care is warranted for akpm patches, please.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html