netdev - Re: [BUGFIX][PATCH 2/3] memcg/tcp: remove static_branch_slow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F744038.1000900@parallels.com>
Date:	Thu, 29 Mar 2012 12:58:00 +0200
From:	Glauber Costa <glommer@...allels.com>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
CC:	<netdev@...r.kernel.org>, David Miller <davem@...emloft.net>,
	"Andrew Morton" <akpm@...ux-foundation.org>
Subject: Re: [BUGFIX][PATCH 2/3] memcg/tcp: remove static_branch_slow_dec()
 at changing limit

On 03/29/2012 09:07 AM, KAMEZAWA Hiroyuki wrote:
> tcp memcontrol uses static_branch to optimize limit=RESOURCE_MAX case.
> If all cgroup's limit=RESOUCE_MAX, resource usage is not accounted.
> But it's buggy now.
> 
> For example, do following
>   # while sleep 1;do
>     echo 9223372036854775807>  /cgroup/memory/A/memory.kmem.tcp.limit_in_bytes;
>     echo 300M>  /cgroup/memory/A/memory.kmem.tcp.limit_in_bytes;
>     done
> 
> and run network application under A. tcp's usage is sometimes accounted
> and sometimes not accounted because of frequent changes of static_branch.
> Then, finally, you can see broken tcp.usage_in_bytes.
> WARN_ON() is printed because res_counter->usage goes below 0.
> ==
> kernel: ------------[ cut here ]----------
> kernel: WARNING: at kernel/res_counter.c:96 res_counter_uncharge_locked+0x37/0x40()
>   <snip>
> kernel: Pid: 17753, comm: bash Tainted: G  W    3.3.0+ #99
> kernel: Call Trace:
> kernel:<IRQ>   [<ffffffff8104cc9f>] warn_slowpath_common+0x7f/0xc0
> kernel: [<ffffffff810d7e88>] ? rb_reserve__next_event+0x68/0x470
> kernel: [<ffffffff8104ccfa>] warn_slowpath_null+0x1a/0x20
> kernel: [<ffffffff810b4e37>] res_counter_uncharge_locked+0x37/0x40
> ...
> ==
> 
> This patch removes static_branch_slow_dec() at changing res_counter's
> limit to RESOUCE_MAX. By this, once accounting started, the accountting
> will continue until the tcp cgroup is destroyed.
> 
> I think this will not be problem in real use.
> 

So...

Are the warnings still there if you have your other patch in this series?
Maybe what we should do is, flush the resource counters so they go back
to 0 besides decrementing the static branch. This way we get a more
consistent behavior.

Another thing to keep in mind, is that the static branch will only be
inactive if we turn off *all* controllers. You see this happening
because you are only testing with one.
So even if we go to the route you're proposing, we could probably try
doing something on the
global level, instead of a per-memcg boolean flat.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html