* [PATCH] mballoc: fix hot spins after err_freebuddy and err_freemeta @ 2008-04-17 16:09 Roel Kluin 2008-04-17 17:58 ` Aneesh Kumar K.V 0 siblings, 1 reply; 6+ messages in thread From: Roel Kluin @ 2008-04-17 16:09 UTC (permalink / raw) To: alex, sct, akpm, adilger; +Cc: linux-ext4, lkml 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. Signed-off-by: Roel Kluin <12o3l@tiscali.nl> --- diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index ef97f19..afba9b8 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -2618,14 +2618,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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mballoc: fix hot spins after err_freebuddy and err_freemeta 2008-04-17 16:09 [PATCH] mballoc: fix hot spins after err_freebuddy and err_freemeta Roel Kluin @ 2008-04-17 17:58 ` Aneesh Kumar K.V 2008-04-17 18:29 ` [PATCH v2] " Roel Kluin 0 siblings, 1 reply; 6+ messages in thread From: Aneesh Kumar K.V @ 2008-04-17 17:58 UTC (permalink / raw) To: Roel Kluin; +Cc: alex, sct, akpm, adilger, linux-ext4, lkml On Thu, Apr 17, 2008 at 06:09:14PM +0200, Roel Kluin wrote: > 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. > > Signed-off-by: Roel Kluin <12o3l@tiscali.nl> > --- > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index ef97f19..afba9b8 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -2618,14 +2618,14 @@ static int ext4_mb_init_backend(struct super_block *sb) > return 0; 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;. > > 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); -aneesh ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] mballoc: fix hot spins after err_freebuddy and err_freemeta 2008-04-17 17:58 ` Aneesh Kumar K.V @ 2008-04-17 18:29 ` Roel Kluin 2008-04-18 5:14 ` Aneesh Kumar K.V 0 siblings, 1 reply; 6+ messages in thread From: Roel Kluin @ 2008-04-17 18:29 UTC (permalink / raw) To: Aneesh Kumar K.V; +Cc: alex, sct, akpm, adilger, linux-ext4, lkml 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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mballoc: fix hot spins after err_freebuddy and err_freemeta 2008-04-17 18:29 ` [PATCH v2] " Roel Kluin @ 2008-04-18 5:14 ` Aneesh Kumar K.V 2008-04-18 5:21 ` Aneesh Kumar K.V 0 siblings, 1 reply; 6+ messages in thread From: Aneesh Kumar K.V @ 2008-04-18 5:14 UTC (permalink / raw) To: Roel Kluin; +Cc: alex, sct, akpm, adilger, linux-ext4, lkml 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 ? diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index 28b5ada..c14f566 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,7 +2329,7 @@ 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) kfree(sbi->s_group_info[i]); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mballoc: fix hot spins after err_freebuddy and err_freemeta 2008-04-18 5:14 ` Aneesh Kumar K.V @ 2008-04-18 5:21 ` Aneesh Kumar K.V 2008-04-18 12:13 ` Roel Kluin 0 siblings, 1 reply; 6+ messages in thread From: Aneesh Kumar K.V @ 2008-04-18 5:21 UTC (permalink / raw) To: Roel Kluin; +Cc: alex, sct, akpm, adilger, linux-ext4, lkml 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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mballoc: fix hot spins after err_freebuddy and err_freemeta 2008-04-18 5:21 ` Aneesh Kumar K.V @ 2008-04-18 12:13 ` Roel Kluin 0 siblings, 0 replies; 6+ messages in thread From: Roel Kluin @ 2008-04-18 12:13 UTC (permalink / raw) To: Aneesh Kumar K.V; +Cc: alex, sct, akpm, adilger, linux-ext4, lkml Aneesh Kumar K.V wrote: > 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;. >> 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. > Won't this work as well? --- In ext4_mb_init_backend() 'i' is of type ext4_group_t. Since unsigned, i >= 0 is always true, so fix hot spins after err_freebuddy: and -meta: and prevent decrements when zero. Signed-off-by: Roel Kluin <12o3l@tiscali.nl> --- diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index ef97f19..054cd33 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -2572,13 +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; } desc = ext4_get_group_desc(sb, i, NULL); if (desc == NULL) { printk(KERN_ERR "EXT4-fs: can't read descriptor %lu\n", i); + i++; goto err_freebuddy; } memset(meta_group_info[j], 0, len); @@ -2618,13 +2618,11 @@ static int ext4_mb_init_backend(struct super_block *sb) return 0; err_freebuddy: - while (i >= 0) { + while (i-- > 0) kfree(ext4_get_group_info(sb, i)); - i--; - } i = num_meta_group_infos; err_freemeta: - while (--i >= 0) + while (i-- > 0) kfree(sbi->s_group_info[i]); iput(sbi->s_buddy_cache); err_freesgi: ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-04-18 12:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-04-17 16:09 [PATCH] mballoc: fix hot spins after err_freebuddy and err_freemeta Roel Kluin 2008-04-17 17:58 ` Aneesh Kumar K.V 2008-04-17 18:29 ` [PATCH v2] " Roel Kluin 2008-04-18 5:14 ` Aneesh Kumar K.V 2008-04-18 5:21 ` Aneesh Kumar K.V 2008-04-18 12:13 ` Roel Kluin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox