From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mikael Pettersson <mikpe@it.uu.se>
Subject: [PATCH] e2fsprogs: m68k bitops fixes
Date: Sun, 16 Dec 2012 14:40:35 +0100
Message-ID: <20685.53075.403265.997210@pilspetsen.it.uu.se>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Cc: Mikael Pettersson <mikpe@it.uu.se>
To: linux-ext4@vger.kernel.org
Return-path: <linux-ext4-owner@vger.kernel.org>
Received: from smtp1.uu.se ([130.238.7.54]:60834 "EHLO smtp1.uu.se"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752774Ab2LPNrL (ORCPT <rfc822;linux-ext4@vger.kernel.org>);
	Sun, 16 Dec 2012 08:47:11 -0500
Sender: linux-ext4-owner@vger.kernel.org
List-ID: <linux-ext4.vger.kernel.org>

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 <mikpe@it.uu.se>
---
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;
 }

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mikael Pettersson <mikpe@it.uu.se>
Subject: Re: [PATCH] e2fsprogs: m68k bitops fixes
Date: Sun, 16 Dec 2012 22:39:32 +0100
Message-ID: <20686.16276.932405.296864@pilspetsen.it.uu.se>
References: <20685.53075.403265.997210@pilspetsen.it.uu.se>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Cc: linux-ext4@vger.kernel.org, Andreas Schwab <schwab@linux-m68k.org>
To: Mikael Pettersson <mikpe@it.uu.se>
Return-path: <linux-ext4-owner@vger.kernel.org>
Received: from smtp1.uu.se ([130.238.7.54]:38598 "EHLO smtp1.uu.se"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751365Ab2LPVj7 (ORCPT <rfc822;linux-ext4@vger.kernel.org>);
	Sun, 16 Dec 2012 16:39:59 -0500
In-Reply-To: <20685.53075.403265.997210@pilspetsen.it.uu.se>
Sender: linux-ext4-owner@vger.kernel.org
List-ID: <linux-ext4.vger.kernel.org>

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

--- 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__ */

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Theodore Ts'o <tytso@mit.edu>
Subject: Re: [PATCH] e2fsprogs: m68k bitops fixes
Date: Mon, 17 Dec 2012 09:49:02 -0500
Message-ID: <20121217144902.GC31563@thunk.org>
References: <20685.53075.403265.997210@pilspetsen.it.uu.se>
 <20686.16276.932405.296864@pilspetsen.it.uu.se>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: linux-ext4@vger.kernel.org, Andreas Schwab <schwab@linux-m68k.org>
To: Mikael Pettersson <mikpe@it.uu.se>
Return-path: <linux-ext4-owner@vger.kernel.org>
Received: from li9-11.members.linode.com ([67.18.176.11]:38876 "EHLO
	imap.thunk.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752360Ab2LQOtN (ORCPT <rfc822;linux-ext4@vger.kernel.org>);
	Mon, 17 Dec 2012 09:49:13 -0500
Content-Disposition: inline
In-Reply-To: <20686.16276.932405.296864@pilspetsen.it.uu.se>
Sender: linux-ext4-owner@vger.kernel.org
List-ID: <linux-ext4.vger.kernel.org>

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

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andreas Schwab <schwab@linux-m68k.org>
Subject: Re: [PATCH] e2fsprogs: m68k bitops fixes
Date: Mon, 17 Dec 2012 19:15:30 +0100
Message-ID: <m2a9tc7dfh.fsf@igel.home>
References: <20685.53075.403265.997210@pilspetsen.it.uu.se>
	<20686.16276.932405.296864@pilspetsen.it.uu.se>
	<20121217144902.GC31563@thunk.org>
Mime-Version: 1.0
Content-Type: text/plain
Cc: Mikael Pettersson <mikpe@it.uu.se>, linux-ext4@vger.kernel.org
To: Theodore Ts'o <tytso@mit.edu>
Return-path: <linux-ext4-owner@vger.kernel.org>
Received: from mail-out.m-online.net ([212.18.0.9]:50443 "EHLO
	mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753570Ab2LQSPg (ORCPT
	<rfc822;linux-ext4@vger.kernel.org>); Mon, 17 Dec 2012 13:15:36 -0500
In-Reply-To: <20121217144902.GC31563@thunk.org> (Theodore Ts'o's message of
	"Mon, 17 Dec 2012 09:49:02 -0500")
Sender: linux-ext4-owner@vger.kernel.org
List-ID: <linux-ext4.vger.kernel.org>

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

