diff mbox

e2fsprogs: m68k bitops fixes

Message ID 20686.16276.932405.296864@pilspetsen.it.uu.se
State Accepted, archived
Headers show

Commit Message

Mikael Pettersson Dec. 16, 2012, 9:39 p.m. UTC 
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 <mikpe@it.uu.se>
Cc: Andreas Schwab <schwab@linux-m68k.org>
---
 lib/ext2fs/bitops.h |   39 +--------------------------------------
 1 files changed, 1 insertion(+), 38 deletions(-)

--
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

Comments

Theodore Ts'o Dec. 17, 2012, 2:49 p.m. UTC | #1 
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
--
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
Andreas Schwab Dec. 17, 2012, 6:15 p.m. UTC | #2 
Theodore Ts'o <tytso@mit.edu> 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.
diff mbox

Patch

--- 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__ */
-
 
 #if !defined(_EXT2_HAVE_ASM_SWAB_)