Re: [PATCH]: fix the bug of __free_pages() of mm/page_alloc.c

[zhan rongkai <zhanrk@gmail.com>]

On Thu, 20 Jan 2005 14:31:34 +0000, Russell King
<rmk+lkml@arm.linux.org.uk> wrote:
> On Thu, Jan 20, 2005 at 09:34:17PM +0800, zhan rongkai wrote:
> > [PATCH]: fix the bug of __free_pages() of mm/page_alloc.c
> > =========================================================
> >
> > The buddy allocator's __free_pages() function seems to be buggy.
> >
> > The following codes are from kernel 2.6.10:
> >
> > fastcall void __free_pages(struct page *page, unsigned int order)
> > {
> >       if (!PageReserved(page) && put_page_testzero(page)) {
> >               if (order == 0)
> >                       free_hot_page(page);
> >               else
> >                       __free_pages_ok(page, order);
> >       }
> > }
> >
> > As you know, before truely freeing all pages, this function calls
> > put_page_testzero(page) to
> > drop the refcount of the pages.
> >
> > But, in fact the macro put_page_testzero(page) **only** drops **one**
> > page's refcount.
> > Therefore, if (order > 0), the refcounts of (page+1) ..
> > (page+(1<<order)-1) are unchanged!
> > This will cause __free_pages_ok() to dump stack, because it finds some
> > pages' page_count()
> > are not zero!
> 
> When you allocate a page with order > 0, the first 0-order page has a
> refcount of 1, and the remaining 0-order pages have a refcount of 0.

Thank you for telling me this point.

> If you're triggering this check, I suspect you're fiddling about with
> the individual pages (using get_page on them individually?) which is
> a no-no.
> 
> --
> Russell King
> 

Oh, I forget to tell you that my CPU has no MMU, sorry:-)
Let's see the function set_page_refs() which is called by
prep_new_page() function:

static inline void set_page_refs(struct page *page, int order)
{
#ifdef CONFIG_MMU
	set_page_count(page, 1);
#else
	int i;

	/*
	 * We need to reference all the pages for this order, otherwise if
	 * anyone accesses one of the pages with (get/put) it will be freed.
	 */
	for (i = 0; i < (1 << order); i++)
		set_page_count(page+i, 1);
#endif /* CONFIG_MMU */
}

We can see that it sets all pages' refcount to 1 when there is no MMU.

My previous patch is wrong. Here is new one:
--------------------------------------------

--- linux-2.6.10.orig/mm/page_alloc.c	2004-12-25 05:33:51.000000000 +0800
+++ linux-2.6.10/mm/page_alloc.c	2005-01-21 11:34:57.000000000 +0800
@@ -787,8 +787,23 @@
 }
 
 fastcall void __free_pages(struct page *page, unsigned int order)
-{
-	if (!PageReserved(page) && put_page_testzero(page)) {
+{	
+	if (!PageReserved(page)) {
+#ifdef CONFIG_MMU
+		if (!put_page_testzero(page))
+			return;
+#else
+		int i, result = 1;
+
+		/*
+		 * We need to de-reference all the pages for this order -- see
set_page_refs()
+		 */
+		 for (i = 0; i < (1 << order); i++)
+			 result &= put_page_testzero(page);
+		 if (!result)
+			 BUG();
+#endif /* CONFIG_MMU */
+
 		if (order == 0)
 			free_hot_page(page);
 		else

-- 
Rongkai Zhan