| Message ID | 20120406191401.GA8816@sli.dy.fi |
|---|---|
| State | Superseded, archived |
| Headers | show |
-----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
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.
-----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
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
-----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
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
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;