Skip to content

Commit fe03ccc

Browse files
MiaoheLingregkh
MiaoheLin
authored and
gregkh
committed
hugetlb_cgroup: fix imbalanced css_get and css_put pair for shared mappings
commit d85aecf upstream. 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 exactly one css reference. So in coalesce_file_region case, we should 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 exactly one css reference, we should do css_get() for each file_region and release the reference held by caller when they are done. [linmiaohe@huawei.com: fix imbalanced css_get and css_put pair for shared mappings] Link: https://lkml.kernel.org/r/20210316023002.53921-1-linmiaohe@huawei.com Link: https://lkml.kernel.org/r/20210301120540.37076-1-linmiaohe@huawei.com Fixes: 075a61d ("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> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> Cc: Wanpeng Li <liwp.linux@gmail.com> Cc: Mina Almasry <almasrymina@google.com> Cc: <stable@vger.kernel.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent 269042e commit fe03ccc

3 files changed

Lines changed: 58 additions & 8 deletions

File tree

‎include/linux/hugetlb_cgroup.h‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,11 @@ static inline bool hugetlb_cgroup_disabled(void)
113113
return !cgroup_subsys_enabled(hugetlb_cgrp_subsys);
114114
}
115115

116+
static inline void hugetlb_cgroup_put_rsvd_cgroup(struct hugetlb_cgroup *h_cg)
117+
{
118+
css_put(&h_cg->css);
119+
}
120+
116121
extern int hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages,
117122
struct hugetlb_cgroup **ptr);
118123
extern int hugetlb_cgroup_charge_cgroup_rsvd(int idx, unsigned long nr_pages,
@@ -138,7 +143,8 @@ extern void hugetlb_cgroup_uncharge_counter(struct resv_map *resv,
138143

139144
extern void hugetlb_cgroup_uncharge_file_region(struct resv_map *resv,
140145
struct file_region *rg,
141-
unsigned long nr_pages);
146+
unsigned long nr_pages,
147+
bool region_del);
142148

143149
extern void hugetlb_cgroup_file_init(void) __init;
144150
extern void hugetlb_cgroup_migrate(struct page *oldhpage,
@@ -147,7 +153,8 @@ extern void hugetlb_cgroup_migrate(struct page *oldhpage,
147153
#else
148154
static inline void hugetlb_cgroup_uncharge_file_region(struct resv_map *resv,
149155
struct file_region *rg,
150-
unsigned long nr_pages)
156+
unsigned long nr_pages,
157+
bool region_del)
151158
{
152159
}
153160

@@ -185,6 +192,10 @@ static inline bool hugetlb_cgroup_disabled(void)
185192
return true;
186193
}
187194

195+
static inline void hugetlb_cgroup_put_rsvd_cgroup(struct hugetlb_cgroup *h_cg)
196+
{
197+
}
198+
188199
static inline int hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages,
189200
struct hugetlb_cgroup **ptr)
190201
{

‎mm/hugetlb.c‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,17 @@ static void record_hugetlb_cgroup_uncharge_info(struct hugetlb_cgroup *h_cg,
285285
nrg->reservation_counter =
286286
&h_cg->rsvd_hugepage[hstate_index(h)];
287287
nrg->css = &h_cg->css;
288+
/*
289+
* The caller will hold exactly one h_cg->css reference for the
290+
* whole contiguous reservation region. But this area might be
291+
* scattered when there are already some file_regions reside in
292+
* it. As a result, many file_regions may share only one css
293+
* reference. In order to ensure that one file_region must hold
294+
* exactly one h_cg->css reference, we should do css_get for
295+
* each file_region and leave the reference held by caller
296+
* untouched.
297+
*/
298+
css_get(&h_cg->css);
288299
if (!resv->pages_per_hpage)
289300
resv->pages_per_hpage = pages_per_huge_page(h);
290301
/* pages_per_hpage should be the same for all entries in
@@ -298,6 +309,14 @@ static void record_hugetlb_cgroup_uncharge_info(struct hugetlb_cgroup *h_cg,
298309
#endif
299310
}
300311

312+
static void put_uncharge_info(struct file_region *rg)
313+
{
314+
#ifdef CONFIG_CGROUP_HUGETLB
315+
if (rg->css)
316+
css_put(rg->css);
317+
#endif
318+
}
319+
301320
static bool has_same_uncharge_info(struct file_region *rg,
302321
struct file_region *org)
303322
{
@@ -321,6 +340,7 @@ static void coalesce_file_region(struct resv_map *resv, struct file_region *rg)
321340
prg->to = rg->to;
322341

323342
list_del(&rg->link);
343+
put_uncharge_info(rg);
324344
kfree(rg);
325345

326346
rg = prg;
@@ -332,6 +352,7 @@ static void coalesce_file_region(struct resv_map *resv, struct file_region *rg)
332352
nrg->from = rg->from;
333353

334354
list_del(&rg->link);
355+
put_uncharge_info(rg);
335356
kfree(rg);
336357
}
337358
}
@@ -664,7 +685,7 @@ static long region_del(struct resv_map *resv, long f, long t)
664685

665686
del += t - f;
666687
hugetlb_cgroup_uncharge_file_region(
667-
resv, rg, t - f);
688+
resv, rg, t - f, false);
668689

669690
/* New entry for end of split region */
670691
nrg->from = t;
@@ -685,21 +706,21 @@ static long region_del(struct resv_map *resv, long f, long t)
685706
if (f <= rg->from && t >= rg->to) { /* Remove entire region */
686707
del += rg->to - rg->from;
687708
hugetlb_cgroup_uncharge_file_region(resv, rg,
688-
rg->to - rg->from);
709+
rg->to - rg->from, true);
689710
list_del(&rg->link);
690711
kfree(rg);
691712
continue;
692713
}
693714

694715
if (f <= rg->from) { /* Trim beginning of region */
695716
hugetlb_cgroup_uncharge_file_region(resv, rg,
696-
t - rg->from);
717+
t - rg->from, false);
697718

698719
del += t - rg->from;
699720
rg->from = t;
700721
} else { /* Trim end of region */
701722
hugetlb_cgroup_uncharge_file_region(resv, rg,
702-
rg->to - f);
723+
rg->to - f, false);
703724

