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

[Vasily Averin]

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.