Re: [PATCH 2/2] hugepages: Fix use after free bug in "quota" handling

[David Gibson <dwg@au1.ibm.com>]

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