[Devel] [PATCH RH7] memcg leak due to missing css_put on last kmem uncharge

[Vasily Averin]

On 2/24/21 5:38 PM, Vasily Averin wrote:
> On 2/24/21 11:47 AM, Kirill Tkhai wrote:
>> On 22.02.2021 21:40, Vasily Averin wrote:
>>> When mem_cgroup is removes mem_cgroup_css_offline() calls
>>> memcg_deactivate_kmem() which disables kmem accounting.
>>> If memcg still have some charged kmem final css_put is not called,
>>> and delayed till last kmem will be uncharged.
>>>
>>> Usually kmem is uncharged by using memcg_uncharge_kmem() which have
>>> according checks and if required calls final css_put().
>>>
>>> Though patch added to vz7.162.14 kernel
>>> "mm/memcg: Use per-cpu stock charges for ->kmem and ->cache counters"
>>> enabled kmem charge/uncharge in refill_stock()/drain_stock()
>>> without using of memcg_uncharge_kmem(), as result nobody called
>>> final css_put() after last kmem uncharge.
>>>
>>> This patch adds css_get/put for safe access to memcg in drain_stock(),
>>> and calls an additional css_put() after last kmem uncharge.
>>>
>>> Fixes: "mm/memcg: Use per-cpu stock charges for ->kmem and ->cache counters"
>>> https://bugs.openvz.org/browse/OVZ-7250
>>> Signed-off-by: Vasily Averin <vvs at virtuozzo.com>
>>> ---
>>>  mm/memcontrol.c | 20 ++++++++++++++++----
>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index e0a4309..24e3bd7 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -439,6 +439,8 @@ enum {
>>>  	KMEM_ACCOUNTED_DEAD, /* dead memcg with pending kmem charges */
>>>  };
>>>  
>>> +static void memcg_kmem_release_css(struct mem_cgroup *memcg);
>>> +
>>>  static struct mem_cgroup_per_node *
>>>  mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid)
>>>  {
>>> @@ -2928,11 +2930,15 @@ static void drain_stock(struct memcg_stock_pcp *stock)
>>>  {
>>>  	struct mem_cgroup *old = stock->cached;
>>>  	unsigned long nr_pages = stock->nr_pages + stock->cache_nr_pages + stock->kmem_nr_pages;
>>> +	u64 kmem = 1;
>>> +
>>> +	if (!old)
>>> +		return;
>>>  
>>>  	if (stock->cache_nr_pages)
>>>  		page_counter_uncharge(&old->cache, stock->cache_nr_pages);
>>>  	if (stock->kmem_nr_pages)
>>> -		page_counter_uncharge(&old->kmem, stock->kmem_nr_pages);
>>> +		kmem = page_counter_uncharge(&old->kmem, stock->kmem_nr_pages);
>>>  
>>>  	if (nr_pages) {
>>>  		page_counter_uncharge(&old->memory, nr_pages);
>>> @@ -2942,6 +2948,9 @@ static void drain_stock(struct memcg_stock_pcp *stock)
>>>  		stock->kmem_nr_pages = 0;
>>>  		stock->cache_nr_pages = 0;
>>>  	}
>>> +	css_put(&old->css);
>>
>> css_put()/css_get() pair looks excess. What is your arguments, why do we need them?
>> We have a problem with kmem only, while the rest of accounting looks safe:
>> they are safely uncharged from css_offline->mem_cgroup_reparent_charges(), and they
>> never own css counter.
> 
> We know that kmem can be uncharged after mem_cgroup_css_offline() -> memcg_deactivate_kmem()
> because kmem was not 0 inside  memcg_deactivate_kmem and according css_put() was not called.
> 
> I did not investigated how is called drain_stock uncharged last kmem,
> and I do not see who keeps memcg refcount here.
> Please explain this if you know. 
> 
> If drain_stock() can be called after end of mem_cgroup_css_offline it is a problem.
> css_get/put present in refill_stock()/drain_stock() long time ago, 
> though I did not investigated when and why exactly it was added.
> 
> Anyway, I'm not sure that it is really required in vz7 kernel,
> we did not saw any related troubles.

I've found finally
mem_cgroup_css_offline
  mem_cgroup_reparent_charges
    drain_all_stock_sync()
       drain_stock()

So now I'm agree: additional css_get/put in refill_stock/drain_stock() are not required.