hugetlb: fix resv_map leak in error path
Subsystems (2)
RFC 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.
Comments?
---
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
@@ -2157,6 +2157,15 @@ static void hugetlb_vm_op_open(struct vmkref_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)
_
Quoted 13 lines · Dave Hansen
When called for anonymous (non-shared) mappings,
hugetlb_reserve_pages()does aresv_map_alloc(). It depends on code in hugetlbfs'svm_ops->close()to release that allocation.However, in the
mmap()failure path, we do a plainunmap_region()without theremove_vma()which actually callsvm_ops->close().This is a decent fix. This leak could get reintroduced if new code (say, after
hugetlb_reserve_pages()inhugetlbfs_file_mmap()) decides to return an error. But, I think it would have to unroll the reservation anyway.
Quoted 6 lines
This hasn't been extensively tested. Pretty much compile and boot tested along with Christoph's test case.
Comments?
Signed-off-by Dave Hansen <[email protected]>
Signature · 9 lines
Mel Gorman SUSE Labs -- 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>
Quoted 3 lines · Mel Gorman
Quoted 1 line
Signed-off-by Dave Hansen <[email protected]>Acked-by Mel Gorman <[email protected]>
Reported/tested-by: Christoph Lameter <[email protected]>
--
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>
Quoted 12 lines · Dave Hansen
When called for anonymous (non-shared) mappings,
hugetlb_reserve_pages()does aresv_map_alloc(). It depends on code in hugetlbfs'svm_ops->close()to release that allocation.However, in the
mmap()failure path, we do a plainunmap_region()without theremove_vma()which actually callsvm_ops->close().This is a decent fix. This leak could get reintroduced if new code (say, after
hugetlb_reserve_pages()inhugetlbfs_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.
Comments?
Signed-off-by Dave Hansen <[email protected]>---
I don't think this is cleaner fix. but I also think we should fix the leak asap. so Let's simple fix first.
--
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>