Patchwork Commit c1a1e7fc24d6 causes segfault in ext2fs_new_inode

login
register
mail settings
Submitter Sami Liedes
Date April 6, 2012, 7:14 p.m.
Message ID <20120406191401.GA8816@sli.dy.fi>
Download mbox | patch
Permalink /patch/151253/
State Superseded
Headers show

Comments

Sami Liedes - April 6, 2012, 7:14 p.m.
On Fri, Mar 30, 2012 at 02:38:42PM -0500, Eric Sandeen wrote:
> On 3/30/12 8:19 AM, Richard W.M. Jones wrote:
> > It seems like a non-64-bit-compatible bitmap was being created, and
> > that doesn't have the bitmap->bitmap_ops field initialized because
> > gen_bitmap.c doesn't use this field.  Somehow, though, we end up
> > calling a function in gen_bitmap64.c which requires that this field be
> > defined.

Argh, indeed. I thought the 32-bit bitfields also have the bitmap_ops
field (and in the same offset), but they don't.

> Well here's what's busted:
> 
>         if (bitmap->bitmap_ops->find_first_zero)
>                 return bitmap->bitmap_ops->find_first_zero(bitmap, start, end, out);
> 
>         if (!bitmap || !EXT2FS_IS_64_BITMAP(bitmap) || bitmap->cluster_bits)
>                 return EINVAL;
> 
> bitmap->bitmap_ops->find_first_zero only exists for a 64-bit bitmap, which
> gets tested after we try to deref it :(

Right, that's quite bogus. The !bitmap test should obviously be first,
not after we first dereference it. Then we should test for 64-bitness,
and only ever touch the bitmap_ops field if we have a 64-bit bitmap.

> I am a little confused by the existence of two different
> struct ext2fs_struct_generic_bitmap's in the code.  But treating one as the
> other looks doomed to failure ;)

In addition to that, there are actually three different versions of
many operations; they are named ext2_foo_bmap(), ext2_foo_bitmap() and
ext2_foo_bitmap2(). I'm quite confused too.

While I suggest passing EXT2_FLAG_64BITS to ext2fs_open() - it should
never break anything and only makes things faster - the code obviously
shouldn't break when that is not passed.

(I wonder if it would make sense to have something like
EXT2_BASE_FLAGS that could include any flags which never hurt,
currently apparently only EXT2_FLAG_64BITS. From the name of the flag
EXT2_FLAG_64BITS it's not obvious that you should always use it. As
far as I understand correctly, it's there only for ABI compatibility
with code compiled before the flag was introduced and it never makes
sense to not pass it in any new code.)

The patch below unbreaks the code (well, at least resize2fs
modified to not pass EXT2_FLAG_64BITS) for me.

	Sami Liedes

------------------------------------------------------------

commit bb8fe012a3b1705809f6129cd9c78d2cd855b1f8
Author: Sami Liedes <sami.liedes@iki.fi>
Date:   Fri Apr 6 22:06:30 2012 +0300

    Fix ext2fs_find_first_zero_generic_bmap() for 32-bit bitmaps.
    
    ext2fs_find_first_zero_generic_bmap() tries to handle old-style 32-bit
    bitmaps, but fails in several different ways.
    
    Fix the order of the (in)validity tests and the fallback code to never
    dereference the bitmap, as we don't know at that point if it's a
    32-bit bitmap or a 64-bitmap bitmap whose backend implementation does
    not support find_first_zero(). Use the generic bitop functions for
    everything instead.
    
    Addresses-Red-Hat-Bugzilla: #808421
    
    Signed-off-by: Sami Liedes <sami.liedes@iki.fi>
    Reported-by: Richard W.M. Jones <rjones@redhat.com>
Eric Sandeen - April 6, 2012, 7:19 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/6/12 12:14 PM, Sami Liedes wrote:
> On Fri, Mar 30, 2012 at 02:38:42PM -0500, Eric Sandeen wrote:
>> On 3/30/12 8:19 AM, Richard W.M. Jones wrote:
>>> It seems like a non-64-bit-compatible bitmap was being created, and
>>> that doesn't have the bitmap->bitmap_ops field initialized because
>>> gen_bitmap.c doesn't use this field.  Somehow, though, we end up
>>> calling a function in gen_bitmap64.c which requires that this field be
>>> defined.
> 
> Argh, indeed. I thought the 32-bit bitfields also have the bitmap_ops
> field (and in the same offset), but they don't.

nope :)

