hugetlb: fix resv_map leak in error path

PATCH In review Dave Hansen 4 participants 6 messages May 21, 2012 – May 22, 2012

Subsystems (3)

Stable branch Memory management HugeTLB

lore

Share & tools

lore

Dave Hansen

author

2012-05-21 20:28 UTC #0 lore raw

PATCH 1/1 hugetlb: fix resv_map leak in error path

When called for anonymous (non-shared) mappings, hugetlb_reserve_pages() does a resv_map_alloc(). It depends on code in hugetlbfs's vm_ops->close() to release that allocation.

However, in the mmap() failure path, we do a plain unmap_region() without the remove_vma() which actually calls vm_ops->close().

This is a decent fix. This leak could get reintroduced if new code (say, after hugetlb_reserve_pages() in hugetlbfs_file_mmap()) decides to return an error. But, I think it would have to unroll the reservation anyway.

This hasn't been extensively tested. Pretty much compile and boot tested along with Christoph's test case:

	http://marc.info/?l=linux-mm&m=133728900729735

Signed-off-by Dave Hansen <[email protected]>

Acked-by Mel Gorman <[email protected]>

ecked-by KOSAKI Motohiro <[email protected]>

Reported/tested-by: Christoph Lameter <[email protected]>

---

 linux-2.6.git-dave/mm/hugetlb.c |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

mm/hugetlb.c renamed from mm/hugetlb.c~hugetlb-fix-leak +22 -6

diff -puN mm/hugetlb.c~hugetlb-fix-leak mm/hugetlb.c
@@ -2157,6 +2157,15 @@ static void hugetlb_vm_op_open(struct vm
 		kref_get(&reservations->refs);
 }
 
