Re: [ 3.8.y.z extended stable ] Patch "memcg, kmem: fix reference count handling on the error path" has been added to staging queue

Re: [ 3.8.y.z extended stable ] Patch "memcg, kmem: fix reference count handling on the error path" has been added to staging queue

[Andrew Morton]

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.

> 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.

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.

Re: [ 3.8.y.z extended stable ] Patch "memcg, kmem: fix reference count handling on the error path" has been added to staging queue

[Kamal Mostafa]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [ 3.8.y.z extended stable ] Patch "memcg, kmem: fix reference count handling on the error path" has been added to staging queue

[Andrew Morton]

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.

> > 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.

Re: [ 3.8.y.z extended stable ] Patch "memcg, kmem: fix reference count handling on the error path" has been added to staging queue

[Li Zefan]

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).

But surely the bug is extreamly unlikely, 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.

Re: [ 3.8.y.z extended stable ] Patch "memcg, kmem: fix reference count handling on the error path" has been added to staging queue

[Michal Hocko]

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.

-- 
Michal Hocko
SUSE Labs