lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:	Thu, 21 Apr 2011 13:10:31 +0200 (CEST)
From:	Lukas Czerner <lczerner@...hat.com>
To:	Rogier Wolff <R.E.Wolff@...Wizard.nl>
cc:	Lukas Czerner <lczerner@...hat.com>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: protect bb_first_free in ext4_trim_all_free() with
 group lock

On Thu, 21 Apr 2011, Rogier Wolff wrote:

> 
> Hi Lukas, 

Hi Rogier,

> 
> IMHO, I think it was written intentionally like that. You do as much
> as you can outside "having the lock", especially these "search" things
> are written this way. Doing it this way means that there is less
> lock-contention when things get busy.
> 
> Now I agree that this one statement without any function calls is
> going to be reasonably fast on any modern hardware...
> 
> 	Roger. 
> 
> 
>  	bitmap = e4b.bd_bitmap;
> +
> +	/* we take the lock AFTER this statement because if it
> +	   gets modified under us we'll correct later. */ 

The comment is not right. We do not correct it later in the case that
some space was freed and we still have the old value, hence skipping the
difference between the old and the new bb_first_free. As I mentioned in
the description this is not a big deal, but the fix is simple enough and
there is absolutely no "slowdown" in moving one simple condition into
critical section.

Thanks!
-Lukas

>  	start = (e4b.bd_info->bb_first_free > start) ?
>  		e4b.bd_info->bb_first_free : start;
> 	ext4_lock_group(sb, group);
>  
>  	while (start < max) {
>  		start = mb_find_next_zero_bit(bitmap, max, start);
> 
> 
> On Thu, Apr 21, 2011 at 12:26:36PM +0200, Lukas Czerner wrote:
> > We should protect reading bd_info->bb_first_free with the group lock
> > because otherwise we might miss some free blocks. This is not a big deal
> > at all, but the change to do right thing is really simple, so lets do
> > that.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@...hat.com>
> > ---
> >  fs/ext4/mballoc.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index 776d7a8..804232a 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -4775,11 +4775,11 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
> >  				"information for %u", group);
> >  		return ret;
> >  	}
> > -
> >  	bitmap = e4b.bd_bitmap;
> > +
> > +	ext4_lock_group(sb, group);
> >  	start = (e4b.bd_info->bb_first_free > start) ?
> >  		e4b.bd_info->bb_first_free : start;
> > -	ext4_lock_group(sb, group);
> >  
> >  	while (start < max) {
> >  		start = mb_find_next_zero_bit(bitmap, max, start);
> > -- 
> > 1.7.4.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists