lkml.org 
[lkml]   [2012]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 2/2] hugepages: Fix use after free bug in "quota" handling
On Wed, Mar 07, 2012 at 04:27:20PM -0800, Andrew Morton wrote:
> On Wed, 7 Mar 2012 15:48:14 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > hugetlbfs_{get,put}_quota() are badly named. They don't interact with the
> > general quota handling code, and they don't much resemble its behaviour.
> > Rather than being about maintaining limits on on-disk block usage by
> > particular users, they are instead about maintaining limits on in-memory
> > page usage (including anonymous MAP_PRIVATE copied-on-write pages)
> > associated with a particular hugetlbfs filesystem instance.
> >
> > Worse, they work by having callbacks to the hugetlbfs filesystem code from
> > the low-level page handling code, in particular from free_huge_page().
> > This is a layering violation of itself, but more importantly, if the kernel
> > does a get_user_pages() on hugepages (which can happen from KVM amongst
> > others), then the free_huge_page() can be delayed until after the
> > associated inode has already been freed. If an unmount occurs at the
> > wrong time, even the hugetlbfs superblock where the "quota" limits are
> > stored may have been freed.
> >
> > Andrew Barry proposed a patch to fix this by having hugepages, instead of
> > storing a pointer to their address_space and reaching the superblock from
> > there, had the hugepages store pointers directly to the superblock, bumping
> > the reference count as appropriate to avoid it being freed. Andrew Morton
> > rejected that version, however, on the grounds that it made the existing
> > layering violation worse.
> >
> > This is a reworked version of Andrew's patch, which removes the extra, and
> > some of the existing, layering violation. It works by introducing the
> > concept of a hugepage "subpool" at the lower hugepage mm layer - that is
> > a finite logical pool of hugepages to allocate from. hugetlbfs now creates
> > a subpool for each filesystem instance with a page limit set, and a pointer
> > to the subpool gets added to each allocated hugepage, instead of the
> > address_space pointer used now. The subpool has its own lifetime and is
> > only freed once all pages in it _and_ all other references to it (i.e.
> > superblocks) are gone.
> >
> > subpools are optional - a NULL subpool pointer is taken by the code to mean
> > that no subpool limits are in effect.
> >
> > Previous discussion of this bug found in: "Fix refcounting in hugetlbfs
> > quota handling.". See: https://lkml.org/lkml/2011/8/11/28 or
> > http://marc.info/?l=linux-mm&m=126928970510627&w=1
> >
> > v2: Fixed a bug spotted by Hillf Danton, and removed the extra parameter to
> > alloc_huge_page() - since it already takes the vma, it is not necessary.
>
> Looks good - thanks for doing this.
>
> Some comments, nothing serious:

Cleanup patch addressing these comments below. I've done this as a
diff applied on top of the original patch.

From 756a0901a21cce59cc82d0727f6463c3f71eecbf Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Thu, 8 Mar 2012 13:09:57 +1100
Subject: [PATCH] Cleanups for "hugepages: Fix use after free bug in 'quota'
handling"

This patch makes some cleanups to an earlier patch of mine fixing a
use after free bug in the hugetlbfs "quota" handling (actually
per-filesystem page limits, not related to normal use of quotas).

These cleanups and extra documentation were mostly suggested by Andrew
Morton.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
fs/hugetlbfs/inode.c | 3 +-
include/linux/hugetlb.h | 16 +++++++++--
mm/hugetlb.c | 69 +++++++++++++++++++++++++++-------------------
3 files changed, 54 insertions(+), 34 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 74c6ba2..536672a 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -910,8 +910,7 @@ hugetlbfs_fill_super(struct super_block *sb, void *data, int silent)
sb->s_root = root;
return 0;
out_free:
- if (sbinfo->spool)
- kfree(sbinfo->spool);
+ kfree(sbinfo->spool);
kfree(sbinfo);
return -ENOMEM;
}
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index cf01817..8fdb595 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -14,13 +14,23 @@ struct user_struct;
#include <linux/shm.h>
#include <asm/tlbflush.h>

