Messages in this thread | | | Date | Fri, 9 Mar 2012 01:18:16 +1100 | From | David Gibson <> | Subject | Re: [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
| |