| Message ID | 1374015242-28112-1-git-send-email-kamal@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2013-07-16 at 16:06 -0700, Andrew Morton wrote: > On Tue, 16 Jul 2013 15:54:02 -0700 Kamal Mostafa <kamal@canonical.com> 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. Because it said "Cc: <stable@vger.kernel.org> [3.8]", making me think the author specifically wanted it to be applied to 3.8-stable. >[...] > Note to stable tree maintainers: I carefully evaluate every patch I > handle to decide whether or not it should be backported. Every single > one. Might you consider stripping off the "Cc: stable [3.8]" line when you add your SOB, in such a case? > Hence if you decide to backport a patch which I merged, you are > overriding an earlier decision of mine. I'm happy to defer to your judgment here. I've dropped it from the linux-3.8.y-queue. > 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. I'll try to watch out for them. Thanks Andrew, -Kamal
On Wed 17-07-13 14:25:26, Li Zefan wrote: > On 2013/7/17 8:45, Andrew Morton wrote: > > On Tue, 16 Jul 2013 16:40:09 -0700 Kamal Mostafa <kamal@canonical.com> wrote: > > > >> On Tue, 2013-07-16 at 16:06 -0700, Andrew Morton wrote: > >>> On Tue, 16 Jul 2013 15:54:02 -0700 Kamal Mostafa <kamal@canonical.com> 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. > >> > >> Because it said "Cc: <stable@vger.kernel.org> [3.8]", making me think > >> the author specifically wanted it to be applied to 3.8-stable. > > > > Damn, so it did, sorry. Your version removed that line. > > > > I don't know why I did that - afaict problems can only occur when > > kmalloc(GFP_KERNEL) fails, and that's exceedingly unlikely. > > > > I added the stable tag to this and the other patch, because the bugs were > regressions introduced in recent kernels, and Michal sugguest we may want > to backport them (but not in a strong feeling). Yes it was a clear regression so I considered it good enough for stable. > But surely the bug is extreamly unlikely, But now that I am thinking about it some more it doesn't sounds that unlikely. With kmem accounting the kmalloc allocation might fail much more easily. What if the task which creates the group reaches the kmem limit in its group? So I think adding this to stable makes some sense. I agree that the changelog could have been much more specific and will try to do better next time. The patch fixes reference counting imbalance and potential use-after-free. > and seems we currently want to be more strictly on what patches should > go into stable, I think it's fine to drop it from stable.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index fd7c0d3..618db74 100644 --- 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 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: http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.8.y-queue This patch is scheduled to be released in version 3.8.13.5. If you, or anyone else, feels it should not be added to this tree, please reply to this email. For more information about the 3.8.y.z tree, see https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable Thanks. -Kamal ------ From 030b24c55bfbae7ce607b134e669b2fb5b63de12 Mon Sep 17 00:00:00 2001 From: Michal Hocko <mhocko@suse.cz> 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. Signed-off-by: Michal Hocko <mhocko@suse.cz> Signed-off-by: Li Zefan <lizefan@huawei.com> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Cc: Hugh Dickins <hughd@google.com> Cc: Tejun Heo <tj@kernel.org> Cc: Glauber Costa <glommer@openvz.org> Cc: Johannes Weiner <hannes@cmpxchg.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> [ kamal: backport to 3.8 ] Signed-off-by: Kamal Mostafa <kamal@canonical.com> --- mm/memcontrol.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) -- 1.8.1.2