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

[Kirill Tkhai]

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.

> +	if (kmem == 0)
> +		memcg_kmem_release_css(old);
>  	stock->cached = NULL;
>  }
>  
> @@ -2978,6 +2987,7 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages,
>  
>  	if (stock->cached != memcg) { /* reset if necessary */
>  		drain_stock(stock);
> +		css_get(&memcg->css);
>  		stock->cached = memcg;
>  	}
>  
> @@ -3608,10 +3618,12 @@ void memcg_uncharge_kmem(struct mem_cgroup *memcg,
>  	if (do_swap_account)
>  		page_counter_uncharge(&memcg->memsw, nr_pages);
>  
> -	/* Not down to 0 */
> -	if (kmem)
> -		return;
> +	if (kmem == 0)
> +		memcg_kmem_release_css(memcg);
> +}
>  
> +static void memcg_kmem_release_css(struct mem_cgroup *memcg)
> +{

We should not mix refactoring and functional changes. This should go in a separate patch.

>  	/*
>  	 * Releases a reference taken in memcg_deactivate_kmem in case
>  	 * this last uncharge is racing with the offlining code or it is
>