+static void resv_map_put(struct vm_area_struct *vma)
+{
+	struct resv_map *reservations = vma_resv_map(vma);
+
+	if (!reservations)
+		return;
+	kref_put(&reservations->refs, resv_map_release);
+}
+
 static void hugetlb_vm_op_close(struct vm_area_struct *vma)
 {
 	struct hstate *h = hstate_vma(vma);

@@ -2173,7 +2182,7 @@ static void hugetlb_vm_op_close(struct v
 		reserve = (end - start) -
 			region_count(&reservations->regions, start, end);
 
-		kref_put(&reservations->refs, resv_map_release);
+		resv_map_put(vma);
 
 		if (reserve) {
 			hugetlb_acct_memory(h, -reserve);

@@ -2990,12 +2999,16 @@ int hugetlb_reserve_pages(struct inode *
 		set_vma_resv_flags(vma, HPAGE_RESV_OWNER);
 	}
 
-	if (chg < 0)
-		return chg;
+	if (chg < 0) {
+		ret = chg;
+		goto out_err;
+	}
 
 	/* There must be enough pages in the subpool for the mapping */
-	if (hugepage_subpool_get_pages(spool, chg))
-		return -ENOSPC;
+	if (hugepage_subpool_get_pages(spool, chg)) {
+		ret = -ENOSPC;
+		goto out_err;
+	}
 
 	/*
 	 * Check enough hugepages are available for the reservation.

@@ -3004,7 +3017,7 @@ int hugetlb_reserve_pages(struct inode *
 	ret = hugetlb_acct_memory(h, chg);
 	if (ret < 0) {
 		hugepage_subpool_put_pages(spool, chg);
-		return ret;
+		goto out_err;
 	}
 
 	/*

@@ -3021,6 +3034,9 @@ int hugetlb_reserve_pages(struct inode *
 	if (!vma || vma->vm_flags & VM_MAYSHARE)
 		region_add(&inode->i_mapping->private_list, from, to);
 	return 0;
+out_err:
+	resv_map_put(vma);
+	return ret;
 }
 
 void hugetlb_unreserve_pages(struct inode *inode, long offset, long freed)

diff -puN Documentation/stable_kernel_rules.txt~hugetlb-fix-leak Documentation/stable_kernel_rules.txt _

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KOSAKI Motohiro

2012-05-21 22:00 UTC #1 lore raw

Quoted 12 lines · Dave Hansen

When called for anonymous (non-shared) mappings, hugetlb_reserve_pages() does a resv_map_alloc(). It depends on code in hugetlbfs's vm_ops->close() to release that allocation.
However, in the mmap() failure path, we do a plain unmap_region() without the remove_vma() which actually calls vm_ops->close().
This is a decent fix. This leak could get reintroduced if new code (say, after hugetlb_reserve_pages() in hugetlbfs_file_mmap()) decides to return an error. But, I think it would have to unroll the reservation anyway.

Quoted 8 lines

This hasn't been extensively tested. Pretty much compile and boot tested along with Christoph's test case:
	http://marc.info/?l=linux-mm&m=133728900729735
Signed-off-by Dave Hansen <[email protected]>
Acked-by Mel Gorman <[email protected]>
ecked-by KOSAKI Motohiro <[email protected]>

typo. ;-)

Quoted 1 line

Reported/tested-by: Christoph Lameter <[email protected]>

Andrew Morton

maintainer

2012-05-22 20:45 UTC #2 lore raw

In reply to Dave Hansen

Quoted 11 lines · On Mon, 21 May 2012 13:28:14 -0700 Dave Hansen <[email protected]> wrote:

When called for anonymous (non-shared) mappings, hugetlb_reserve_pages() does a resv_map_alloc(). It depends on code in hugetlbfs's vm_ops->close() to release that allocation.
However, in the mmap() failure path, we do a plain unmap_region() without the remove_vma() which actually calls vm_ops->close().
This is a decent fix. This leak could get reintroduced if new code (say, after hugetlb_reserve_pages() in hugetlbfs_file_mmap()) decides to return an error. But, I think it would have to unroll the reservation anyway.

How far back does this bug go? The patch applies to 3.4 but gets rejects in 3.3 and earlier.

Quoted 4 lines

This hasn't been extensively tested. Pretty much compile and boot tested along with Christoph's test case:
	http://marc.info/?l=linux-mm&m=133728900729735

That isn't my favoritest ever changelog text :(

Dave Hansen

author

2012-05-22 21:01 UTC #3 lore raw

Quoted 10 lines · Andrew Morton

Quoted 7 lines · On Mon, 21 May 2012 13:28:14 -0700 Dave Hansen <[email protected]> wrote:
When called for anonymous (non-shared) mappings, hugetlb_reserve_pages() does a resv_map_alloc(). It depends on code in hugetlbfs's vm_ops->close() to release that allocation.
However, in the mmap() failure path, we do a plain unmap_region() without the remove_vma() which actually calls vm_ops->close().

Quoted 7 lines

Quoted 4 lines
This is a decent fix. This leak could get reintroduced if new code (say, after hugetlb_reserve_pages() in hugetlbfs_file_mmap()) decides to return an error. But, I think it would have to unroll the reservation anyway.
How far back does this bug go? The patch applies to 3.4 but gets rejects in 3.3 and earlier.

commit 17c9d12e126cb0de8d535dc1908c4819d712bc68
Date: Wed Feb 11 16:34:16 2009 +0000

So, ~2.6.30.

I don't think it existed before that. The code was there, but the ordering made it OK.

Christoph Lameter

2012-05-22 21:05 UTC #4 lore raw

In reply to Andrew Morton

Quoted 10 lines · Andrew Morton

Quoted 7 lines · On Mon, 21 May 2012 13:28:14 -0700 Dave Hansen <[email protected]> wrote:
When called for anonymous (non-shared) mappings, hugetlb_reserve_pages() does a resv_map_alloc(). It depends on code in hugetlbfs's vm_ops->close() to release that allocation.
However, in the mmap() failure path, we do a plain unmap_region() without the remove_vma() which actually calls vm_ops->close().

Quoted 7 lines

Quoted 4 lines
This is a decent fix. This leak could get reintroduced if new code (say, after hugetlb_reserve_pages() in hugetlbfs_file_mmap()) decides to return an error. But, I think it would have to unroll the reservation anyway.
How far back does this bug go? The patch applies to 3.4 but gets rejects in 3.3 and earlier.

The earliest that I have seen it on was 2.6.32. I have rediffed the patch against 2.6.32 and 3.2.0.

----

Andrew Morton

maintainer

2012-05-22 21:28 UTC #5 lore raw

Quoted 5 lines · On Tue, 22 May 2012 16:05:02 -0500 (CDT) Christoph Lameter <[email protected]> wrote:

Quoted 2 lines
How far back does this bug go? The patch applies to 3.4 but gets rejects in 3.3 and earlier.
The earliest that I have seen it on was 2.6.32. I have rediffed the patch against 2.6.32 and 3.2.0.

Great, thanks. I did

: From: Dave Hansen <[email protected]>
: Subject: hugetlb: fix resv_map leak in error path
:
: When called for anonymous (non-shared) mappings, hugetlb_reserve_pages() : does a resv_map_alloc(). It depends on code in hugetlbfs's
: vm_ops->close() to release that allocation.
:
: However, in the mmap() failure path, we do a plain unmap_region() without : the remove_vma() which actually calls vm_ops->close().
:
: This is a decent fix. This leak could get reintroduced if new code (say, : after hugetlb_reserve_pages() in hugetlbfs_file_mmap()) decides to return : an error. But, I think it would have to unroll the reservation anyway. :
: Christoph's test case:
:
: http://marc.info/?l=linux-mm&m=133728900729735
:
: This patch applies to 3.4 and later. A version for earlier kernels is at : https://lkml.org/lkml/2012/5/22/418.
:
: Signed-off-by: Dave Hansen <[email protected]>
: Acked-by: Mel Gorman <[email protected]>
: Acked-by: KOSAKI Motohiro <[email protected]>
: Reported-by: Christoph Lameter <[email protected]>
: Tested-by: Christoph Lameter <[email protected]>
: Cc: Andrea Arcangeli <[email protected]>
: Cc: <[email protected]> [2.6.32+]