From: "Aneesh Kumar K.V" Subject: Re: [PATCH v2] mballoc: fix hot spins after err_freebuddy and err_freemeta Date: Fri, 18 Apr 2008 10:51:03 +0530 Message-ID: <20080418052103.GB10568@skywalker> References: <4807762A.9090202@tiscali.nl> <20080417175843.GA7517@skywalker> <480796FE.2010707@tiscali.nl> <20080418051414.GA10568@skywalker> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: alex@clusterfs.com, sct@redhat.com, akpm@linux-foundation.org, adilger@clusterfs.com, linux-ext4@vger.kernel.org, lkml To: Roel Kluin <12o3l@tiscali.nl> Return-path: Content-Disposition: inline In-Reply-To: <20080418051414.GA10568@skywalker> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-ext4.vger.kernel.org On Fri, Apr 18, 2008 at 10:44:14AM +0530, Aneesh Kumar K.V wrote: > On Thu, Apr 17, 2008 at 08:29:18PM +0200, Roel Kluin wrote: > > Aneesh Kumar K.V wrote: > > > > > The function needs more changes. For ex: > > > > > > 2279 if (meta_group_info[j] == NULL) { > > > 2280 printk(KERN_ERR "EXT4-fs: can't allocate buddy mem\n"); > > > 2281 i--; > > > 2282 goto err_freebuddy; > > > 2283 } > > > > > > That decrement i--; could result in bad value if i == 0;. > > > > Thanks Aneesh, > > --- > > Signed-off-by: Roel Kluin <12o3l@tiscali.nl> > > ext4_mb_init_backend() has a variable i of type ext4_group_t. which is typedefined > > in include/linux/ext4_fs_i.h:34 to unsigned long. Since unsigned, i >= 0 is always > > true, so fix hot spins after err_freebuddy and err_freemeta. > > Also when meta_group_info cannot be allocated prevent a decrement of i when zero. > > > > Signed-off-by: Roel Kluin <12o3l@tiscali.nl> > > --- > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > > index ef97f19..2c13dca 100644 > > --- a/fs/ext4/mballoc.c > > +++ b/fs/ext4/mballoc.c > > @@ -2572,8 +2572,13 @@ static int ext4_mb_init_backend(struct super_block *sb) > > meta_group_info[j] = kzalloc(len, GFP_KERNEL); > > if (meta_group_info[j] == NULL) { > > printk(KERN_ERR "EXT4-fs: can't allocate buddy mem\n"); > > - i--; > > - goto err_freebuddy; > > + if (i != 0) { > > + i--; > > + goto err_freebuddy; > > + } else { > > + i = num_meta_group_infos - 1; > > + goto err_freemeta; > > + } > > } > > desc = ext4_get_group_desc(sb, i, NULL); > > if (desc == NULL) { > > @@ -2618,14 +2623,14 @@ static int ext4_mb_init_backend(struct super_block *sb) > > return 0; > > > > err_freebuddy: > > - while (i >= 0) { > > + do { > > kfree(ext4_get_group_info(sb, i)); > > - i--; > > - } > > - i = num_meta_group_infos; > > + } while (i-- != 0); > > + i = num_meta_group_infos - 1; > > err_freemeta: > > - while (--i >= 0) > > + do { > > kfree(sbi->s_group_info[i]); > > + } while (i-- != 0); > > iput(sbi->s_buddy_cache); > > err_freesgi: > > kfree(sbi->s_group_info); > > > > Won't this also have a memory corruption ? Let's say we fail in the first > loop itslef. That's with i = 0, and since we are using kmalloc. > we may find sbi->s_group_info[0] having some random values. So the > kfree can crash. Why not a simple change like below ? > Updated one. diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index 28b5ada..572d809 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -2223,7 +2223,7 @@ static noinline void ext4_mb_store_history(struct ext4_allocation_context *ac) static int ext4_mb_init_backend(struct super_block *sb) { - ext4_group_t i; + long i; /* should be able to store group number */ int j, len, metalen; struct ext4_sb_info *sbi = EXT4_SB(sb); int num_meta_group_infos = @@ -2257,6 +2257,7 @@ static int ext4_mb_init_backend(struct super_block *sb) if (meta_group_info == NULL) { printk(KERN_ERR "EXT4-fs: can't allocate mem for a " "buddy group\n"); + i--; goto err_freemeta; } sbi->s_group_info[i] = meta_group_info; @@ -2328,10 +2329,12 @@ err_freebuddy: kfree(ext4_get_group_info(sb, i)); i--; } - i = num_meta_group_infos; + i = num_meta_group_infos - 1; err_freemeta: - while (--i >= 0) + while (i >= 0) { kfree(sbi->s_group_info[i]); + i--; + } iput(sbi->s_buddy_cache); err_freesgi: kfree(sbi->s_group_info);