Re: [PATCH v2] hugetlb_cgroup: fix imbalanced css_get and css_put pair for shared mappings

From: Mike Kravetz <mike.kravetz@oracle.com>
To: Miaohe Lin <linmiaohe@huawei.com>, akpm@linux-foundation.org
Cc: almasrymina@google.com, rientjes@google.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] hugetlb_cgroup: fix imbalanced css_get and css_put pair for shared mappings
Date: Mon, 15 Mar 2021 11:42:35 -0700	[thread overview]
Message-ID: <2b23bbf7-bb32-4ed7-87ae-5e55b4bc6629@oracle.com> (raw)
In-Reply-To: <7868ec9c-0762-2a78-2dfc-2bd07dca15f5@huawei.com>

On 3/12/21 7:11 PM, Miaohe Lin wrote:
> On 2021/3/13 3:09, Mike Kravetz wrote:
>> On 3/1/21 4:05 AM, Miaohe Lin wrote:
>>> The current implementation of hugetlb_cgroup for shared mappings could have
>>> different behavior. Consider the following two scenarios:
>>>
>>> 1.Assume initial css reference count of hugetlb_cgroup is 1:
>>>   1.1 Call hugetlb_reserve_pages with from = 1, to = 2. So css reference
>>> count is 2 associated with 1 file_region.
>>>   1.2 Call hugetlb_reserve_pages with from = 2, to = 3. So css reference
>>> count is 3 associated with 2 file_region.
>>>   1.3 coalesce_file_region will coalesce these two file_regions into one.
>>> So css reference count is 3 associated with 1 file_region now.
>>>
>>> 2.Assume initial css reference count of hugetlb_cgroup is 1 again:
>>>   2.1 Call hugetlb_reserve_pages with from = 1, to = 3. So css reference
>>> count is 2 associated with 1 file_region.
>>>
>>> Therefore, we might have one file_region while holding one or more css
>>> reference counts. This inconsistency could lead to imbalanced css_get()
>>> and css_put() pair. If we do css_put one by one (i.g. hole punch case),
>>> scenario 2 would put one more css reference. If we do css_put all together
>>> (i.g. truncate case), scenario 1 will leak one css reference.
>>>
>>> The imbalanced css_get() and css_put() pair would result in a non-zero
>>> reference when we try to destroy the hugetlb cgroup. The hugetlb cgroup
>>> directory is removed __but__ associated resource is not freed. This might
>>> result in OOM or can not create a new hugetlb cgroup in a busy workload
>>> ultimately.
>>>
>>> In order to fix this, we have to make sure that one file_region must hold
>>> and only hold one css reference. So in coalesce_file_region case, we should
>>
>> I think this would sound/read better if stated as:
>> ... one file_region must hold exactly one css reference ...
>>
>> This phrase is repeated in comments throughout the patch.
>>
>>> release one css reference before coalescence. Also only put css reference
>>> when the entire file_region is removed.
>>>
>>> The last thing to note is that the caller of region_add() will only hold
>>> one reference to h_cg->css for the whole contiguous reservation region.
>>> But this area might be scattered when there are already some file_regions
>>> reside in it. As a result, many file_regions may share only one h_cg->css
>>> reference. In order to ensure that one file_region must hold and only hold
>>> one h_cg->css reference, we should do css_get() for each file_region and
>>> release the reference held by caller when they are done.
>>
>> Thanks for adding this to the commit message.
>>
>>>
>>> Fixes: 075a61d07a8e ("hugetlb_cgroup: add accounting for shared mappings")
>>> Reported-by: kernel test robot <lkp@intel.com> (auto build test ERROR)
>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>> Cc: stable@kernel.org
>>> ---
>>> v1->v2:
>>> 	Fix auto build test ERROR when CGROUP_HUGETLB is disabled.
>>> ---
>>>  include/linux/hugetlb_cgroup.h | 15 ++++++++++--
>>>  mm/hugetlb.c                   | 42 ++++++++++++++++++++++++++++++----
>>>  mm/hugetlb_cgroup.c            | 11 +++++++--
>>>  3 files changed, 60 insertions(+), 8 deletions(-)
>>
>> Just a few minor nits below, all in comments.  It is not required, but
>> would be nice to update these.  Code looks good.
>>
> 
> Many thanks for review! I will fix all this nits. Should I resend this patch or send another
> one to fix this? Just let me know which is the easiest one for you.
> 
> Thanks again. :)
> 

This is really Andrew's call as it goes through his tree.

I would suggest you just update the comments and send another verion.
In that way, Andrew can do whatever is easiest for him.
-- 
Mike Kravetz