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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130311060331.GB3867@gmail.com>
Date:	Mon, 11 Mar 2013 14:03:31 +0800
From:	Zheng Liu <gnehzuil.liu@...il.com>
To:	Theodore Ts'o <tytso@....edu>
Cc:	linux-ext4@...r.kernel.org, Zheng Liu <wenqing.lz@...bao.com>,
	Dmitry Monakhov <dmonakhov@...nvz.org>
Subject: Re: [PATCH v2 1/5] ext4: improve ext4_es_can_be_merged() to avoid a
 potential overflow

On Sun, Mar 10, 2013 at 08:43:58PM -0400, Theodore Ts'o wrote:
> On Wed, Mar 06, 2013 at 10:17:11PM +0800, Zheng Liu wrote:
> > +	if (ext4_es_status(es1) ^ ext4_es_status(es2))
> >  		return 0;
> >  
> > -	if (ext4_es_status(es1) != ext4_es_status(es2))
> 
> Did you have a reason why changed != to ^?  

Honestly, no.  Just because subconsciously I think bit operation is
faster than other operations, and in ext4_can_extents_be_merged() it
also use '^'.  So I guess there is an optimization.

> 
> It's identical from a functional perspective, but it's less obvious to
> future readers of the code what's going on.  I tried checking to see
> if GCC did any better optimizing the code, but it doesn't seem to make
> any difference.  I'm going to switch it back to !=....

Obviously I'm wrong.  Thanks for checking it.

> 
> > +	/* we need to check delayed extent is without unwritten status */
> > +	if (ext4_es_is_delayed(es1) && !ext4_es_is_unwritten(es1))
> > +		return 1;
> 
> I'm not sure why we need to check the unwritten status?  Under what
> circumstances would we have an extent marked as under delayed
> allocation but also unwritten?

We could do some buffered writes into a hole.  So the extent will be
with delayed status.  Before these extents are written out, user might
uses fallocate(2) to preallocate some blocks at the same offset.  Then
these extents are marked as unwritten status.  But we still need to keep
delayed status because later these extents will be written out and we
will update reserved space according to these extents, especially in a
bigalloc file system.

> 
> 							- Ted
> 
> This is how I've restructured this function for now mainly to make it
> easier to understand;
> 
> static int ext4_es_can_be_merged(struct extent_status *es1,
> 				 struct extent_status *es2)
> {
> 	if (ext4_es_status(es1) != ext4_es_status(es2))
> 		return 0;
> 
> 	if (((__u64) es1->es_len) + es2->es_len > 0xFFFFFFFFULL)
> 		return 0;
> 
> 	if (((__u64) es1->es_lblk) + es1->es_len != es2->es_lblk)
> 		return 0;
> 
> 	if ((ext4_es_is_written(es1) || ext4_es_is_unwritten(es1)) &&
> 	    (ext4_es_pblock(es1) + es1->es_len == ext4_es_pblock(es2)))
> 		return 1;
> 
> 	if (ext4_es_is_hole(es1))
> 		return 1;
> 
> 	/* we need to check delayed extent is without unwritten status */
> 	if (ext4_es_is_delayed(es1) && !ext4_es_is_unwritten(es1))
> 		return 1;
> 
> 	return 0;
> }

It looks good to me.

Thanks,
                                                - Zheng
--
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