>> Well here's what's busted:
>>
>>         if (bitmap->bitmap_ops->find_first_zero)
>>                 return bitmap->bitmap_ops->find_first_zero(bitmap, start, end, out);
>>
>>         if (!bitmap || !EXT2FS_IS_64_BITMAP(bitmap) || bitmap->cluster_bits)
>>                 return EINVAL;
>>
>> bitmap->bitmap_ops->find_first_zero only exists for a 64-bit bitmap, which
>> gets tested after we try to deref it :(
> 
> Right, that's quite bogus. The !bitmap test should obviously be first,
> not after we first dereference it. Then we should test for 64-bitness,
> and only ever touch the bitmap_ops field if we have a 64-bit bitmap.
> 
>> I am a little confused by the existence of two different
>> struct ext2fs_struct_generic_bitmap's in the code.  But treating one as the
>> other looks doomed to failure ;)
> 
> In addition to that, there are actually three different versions of
> many operations; they are named ext2_foo_bmap(), ext2_foo_bitmap() and
> ext2_foo_bitmap2(). I'm quite confused too.

Yeah, it's rather inscrutable.

> While I suggest passing EXT2_FLAG_64BITS to ext2fs_open() - it should
> never break anything and only makes things faster - the code obviously
> shouldn't break when that is not passed.

Right, that was the whole point of the inscrutable mess above :(

> (I wonder if it would make sense to have something like
> EXT2_BASE_FLAGS that could include any flags which never hurt,
> currently apparently only EXT2_FLAG_64BITS. From the name of the flag
> EXT2_FLAG_64BITS it's not obvious that you should always use it. As
> far as I understand correctly, it's there only for ABI compatibility
> with code compiled before the flag was introduced and it never makes
> sense to not pass it in any new code.)
> 
> The patch below unbreaks the code (well, at least resize2fs
> modified to not pass EXT2_FLAG_64BITS) for me.

Thanks; I am actually just now testing my own patch, I wasn't sure if
you were around and I wanted to get this fixed.  :)

Mine is more invasive, with all of the full-on inscrutable redirection
mess every other bitmap function uses ;)  But maybe that's not necessary
for this new function, since there will be no old users of it.

We probably need a way to test at least every binary in e2fsprogs
with the 64-bit flags off, to be sure that old apps won't break.

For now I just edited every util which set the 64 bit flag and turned
that off, and ran make check.

I may send my patch as well, and see what Ted thinks is the best
fit for the current code layout... yours is simpler, and without
any API concerns/constraints, it may be better.

Thanks,
- -Eric

