This patch fixes two problems with the m68k-specific bitops
in e2fsprogs.
The first problem is that tst_bitops SEGVs:
LD_LIBRARY_PATH=../../lib DYLD_LIBRARY_PATH=../../lib ./tst_bitops
make[1]: *** [check] Segmentation fault
Post-mortem debugging shows that this occurs in the m68k-specific
ext2fs_set_bit() when called with BIG_TEST_BIT as bit number:
Core was generated by `./tst_bitops'.
Program terminated with signal 11, Segmentation fault.
#0 0x80000820 in ext2fs_set_bit (addr=0xc01ba008, nr=2147483690) at ../../lib/ext2fs/bitops.h:357
357 __asm__ __volatile__ ("bfset %2@{%1:#1}; sne %0"
(gdb) bt
#0 0x80000820 in ext2fs_set_bit (addr=0xc01ba008, nr=2147483690) at ../../lib/ext2fs/bitops.h:357
#1 main (argc=1, argv=0xefc31174) at tst_bitops.c:112
(gdb) up
#1 main (argc=1, argv=0xefc31174) at tst_bitops.c:112
112 ext2fs_set_bit(BIG_TEST_BIT, bigarray);
The SEGV occurs because bfset et al interpret the bit number as a
_signed_ value, so 2147483690 is actually -2147483606, causing an
access to a location about 1/4 GB before 'bigarray'.
To fix this I adjust the base address to point to the correct byte,
and limit the bit number to be within that byte, before performing
the bfset (and likewise in the clear_bit and test_bit functions).
This is similar to what the x86-specific bitops do.
With this in place the SEGV is gone and the tests pass, but there
is still some strange output from tst_bitops:
big bit number (2147483690) test: 0, expected 4
big bit number (2147483690) test: 4, expected 0
ext2fs_set_bit big_test successful
The test and expected values differ, yet the test case doesn't fail.
A second look at the m68k bitop asm() statements makes the reason clear:
there are _no_ dependencies whatsoever between the asm()s and the
memory locations being accessed (a dependency on the address isn't
enough to create a dependency on memory locations reachable from it,
and __volatile__ doesn't create memory dependencies either). The
simplest fix is to add "memory" clobbers (the kernel's bitops also
have memory clobbers), and with those in place the tests still pass
and the debug output looks sane:
big bit number (2147483690) test: 4, expected 4
big bit number (2147483690) test: 0, expected 0
ext2fs_set_bit big_test successful
Tested on m68k-linux with e2fsprogs-1.42.6 and gcc-4.6.3.
Signed-off-by: Mikael Pettersson <[email protected]>
---
The Linux/m68k kernel's bfset et al bitops may also be broken,
but that depends on whether they can ever be applied to large
enough bitmaps to make the bit numbers negative. I'll raise
the issue on the linux-m68k list at vger.
lib/ext2fs/bitops.h | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
--- e2fsprogs-1.42.6/lib/ext2fs/bitops.h.~1~ 2012-09-10 04:53:00.000000000 +0200
+++ e2fsprogs-1.42.6/lib/ext2fs/bitops.h 2012-12-16 13:39:46.000000000 +0100
@@ -354,8 +354,9 @@ _INLINE_ int ext2fs_set_bit(unsigned int
{
char retval;
+ addr = (void *) ((unsigned char *) addr + (nr >> 3));
__asm__ __volatile__ ("bfset %2@{%1:#1}; sne %0"
- : "=d" (retval) : "d" (nr^7), "a" (addr));
+ : "=d" (retval) : "d" ((nr & 7) ^ 7), "a" (addr) : "memory");
return retval;
}
@@ -364,8 +365,9 @@ _INLINE_ int ext2fs_clear_bit(unsigned i
{
char retval;
+ addr = (void *) ((unsigned char *) addr + (nr >> 3));
__asm__ __volatile__ ("bfclr %2@{%1:#1}; sne %0"
- : "=d" (retval) : "d" (nr^7), "a" (addr));
+ : "=d" (retval) : "d" ((nr & 7) ^ 7), "a" (addr) : "memory");
return retval;
}
@@ -374,8 +376,9 @@ _INLINE_ int ext2fs_test_bit(unsigned in
{
char retval;
+ addr = (const void *) ((const unsigned char *) addr + (nr >> 3));
__asm__ __volatile__ ("bftst %2@{%1:#1}; sne %0"
- : "=d" (retval) : "d" (nr^7), "a" (addr));
+ : "=d" (retval) : "d" ((nr & 7) ^ 7), "a" (addr) : "memory");
return retval;
}
Mikael Pettersson writes:
> This patch fixes two problems with the m68k-specific bitops
> in e2fsprogs.
>
> The first problem is that tst_bitops SEGVs:
>
> LD_LIBRARY_PATH=../../lib DYLD_LIBRARY_PATH=../../lib ./tst_bitops
> make[1]: *** [check] Segmentation fault
>
> Post-mortem debugging shows that this occurs in the m68k-specific
> ext2fs_set_bit() when called with BIG_TEST_BIT as bit number:
>
> Core was generated by `./tst_bitops'.
> Program terminated with signal 11, Segmentation fault.
> #0 0x80000820 in ext2fs_set_bit (addr=0xc01ba008, nr=2147483690) at ../../lib/ext2fs/bitops.h:357
> 357 __asm__ __volatile__ ("bfset %2@{%1:#1}; sne %0"
> (gdb) bt
> #0 0x80000820 in ext2fs_set_bit (addr=0xc01ba008, nr=2147483690) at ../../lib/ext2fs/bitops.h:357
> #1 main (argc=1, argv=0xefc31174) at tst_bitops.c:112
> (gdb) up
> #1 main (argc=1, argv=0xefc31174) at tst_bitops.c:112
> 112 ext2fs_set_bit(BIG_TEST_BIT, bigarray);
>
> The SEGV occurs because bfset et al interpret the bit number as a
> _signed_ value, so 2147483690 is actually -2147483606, causing an
> access to a location about 1/4 GB before 'bigarray'.
>
> To fix this I adjust the base address to point to the correct byte,
> and limit the bit number to be within that byte, before performing
> the bfset (and likewise in the clear_bit and test_bit functions).
> This is similar to what the x86-specific bitops do.
>
> With this in place the SEGV is gone and the tests pass, but there
> is still some strange output from tst_bitops:
>
> big bit number (2147483690) test: 0, expected 4
> big bit number (2147483690) test: 4, expected 0
> ext2fs_set_bit big_test successful
>
> The test and expected values differ, yet the test case doesn't fail.
>
> A second look at the m68k bitop asm() statements makes the reason clear:
> there are _no_ dependencies whatsoever between the asm()s and the
> memory locations being accessed (a dependency on the address isn't
> enough to create a dependency on memory locations reachable from it,
> and __volatile__ doesn't create memory dependencies either). The
> simplest fix is to add "memory" clobbers (the kernel's bitops also
> have memory clobbers), and with those in place the tests still pass
> and the debug output looks sane:
>
> big bit number (2147483690) test: 4, expected 4
> big bit number (2147483690) test: 0, expected 0
> ext2fs_set_bit big_test successful
>
> Tested on m68k-linux with e2fsprogs-1.42.6 and gcc-4.6.3.
Andreas Schwab suggested that there was no longer an advantage
to using inline asm for these operations, so this new patch
deletes the m68k-specific code from lib/ext2fs/bitops.h (leaving
only 32-bit x86 to still use inline asm there).
Tested on m68k-linux with e2fsprogs-1.42.6 and gcc-4.6.3 as before.
All tests pass and the debug output looks sane.
I compared the e2fsck binaries from the previous build with this
one. They had identical .text sizes, and almost the same number
of bit field instructions (obviously compiler-generated), so this
change should have no serious performance implications.
Signed-off-by: Mikael Pettersson <[email protected]>
Cc: Andreas Schwab <[email protected]>
---
lib/ext2fs/bitops.h | 39 +--------------------------------------
1 files changed, 1 insertion(+), 38 deletions(-)
--- e2fsprogs-1.42.6/lib/ext2fs/bitops.h.~1~ 2012-09-10 04:53:00.000000000 +0200
+++ e2fsprogs-1.42.6/lib/ext2fs/bitops.h 2012-12-16 17:41:09.000000000 +0100
@@ -200,7 +200,7 @@ extern errcode_t ext2fs_find_first_zero_
*/
#ifdef NO_INLINE_FUNCS
#if (defined(__GNUC__) && (defined(__i386__) || defined(__i486__) || \
- defined(__i586__) || defined(__mc68000__)))
+ defined(__i586__)))
/* This prevents bitops.c from trying to include the C */
/* function version of these functions */
#define _EXT2_HAVE_ASM_BITOPS_
@@ -345,43 +345,6 @@ _INLINE_ __u16 ext2fs_swab16(__u16 val)
#endif /* i386 */
-#if ((defined __GNUC__) && !defined(_EXT2_USE_C_VERSIONS_) && \
- (defined(__mc68000__)))
-
-#define _EXT2_HAVE_ASM_BITOPS_
-
-_INLINE_ int ext2fs_set_bit(unsigned int nr,void * addr)
-{
- char retval;
-
- __asm__ __volatile__ ("bfset %2@{%1:#1}; sne %0"
- : "=d" (retval) : "d" (nr^7), "a" (addr));
-
- return retval;
-}
-
-_INLINE_ int ext2fs_clear_bit(unsigned int nr, void * addr)
-{
- char retval;
-
- __asm__ __volatile__ ("bfclr %2@{%1:#1}; sne %0"
- : "=d" (retval) : "d" (nr^7), "a" (addr));
-
- return retval;
-}
-
-_INLINE_ int ext2fs_test_bit(unsigned int nr, const void * addr)
-{
- char retval;
-
- __asm__ __volatile__ ("bftst %2@{%1:#1}; sne %0"
- : "=d" (retval) : "d" (nr^7), "a" (addr));
-
- return retval;
-}
-
-#endif /* __mc68000__ */
On Sun, Dec 16, 2012 at 10:39:32PM +0100, Mikael Pettersson wrote:
> Andreas Schwab suggested that there was no longer an advantage
> to using inline asm for these operations, so this new patch
> deletes the m68k-specific code from lib/ext2fs/bitops.h (leaving
> only 32-bit x86 to still use inline asm there).
Yep, agreed. Thanks for the patch! I've applied it in the e2fsprogs
maint branch.
It would be a good idea to check the m68k kernel, since it was cribbed
from the kernel sources. I do agree though that it's unlikely to be a
problem in kernel code.
Thanks again!!
- Ted
Theodore Ts'o <[email protected]> writes:
> It would be a good idea to check the m68k kernel, since it was cribbed
> from the kernel sources.
The kernel bitops always use signed bit numbers.
Andreas.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."