Re: [PATCH][2.4.23-pre4] page_alloc uninitialised variable bug

[Andrea Arcangeli <andrea@suse.de>]

On Sat, Sep 13, 2003 at 09:39:19PM +0200, Mikael Pettersson wrote:
> mm/page_alloc in 2.4.23-pre4 triggers this warning:
> 
> page_alloc.c: In function `balance_classzone':
> page_alloc.c:259: warning: `__freed' might be used uninitialized in this function
> 
> There is a case where balance_classzone() returns NULL and an
> uninitialised value in *freed, and the caller, __alloc_pages(),
> also uses the uninitialised value.
> 
> Changing balance_classzone() to not do "*freed = __freed;" in
> this case is inadequate since __alloc_pages() will still look
> at the bogus value. Someone needs to initialise the damn thing;
> the patch below makes balance_classzone() do it.
> 
> /Mikael
> 
> --- linux-2.4.23-pre4/mm/page_alloc.c.~1~	2003-09-13 19:11:48.000000000 +0200
> +++ linux-2.4.23-pre4/mm/page_alloc.c	2003-09-13 20:58:56.000000000 +0200
> @@ -256,7 +256,7 @@
>  static struct page * balance_classzone(zone_t * classzone, unsigned int gfp_mask, unsigned int order, int * freed)
>  {
>  	struct page * page = NULL;
> -	int __freed;
> +	int __freed = 0;
>  
>  	if (!(gfp_mask & __GFP_WAIT))
>  		goto out;

this wasn't a bug, there is absolutely no need to initialize that, it's
just that gcc is not smart enough to understand that automatically and
it generated a false positive.

the real cleanup is to nuke the !(gfp_mask & __GFP_WAIT) that can't ever
happen if you follow the code. This is exactly what my tree does. Not
sure how this part wasn't merged into mainline.

See my tree:

static struct page * FASTCALL(balance_classzone(zone_t *, unsigned int,
unsigned int, int *));
static struct page * balance_classzone(zone_t * classzone, unsigned int
gfp_mask, unsigned int order, int * freed)
{
	struct page * page = NULL;
	int __freed;

	if (in_interrupt())
		BUG();

	if (current->local_page.page)
		BUG();
	current->local_page.classzone = classzone;
	current->flags |= PF_MEMALLOC | (!order ? PF_FREE_PAGES : 0);

	__freed = try_to_free_pages_zone(classzone, gfp_mask);

no way that __freed is not initialized.

this is the right cleanup for mainline to avoid the harmless compile
time warning. Please test it so Marcelo can apply it. Sorry also for the
delay in the watermark fixes, I finally sorted out a subtle x86-64 bug
yesterday, so I should be able to port the watermark stuff to mainline
early next week.

--- 2.4.23pre4/mm/page_alloc.c.~1~	2003-09-13 00:08:04.000000000 +0200
+++ 2.4.23pre4/mm/page_alloc.c	2003-09-14 01:05:24.000000000 +0200
@@ -258,8 +258,6 @@ static struct page * balance_classzone(z
 	struct page * page = NULL;
 	int __freed;
 
-	if (!(gfp_mask & __GFP_WAIT))
-		goto out;
 	if (in_interrupt())
 		BUG();
 

Thanks,

Andrea

/*
 * If you refuse to depend on closed software for a critical
 * part of your business, these links may be useful:
 *
 * rsync.kernel.org::pub/scm/linux/kernel/bkcvs/linux-2.5/
 * rsync.kernel.org::pub/scm/linux/kernel/bkcvs/linux-2.4/
 * http://www.cobite.com/cvsps/
 *
 * svn://svn.kernel.org/linux-2.6/trunk
 * svn://svn.kernel.org/linux-2.4/trunk
 */