linux-ext4 - Re: [PATCH] ext4: protect bb_first_free in ext4_trim_all

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 linux-cve-announce 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]

Message-ID: <alpine.LFD.2.00.1104211304480.14126@dhcp-27-109.brq.redhat.com>
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