linux-ext4 - Re: [PATCH 1/1] fs: Small refactoring of the code in ext4 2.6.37-rc1

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 4 Nov 2010 23:31:58 +0100 (CET)
From:	Jesper Juhl <jj@...osbits.net>
To:	André Luis Pereira dos Santos - BSRSoft 
	<andre@...soft.com.br>
cc:	Andreas Dilger <adilger.kernel@...ger.ca>,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] fs: Small refactoring of the code in ext4
 2.6.37-rc1

On Thu, 4 Nov 2010, André Luis Pereira dos Santos - BSRSoft wrote:

> From: Andre Luis Pereira dos Santos <andre@...soft.com.br>
> 
> Hi.
> Small refactoring of the code in order to make minor enhancements to critical areas.
> The notation x + 1 has been replaced by more efficient notation x + +.
> 
> Signed-off-by: Andre Luis Pereira dos Santos <andre@...soft.com.br>
> ---
> Signed-off-by: Andre Luis Pereira dos Santos <andre@...soft.com.br>
> --- linux-2.6.37-rc1/fs/ext4/extents.c	2010-11-01 09:54:12.000000000 -0200
> +++ linux-2.6.37-rc1-patched/fs/ext4/extents.c	2010-11-04 19:54:26.000000000 -0200
> @@ -555,9 +555,9 @@ ext4_ext_binsearch(struct inode *inode,
>  	while (l <= r) {
>  		m = l + (r - l) / 2;
>  		if (block < le32_to_cpu(m->ee_block))
> -			r = m - 1;
> +			r = m--;
>  		else
> -			l = m + 1;
> +			l = m++;

These do not give identical results.

foo = bar + 1;  assigns (bar + 1) to foo.
foo = bar--;  assigns bar to foo then decrements bar.
foo = --bar;  decrements bar then assigns bar to foo.

So your change both change the value that will be assigned to 'r' and 'l' 
and also modify 'm' which was not previously modified.


>  		ext_debug("%p(%u):%p(%u):%p(%u) ", l, le32_to_cpu(l->ee_block),
>  				m, le32_to_cpu(m->ee_block),
>  				r, le32_to_cpu(r->ee_block));
> @@ -1557,7 +1557,7 @@ static int ext4_ext_try_to_merge(struct
>  		if (ext4_ext_is_uninitialized(ex))
>  			uninitialized = 1;
>  		ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex)
> -				+ ext4_ext_get_actual_len(ex + 1));
> +				+ ext4_ext_get_actual_len(ex++));

After your change gcc complains:

 fs/ext4/extents.c:1559:16: warning: operation on ‘ex’ may be undefined
 fs/ext4/extents.c:1559:16: warning: operation on ‘ex’ may be undefined

which it is correct in doing since you are now modifying the value of the 
pointer which is dereferenced in the assignment. Previously the value of 
(ex+1) was simply passed to ext4_ext_get_actual_len(), but now you are 
passing the value of (ex) to ext4_ext_get_actual_len() and then 
subsequently incrementing 'ex' itself.


>  		if (uninitialized)
>  			ext4_ext_mark_uninitialized(ex);
> 
> 


Was this patch even compile tested?

-- 
Jesper Juhl <jj@...osbits.net>             http://www.chaosbits.net/
Plain text mails only, please      http://www.expita.com/nomime.html
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html