+/*
+ * A hugepage subpool represents a notional finite bucket of
+ * hugepages. They're used by the hugetlbfs code to implement
+ * per-filesystem-instance limits on hugepage usage.
+ */
struct hugepage_subpool {
spinlock_t lock;
- long count;
- long max_hpages, used_hpages;
+ /* Total number of hugepages in the subpool */
+ unsigned long max_hpages;
+ /* Number of currently allocated hugepages in the subpool */
+ unsigned long used_hpages;
+ /* Reference count of anything else keeping the subpool in existence */
+ /* (e.g. hugetlbfs superblocks) */
+ unsigned refcount;
};

-struct hugepage_subpool *hugepage_new_subpool(long nr_blocks);
+struct hugepage_subpool *hugepage_new_subpool(unsigned long nr_blocks);
void hugepage_put_subpool(struct hugepage_subpool *spool);

int PageHuge(struct page *page);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 36b38b3a..aa6316b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -53,19 +53,22 @@ static unsigned long __initdata default_hstate_size;
*/
static DEFINE_SPINLOCK(hugetlb_lock);

-static inline void unlock_or_release_subpool(struct hugepage_subpool *spool)
+static inline void unlock_and_release_subpool(struct hugepage_subpool *spool)
{
- bool free = (spool->count == 0) && (spool->used_hpages == 0);
+ bool free = (spool->refcount == 0) && (spool->used_hpages == 0);

spin_unlock(&spool->lock);

- /* If no pages are used, and no other handles to the subpool
- * remain, free the subpool the subpool remain */
+ /*
+ * If there are no pages left still in the subpool, _and_
+ * there are no other references to it, we can free the
+ * subpool.
+ */
if (free)
kfree(spool);
}

-struct hugepage_subpool *hugepage_new_subpool(long nr_blocks)
+struct hugepage_subpool *hugepage_new_subpool(unsigned long nr_blocks)
{
struct hugepage_subpool *spool;

@@ -74,7 +77,7 @@ struct hugepage_subpool *hugepage_new_subpool(long nr_blocks)
return NULL;

spin_lock_init(&spool->lock);
- spool->count = 1;
+ spool->refcount = 1;
spool->max_hpages = nr_blocks;
spool->used_hpages = 0;

@@ -84,13 +87,17 @@ struct hugepage_subpool *hugepage_new_subpool(long nr_blocks)
void hugepage_put_subpool(struct hugepage_subpool *spool)
{
spin_lock(&spool->lock);
- BUG_ON(!spool->count);
- spool->count--;
- unlock_or_release_subpool(spool);
+ BUG_ON(!spool->refcount);
+ spool->refcount--;
+ unlock_and_release_subpool(spool);
}

-static int hugepage_subpool_get_pages(struct hugepage_subpool *spool,
- long delta)
+/*
+ * Allocate some pages from a subpool, or fail if there aren't enough
+ * pages left
+ */
+static int hugepage_subpool_alloc_pages(struct hugepage_subpool *spool,
+ unsigned long delta)
{
int ret = 0;

@@ -98,27 +105,31 @@ static int hugepage_subpool_get_pages(struct hugepage_subpool *spool,
return 0;

spin_lock(&spool->lock);
- if ((spool->used_hpages + delta) <= spool->max_hpages) {
+ if ((spool->used_hpages + delta) <= spool->max_hpages)
spool->used_hpages += delta;
- } else {
+ else
ret = -ENOMEM;
- }
spin_unlock(&spool->lock);

return ret;
}

-static void hugepage_subpool_put_pages(struct hugepage_subpool *spool,
- long delta)
+/*
+ * Release some pages back to a subpool
+ */
+static void hugepage_subpool_release_pages(struct hugepage_subpool *spool,
+ unsigned long delta)
{
if (!spool)
return;

spin_lock(&spool->lock);
spool->used_hpages -= delta;
- /* If hugetlbfs_put_super couldn't free spool due to
- * an outstanding quota reference, free it now. */
- unlock_or_release_subpool(spool);
+ /*
+ * If hugetlbfs_put_super couldn't free the subpool due to
+ * pages remaining allocated from it, free it now.
+ */
+ unlock_and_release_subpool(spool);
}

static inline struct hugepage_subpool *subpool_inode(struct inode *inode)
@@ -611,9 +622,9 @@ static void free_huge_page(struct page *page)
*/
struct hstate *h = page_hstate(page);
int nid = page_to_nid(page);
- struct hugepage_subpool *spool =
- (struct hugepage_subpool *)page_private(page);
+ struct hugepage_subpool *spool;

+ spool = (struct hugepage_subpool *)page_private(page);
set_page_private(page, 0);
page->mapping = NULL;
BUG_ON(page_count(page));
@@ -629,7 +640,7 @@ static void free_huge_page(struct page *page)
enqueue_huge_page(h, page);
}
spin_unlock(&hugetlb_lock);
- hugepage_subpool_put_pages(spool, 1);
+ hugepage_subpool_release_pages(spool, 1);
}