> 	Sami Liedes
> 
> ------------------------------------------------------------
> 
> commit bb8fe012a3b1705809f6129cd9c78d2cd855b1f8
> Author: Sami Liedes <sami.liedes@iki.fi>
> Date:   Fri Apr 6 22:06:30 2012 +0300
> 
>     Fix ext2fs_find_first_zero_generic_bmap() for 32-bit bitmaps.
>     
>     ext2fs_find_first_zero_generic_bmap() tries to handle old-style 32-bit
>     bitmaps, but fails in several different ways.
>     
>     Fix the order of the (in)validity tests and the fallback code to never
>     dereference the bitmap, as we don't know at that point if it's a
>     32-bit bitmap or a 64-bitmap bitmap whose backend implementation does
>     not support find_first_zero(). Use the generic bitop functions for
>     everything instead.
>     
>     Addresses-Red-Hat-Bugzilla: #808421
>     
>     Signed-off-by: Sami Liedes <sami.liedes@iki.fi>
>     Reported-by: Richard W.M. Jones <rjones@redhat.com>
> 
> diff --git a/lib/ext2fs/gen_bitmap64.c b/lib/ext2fs/gen_bitmap64.c
> index e765d2c..7e9b8a0 100644
> --- a/lib/ext2fs/gen_bitmap64.c
> +++ b/lib/ext2fs/gen_bitmap64.c
> @@ -770,19 +770,25 @@ errcode_t ext2fs_find_first_zero_generic_bmap(ext2fs_generic_bitmap bitmap,
>  {
>  	int b;
>  
> -	if (bitmap->bitmap_ops->find_first_zero)
> +	if (!bitmap)
> +		return EINVAL;
> +
> +	if (EXT2FS_IS_64_BITMAP(bitmap) && bitmap->bitmap_ops->find_first_zero)
>  		return bitmap->bitmap_ops->find_first_zero(bitmap, start, end, out);
>  
> -	if (!bitmap || !EXT2FS_IS_64_BITMAP(bitmap) || bitmap->cluster_bits)
> +	if (!EXT2FS_IS_32_BITMAP(bitmap) || bitmap->cluster_bits)
>  		return EINVAL;
>  
> -	if (start < bitmap->start || end > bitmap->end || start > end) {
> +	// We must be careful to not use any of bitmap's fields here,
> +	// as it may actually be the different 32-bit version of the structure
> +	if (start < ext2fs_get_block_bitmap_start(bitmap) ||
> +	    end > ext2fs_get_block_bitmap_end(bitmap) || start > end) {
>  		warn_bitmap(bitmap, EXT2FS_TEST_ERROR, start);
>  		return EINVAL;
>  	}
>  
>  	while (start <= end) {
> -		b = bitmap->bitmap_ops->test_bmap(bitmap, start);
> +		b = ext2fs_test_generic_bitmap(bitmap, start);
>  		if (!b) {
>  			*out = start;
>  			return 0;

-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPf0HAAAoJECCuFpLhPd7gGyQP/0qpH0dB55Ys5j70OBXIFtzT
rMqYcE7P+HZ66+zkWKqB2lKtnMLFXGxDkeQGQ4ECxrucKpcWWrwLSyvB11Py41rV
wal5Uymg41Q7lRjfnYeraKsObeT9VNlV5d/i1TVRZNG0ooHni/9loEShHnKUnlKs
kOROEFu6x/MHNTGeXtmU3aE2l9SCePJn3rQZHQrTeDo5/hm7lQwPUvTmk5Jg8F/v
uNW9zdnPOOvcSF1nmy8P32GKE750GwcrctVe0S9UUtiGr5XPSW7RdnynELRm/6kh
qpGv4v1dPjmG/uJvltxiKymhSv6zoj9NDqEz5V/iISrBvY108WWBFJVpp/EfgMph
luIPUV1zQ5KwQNyHSHgeGCuwa8R02I2sDwXQ/ZyZomOATK1dJ8s6ODkSFUL1pP3k
xZmnWJM9cw4pagCEagyvD6OtT43vvSVakPKxZGE/U67tVxLNZrzmGm79glDMhs70
xed0T5lYZoJEfoMAfLMW7CQYsceRs7bNLfBos1XL8vMhoq0ak8sXmSIiEzzHPOPK
scM+jKtisDqZE9s3j1LgA+aXhmag4/RwO8zTq0gNmZVD4slqbFXVXuIFjFbFR61i
F6ttZROBvo+//C7E7F1AXytCCvhPVQKgDH1aUq92mUmHsKh3CSEwr4BA+E+7+iOj
2WiGb4k3ZI0AcjcZljOi
=9/4M
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard W.M. Jones - April 6, 2012, 7:22 p.m.
On Fri, Apr 06, 2012 at 10:14:01PM +0300, Sami Liedes wrote:
> While I suggest passing EXT2_FLAG_64BITS to ext2fs_open() - it should
> never break anything and only makes things faster [...]

Thanks -- good to know, and this is what we did to fix febootstrap.

Rich.
Eric Sandeen - April 6, 2012, 7:31 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/6/12 12:19 PM, Eric Sandeen wrote:

...

> Thanks; I am actually just now testing my own patch, I wasn't sure if
> you were around and I wanted to get this fixed.  :)
> 
> Mine is more invasive, with all of the full-on inscrutable redirection
> mess every other bitmap function uses ;)  But maybe that's not necessary
> for this new function, since there will be no old users of it.
> 
> We probably need a way to test at least every binary in e2fsprogs
> with the 64-bit flags off, to be sure that old apps won't break.
> 
> For now I just edited every util which set the 64 bit flag and turned
> that off, and ran make check.
> 
> I may send my patch as well, and see what Ted thinks is the best
> fit for the current code layout... yours is simpler, and without
> any API concerns/constraints, it may be better.

Ok, I also tested your patch through make check
with the 64 bit flags off on all the normal utils, and it passed (for all
but the one test which exclusively requires 64-bit bitmaps).  It seems fine
to me, thanks.

However, there seems to be some unstated (?) naming convention about foo_bmap
(for 64-bit bitmaps) vs. foo_bitmap (for 32-bit bitmaps), and "bitmap2"
functions which seem to take 64-bit args, etc.  I'm not sure if this new
function needs to follow similar conventions...

My patch is probably overkill, so I won't bother to send it unless Ted
thinks something like that is necessary.  Documenting the api preservation
framework for 64-bit bitmaps would be really helpful, I think.

- -Eric
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPf0SMAAoJECCuFpLhPd7ggW4QAJPYaqG9iW6sSg60ySNPlXTW
OM7DF5CAvexUTXFChNyh8C40UpEEo8ms//Etc+G46vSoOjlg3ZvBbmP0fOk8hBhE
ejsNSATpBSwvrgzV8gBi5ZNK6WmefC8PxKXiw17p0gn5oYjKTfBmeFmEjx0oHEGV
zN/BYGDBnzE4n7BGfAACOOZy1HZsWMd2Ud57OpoCsxoB73JrHcz6MEkeGBFqrBBu
WIIPPt4jr6aaEbg2ofQkfJoP3FTHY/tVu8ozAakVWA9AQ/RC3S++9gH44ir4gfYS
PWHUJkNtHn9izcvIlUgly4Vn5s+/Fghyz09qc4LuEzZHwB2NpWHmDsKIrTsIrfWL
f0RI/8uhXdh6T8hhxcYOc0O0vz8iW3k286lr/FzHaX4kXj4yMw4Kf4sMds+v67Zf
aMaQLR+l/UBQeq/vdsrLJcVqMFXovGUH6zmzsplDv98Kl3Lt0gmQHpROqOZPENtb
wvR3ZU4eexcGQxSF0HZtbdhY/PemsYkdSceven5rGr9MXWJUkbhX0wSuXE8jnb7h
wRs/jAvLlAgaPgygAv7eYdccXl069vlFaFHDnoEePlK4lsPC8VpdisxECRjguyz1
nzM6fM9ci3KiGoFBvpwToRL4jos+fUlBnsLQ2v2x1kOGdq8jiJ9C4P3vuz6iE6ZY
WD5TAC8TAzy4+MSx7Gu+
=HKbk
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sami Liedes - April 6, 2012, 7:47 p.m.
On Fri, Apr 06, 2012 at 12:31:25PM -0700, Eric Sandeen wrote:
> However, there seems to be some unstated (?) naming convention about foo_bmap
> (for 64-bit bitmaps) vs. foo_bitmap (for 32-bit bitmaps), and "bitmap2"
> functions which seem to take 64-bit args, etc.  I'm not sure if this new
> function needs to follow similar conventions...

I guess... I thought I had figured those out when I sent the patch,
but I'm not so sure anymore :-)

> My patch is probably overkill, so I won't bother to send it unless Ted
> thinks something like that is necessary.  Documenting the api preservation
> framework for 64-bit bitmaps would be really helpful, I think.

Yeah, I'm sorry it took me a while to look at this and for causing you
extra work.

	Sami
Eric Sandeen - April 6, 2012, 7:49 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/6/12 12:47 PM, Sami Liedes wrote:
> On Fri, Apr 06, 2012 at 12:31:25PM -0700, Eric Sandeen wrote:
>> However, there seems to be some unstated (?) naming convention about foo_bmap
>> (for 64-bit bitmaps) vs. foo_bitmap (for 32-bit bitmaps), and "bitmap2"
>> functions which seem to take 64-bit args, etc.  I'm not sure if this new
>> function needs to follow similar conventions...
> 
> I guess... I thought I had figured those out when I sent the patch,
> but I'm not so sure anymore :-)
> 
>> My patch is probably overkill, so I won't bother to send it unless Ted
>> thinks something like that is necessary.  Documenting the api preservation
>> framework for 64-bit bitmaps would be really helpful, I think.
> 
> Yeah, I'm sorry it took me a while to look at this and for causing you
> extra work.

No worries, I could have waited, too ;)  Thanks!

- -Eric
 
> 	Sami

-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPf0jAAAoJECCuFpLhPd7g2V8P/Rdmakt3hb1mh0C7eu01Cpxw
X16f5Axzhsn0c0bfZsKeEZLB6bM+5TYdlARXATxfnVEZ/upbyCsF6bqZ0I5JxHj5
wTRuB7pwl8E+7vBANOu7aI7YT1Vb0eM2i3zIRONeqkYYYbRGllKeUPCdibnL2T5+
ybiAp2tXLR665LYtt0xcHVQ63L0NfYiditmpoMpxWKNfbu4OGhnpTw6bouzzdg1f
KeXQkMdd26oIl8myrp/zkuHCUtja9aSmv4KrcMcxOuFyCqCuyrQpwDx78fmKpnn8
njy/dqI2QY9c/FmScYcXyAQ6/SJ2gkzjm60dkLT8DvPYLky0ytdQQ3JeFh/kBZX9
iGnRSj+fPjujVtzG73x5d5Q8ayzr6jGR94Vm/krvBJ4Yz5GBJkcuR16/8Lmze8au
oKbaKx4PrvuRqD4PEKzmfkUpVFK0GDbXkKgG0wycU/qaFa8hrTrjIo3web3wNbUc
lZExrKB4Pazu65xrQHQih1uENzlJlC1vkhi99oqnrRDmc4/bvuprp//X9SxboGV+
nglwMEEFL7djPM4GSUMWEFz3qTDLZIXqjPV9XfJJvk6G62BbOHn2Q5YfIsyjNQOx
Wr26EkiGui5OXRAAJ9EYdqtSjip1CPyM05CibOQJeZLDZREmchgJKE0PMcLdWziV
y0l4cQNDitg/8ruuBMKk
=jMuN
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - April 6, 2012, 8:06 p.m.
I was flying back from San Francisco so I decided to take a look at
this while I was flying back.  So my patches overlapped with yours....

I think mine will be a tad bit faster for the fallback path since we
don't end up doing the 32-bit/64-bit each time we test a bit (since I
created a 32-bit and 64-bit ffz function). 

On Fri, Apr 06, 2012 at 10:14:01PM +0300, Sami Liedes wrote:
> 
> Argh, indeed. I thought the 32-bit bitfields also have the bitmap_ops
> field (and in the same offset), but they don't.

Yeah, the only thing which is the same is the offset.  The structure
definition is in gen_bitmap.c and gen_bitmap64.c, and they are
different depending on whether it is a 32-bit or 64-bit bitmap.

> > I am a little confused by the existence of two different
> > struct ext2fs_struct_generic_bitmap's in the code.  But treating one as the
> > other looks doomed to failure ;)
> 
> In addition to that, there are actually three different versions of
> many operations; they are named ext2_foo_bmap(), ext2_foo_bitmap() and
> ext2_foo_bitmap2(). I'm quite confused too.