704725
del += rg->to - f;
705726
rg->to = f;
@@ -5189,13 +5210,25 @@ int hugetlb_reserve_pages(struct inode *inode,
51895210
*/
51905211
long rsv_adjust;
51915212

5213+
/*
5214+
* hugetlb_cgroup_uncharge_cgroup_rsvd() will put the
5215+
* reference to h_cg->css. See comment below for detail.
5216+
*/
51925217
hugetlb_cgroup_uncharge_cgroup_rsvd(
51935218
hstate_index(h),
51945219
(chg - add) * pages_per_huge_page(h), h_cg);
51955220

51965221
rsv_adjust = hugepage_subpool_put_pages(spool,
51975222
chg - add);
51985223
hugetlb_acct_memory(h, -rsv_adjust);
5224+
} else if (h_cg) {
5225+
/*
5226+
* The file_regions will hold their own reference to
5227+
* h_cg->css. So we should release the reference held
5228+
* via hugetlb_cgroup_charge_cgroup_rsvd() when we are
5229+
* done.
5230+
*/
5231+
hugetlb_cgroup_put_rsvd_cgroup(h_cg);
51995232
}
52005233
}
52015234
return 0;

‎mm/hugetlb_cgroup.c‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,8 @@ void hugetlb_cgroup_uncharge_counter(struct resv_map *resv, unsigned long start,
391391

392392
void hugetlb_cgroup_uncharge_file_region(struct resv_map *resv,
393393
struct file_region *rg,
394-
unsigned long nr_pages)
394+
unsigned long nr_pages,
395+
bool region_del)
395396
{
396397
if (hugetlb_cgroup_disabled() || !resv || !rg || !nr_pages)
397398
return;
@@ -400,7 +401,12 @@ void hugetlb_cgroup_uncharge_file_region(struct resv_map *resv,
400401
!resv->reservation_counter) {
401402
page_counter_uncharge(rg->reservation_counter,
402403
nr_pages * resv->pages_per_hpage);
403-
css_put(rg->css);
404+
/*
405+
* Only do css_put(rg->css) when we delete the entire region
406+
* because one file_region must hold exactly one css reference.
407+
*/
408+
if (region_del)
409+
css_put(rg->css);
404410
}
405411
}
406412

0 commit comments

Comments
 (0)