static void prep_new_huge_page(struct hstate *h, struct page *page, int nid)
@@ -1114,7 +1125,7 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
if (chg < 0)
return ERR_PTR(-VM_FAULT_OOM);
if (chg)
- if (hugepage_subpool_get_pages(spool, chg))
+ if (hugepage_subpool_alloc_pages(spool, chg))
return ERR_PTR(-VM_FAULT_SIGBUS);

spin_lock(&hugetlb_lock);
@@ -1124,7 +1135,7 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
if (!page) {
page = alloc_buddy_huge_page(h, NUMA_NO_NODE);
if (!page) {
- hugepage_subpool_put_pages(spool, chg);
+ hugepage_subpool_release_pages(spool, chg);
return ERR_PTR(-VM_FAULT_SIGBUS);
}
}
@@ -2166,7 +2177,7 @@ static void hugetlb_vm_op_close(struct vm_area_struct *vma)

if (reserve) {
hugetlb_acct_memory(h, -reserve);
- hugepage_subpool_put_pages(spool, reserve);
+ hugepage_subpool_release_pages(spool, reserve);
}
}
}
@@ -2395,7 +2406,7 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
*/
address = address & huge_page_mask(h);
pgoff = vma_hugecache_offset(h, vma, address);
- mapping = vma->vm_file->f_dentry->d_inode->i_mapping;
+ mapping = vma->vm_file->f_mapping;

/*
* Take the mapping lock for the duration of the table walk. As
@@ -2981,7 +2992,7 @@ int hugetlb_reserve_pages(struct inode *inode,
return chg;

/* There must be enough pages in the subpool for the mapping */
- if (hugepage_subpool_get_pages(spool, chg))
+ if (hugepage_subpool_alloc_pages(spool, chg))
return -ENOSPC;

/*
@@ -2990,7 +3001,7 @@ int hugetlb_reserve_pages(struct inode *inode,
*/
ret = hugetlb_acct_memory(h, chg);
if (ret < 0) {
- hugepage_subpool_put_pages(spool, chg);
+ hugepage_subpool_release_pages(spool, chg);
return ret;
}

@@ -3020,7 +3031,7 @@ void hugetlb_unreserve_pages(struct inode *inode, long offset, long freed)
inode->i_blocks -= (blocks_per_huge_page(h) * freed);
spin_unlock(&inode->i_lock);

- hugepage_subpool_put_pages(spool, (chg - freed));
+ hugepage_subpool_release_pages(spool, (chg - freed));
hugetlb_acct_memory(h, -(chg - freed));
}

--
1.7.9.1


--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson

\
 
 \ /
  Last update: 2012-03-09 04:43    [W:0.118 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site