Well, actually there are four.  The original 32-bit code has generic
bitmap code that is where most of the implementation exists.  Then we
have separate versions for block and inode bitmaps for the purposes of
type checking.

Then in the 64-bit code, we have the same thing; generic versions plus
block/inode specific versions, plus of course the multiple different
back-end implementations.

I've added test cases to exercise all of this code, which should
prevent problems in the future.  I expect that after we add a
find_first_set function, it's unlikely we'll need to be touching the
bitmap implementations again for the near future....

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

Patch

diff --git a/lib/ext2fs/gen_bitmap64.c b/lib/ext2fs/gen_bitmap64.c
index e765d2c..7e9b8a0 100644
--- a/lib/ext2fs/gen_bitmap64.c
+++ b/lib/ext2fs/gen_bitmap64.c
@@ -770,19 +770,25 @@  errcode_t ext2fs_find_first_zero_generic_bmap(ext2fs_generic_bitmap bitmap,
 {
 	int b;
 
-	if (bitmap->bitmap_ops->find_first_zero)
+	if (!bitmap)
+		return EINVAL;
+
+	if (EXT2FS_IS_64_BITMAP(bitmap) && bitmap->bitmap_ops->find_first_zero)
 		return bitmap->bitmap_ops->find_first_zero(bitmap, start, end, out);
 
-	if (!bitmap || !EXT2FS_IS_64_BITMAP(bitmap) || bitmap->cluster_bits)
+	if (!EXT2FS_IS_32_BITMAP(bitmap) || bitmap->cluster_bits)
 		return EINVAL;
 
-	if (start < bitmap->start || end > bitmap->end || start > end) {
+	// We must be careful to not use any of bitmap's fields here,
+	// as it may actually be the different 32-bit version of the structure
+	if (start < ext2fs_get_block_bitmap_start(bitmap) ||
+	    end > ext2fs_get_block_bitmap_end(bitmap) || start > end) {
 		warn_bitmap(bitmap, EXT2FS_TEST_ERROR, start);
 		return EINVAL;
 	}
 
 	while (start <= end) {
-		b = bitmap->bitmap_ops->test_bmap(bitmap, start);
+		b = ext2fs_test_generic_bitmap(bitmap, start);
 		if (!b) {
 			*out = start;
 			return 0;