From mboxrd@z Thu Jan  1 00:00:00 1970
From: Matthew Wilcox <matthew@wil.cx>
Subject: Re: [KJ] [Patch] fs/ kzalloc conversions
Date: Fri, 24 Feb 2006 08:26:28 -0700
Message-ID: <20060224152628.GO28587@parisc-linux.org>
References: <1140772454.22453.1.camel@alice> <20060224111755.GA7801@mipter.zuzino.mipt.ru>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Eric Sesterhenn <snakebyte@gmx.de>, kernel-janitors@lists.osdl.org,
	linux-fsdevel@vger.kernel.org
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from palinux.external.hp.com ([192.25.206.14]:28309 "EHLO
	palinux.hppa") by vger.kernel.org with ESMTP id S932263AbWBXP0a
	(ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Fri, 24 Feb 2006 10:26:30 -0500
To: Alexey Dobriyan <adobriyan@gmail.com>
Content-Disposition: inline
In-Reply-To: <20060224111755.GA7801@mipter.zuzino.mipt.ru>
Sender: linux-fsdevel-owner@vger.kernel.org
List-Id: linux-fsdevel.vger.kernel.org


[this mail serves two purposes.  One is to educate about the limits of
what a compiler can do, and the other is to alert to a memory-corruption
bug in AIO.  If you're only interested in the latter, please skip to
the bottom.]

On Fri, Feb 24, 2006 at 02:17:55PM +0300, Alexey Dobriyan wrote:
> > @@ -122,10 +122,9 @@ static int aio_setup_ring(struct kioctx
> >  	if (nr_pages > AIO_RING_PAGES) {
> > -		info->ring_pages = kmalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
> > +		info->ring_pages = kzalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
> >  		if (!info->ring_pages)
> >  			return -ENOMEM;
> > -		memset(info->ring_pages, 0, sizeof(struct page *) * nr_pages);
> >  	}
> 
> I thought, kcalloc should be used here, but after looking at size(1)
> outputs kzalloc wins. Which is slightly suspicious.

Look at the definition of kcalloc:

static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
{
        if (n != 0 && size > INT_MAX / n)
                return NULL;
        return kzalloc(n * size, flags);
}

so you'd be calling kcalloc(nr_pages, sizeof(struct page *), GFP_KERNEL)
which would expand to:

	if (nr_pages > AIO_RING_PAGES) {
		if (nr_pages != 0 && sizeof(struct page *) > INT_MAX / nr_pages)
			info->ring_pages = NULL;
		else
			info->ring_pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);

		if (!info->ring_pages)
			return -ENOMEM;
	}

Now, GCC is pretty clever at optimising.  So it probably turns that into:

	if (nr_pages > AIO_RING_PAGES) {
		if (sizeof(struct page *) > INT_MAX / nr_pages)
			return -ENOMEM;
		info->ring_pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
		if (!info->ring_pages)
			return -ENOMEM;
	}

But it can't tell what nr_pages is limited to back in ioctx_alloc().
So it can't optimise away the first test entirely.

[aio-interested readers should pick up again here]

Even if it traces back what nr_pages is limited to (moderately
complicated), it's limited to the global variable aio_max_nr.  Even if
gcc were engaged in whole-program-analysis, it would probably give up
on seeing its address taken in kernel/sysctl.c.  And, at least to this
human's eyes, aio_max_nr actually appears to have *no* limit.

So the test isn't useless and we should use kcalloc here, otherwise an
unthinking sysadmin can increment the aio_max_nr sysctl value to, let's
say, 0x7fffffff.  On a 32-bit machine, the multiplication will wrap,
maybe turn into a small positive number, and we'll gleefully walk off
the end of the array, corrupting data as we go.

And we should set the .extra1 and .extra2 values in the FS_AIO_MAX_NR
clause of kernel/sysctl.c anyway.  Does anyone have thoughts on what the 
*useful* range of this variable is?