lkml.org 
[lkml]   [2012]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] hugepages: Fix use after free bug in "quota" handling
On Thu, Mar 08, 2012 at 10:00:57AM +0530, Aneesh Kumar K.V wrote:
> On Wed, 7 Mar 2012 15:48:14 +1100, David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > out_free:
> > + if (sbinfo->spool)
> > + kfree(sbinfo->spool);
>
> kfree() should handle NULL

I've already changed that at akpm's suggestion.

> > @@ -53,6 +53,84 @@ static unsigned long __initdata default_hstate_size;
> > */
> > static DEFINE_SPINLOCK(hugetlb_lock);
> >
> > +static inline void unlock_or_release_subpool(struct hugepage_subpool *spool)
> > +{
> > + bool free = (spool->count == 0) && (spool->used_hpages == 0);
> > +
>
> I have not see that style in other part of kernel. May be with proper
>
> if (spool->count == 0 && spool->used_hpages == 0)
> free = 1
>
> > + spin_unlock(&spool->lock);
> > +
>
>
> Having the spin_lock held across functions is also strange. Since there
> are only two callers, may be this can be inlined in the callers ?

It's a bit unusual, yes, but it seems to me clearer to outline this
operation in a function.

> > +static inline struct hugepage_subpool *subpool_inode(struct inode
> > *inode)
>
> s/subpool_inode/subpool_from_inode/ ?
>
> > +{
> > + return HUGETLBFS_SB(inode->i_sb)->spool;
> > +}
> > +
> > +static inline struct hugepage_subpool *subpool_vma(struct
> > vm_area_struct *vma)
>
> s/subpool_vma/subpool_from_vma/ ?

These functions are named to match hstate_inode() and hstate_vma().

> > +{
> > + return subpool_inode(vma->vm_file->f_dentry->d_inode);
> > +}
> > +
> > /*
> > * Region tracking -- allows tracking of reservations and instantiated pages
> > * across the pages in a mapping.
> > @@ -533,9 +611,9 @@ static void free_huge_page(struct page *page)
> > */
> > struct hstate *h = page_hstate(page);
> > int nid = page_to_nid(page);
> > - struct address_space *mapping;
> > + struct hugepage_subpool *spool =
> > + (struct hugepage_subpool *)page_private(page);
> >
> > - mapping = (struct address_space *) page_private(page);
> > set_page_private(page, 0);
> > page->mapping = NULL;
> > BUG_ON(page_count(page));
> > @@ -551,8 +629,7 @@ static void free_huge_page(struct page *page)
> > enqueue_huge_page(h, page);
> > }
> > spin_unlock(&hugetlb_lock);
> > - if (mapping)
> > - hugetlb_put_quota(mapping, 1);
> > + hugepage_subpool_put_pages(spool, 1);
>
>
> We would still need the if () checking there. When we do echo x >
> /proc/sys/vm/nr_hugepages we would call free_huge_page to prepare new
> huge page pool. But we don't have page_private set for them.

No, this is correct. hugepage_subpool_put_pages() is built to be a
nop if spool == NULL.

--
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-08 16:41    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site