[PATCH] Endianness bugs in e2fsck

[PATCH] Endianness bugs in e2fsck

[Kalpak Shah]

In ext2fs_swap_inode_full() if to and from inodes are not the same (which is the case when called from e2fsck_get_next_inode_full), then e2fsck cannot recognize any in-inode EAs since the un-swabbed i_extra_isize was being used. So corrected that to use swabbed values all the time.

Also in ext2fs_read_inode_full(), ext2fs_swap_inode_full() should be called with bufsize instead of with length argument. length was coming out to be 128 even with 512 byte inodes thus leaving the rest of the inode unswabbed.

On big-endian machines, ext2fs_get_next_inode_full() calls this for copying the inode:
ext2fs_swap_inode_full(scan->fs,
		       (struct ext2_inode_large *) inode,
		       (struct ext2_inode_large *) scan->ptr,
			0, bufsize);
In ext2fs_swap_inode_full() only the first (GOOD_OLD_INODE_SIZE + i_extra_isize)bytes are copied into inode. The rest of the inode is not zeroed. So memset the inode to zero if swapfs is enabled. On little endian machines, memcpy(inode, scan->ptr, bufsize); is executed thereby hiding this error.

Signed-off-by: Kalpak Shah <kalpak@clusterfs.com>

Index: e2fsprogs-1.39/lib/ext2fs/swapfs.c
===================================================================
--- e2fsprogs-1.39.orig/lib/ext2fs/swapfs.c     2007-06-19 22:31:20.000000000 -0700
+++ e2fsprogs-1.39/lib/ext2fs/swapfs.c  2007-06-19 22:41:43.628732192 -0700
@@ -261,13 +261,13 @@ void ext2fs_swap_inode_full(ext2_filsys
                return; /* no space for EA magic */

        eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
-                                       f->i_extra_isize);
+                                       t->i_extra_isize);

        if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
                return; /* it seems no magic here */

        eat = (__u32 *) (((char *) t) + sizeof(struct ext2_inode) +
-                                       f->i_extra_isize);
+                                       t->i_extra_isize);
        *eat = ext2fs_swab32(*eaf);

        /* convert EA(s) */
Index: e2fsprogs-1.39/lib/ext2fs/inode.c
===================================================================
--- e2fsprogs-1.39.orig/lib/ext2fs/inode.c      2007-06-19 22:31:21.000000000 -0700
+++ e2fsprogs-1.39/lib/ext2fs/inode.c   2007-06-20 01:06:18.017788976 -0700
@@ -471,6 +471,7 @@ errcode_t ext2fs_get_next_inode_full(ext
                scan->bytes_left -= scan->inode_size - extra_bytes;

 #ifdef EXT2FS_ENABLE_SWAPFS
+               memset(inode, 0, bufsize);
                if ((scan->fs->flags & EXT2_FLAG_SWAP_BYTES) ||
                    (scan->fs->flags & EXT2_FLAG_SWAP_BYTES_READ))
                        ext2fs_swap_inode_full(scan->fs,
@@ -485,6 +486,7 @@ errcode_t ext2fs_get_next_inode_full(ext
                scan->scan_flags &= ~EXT2_SF_BAD_EXTRA_BYTES;
        } else {
 #ifdef EXT2FS_ENABLE_SWAPFS
+               memset(inode, 0, bufsize);
                if ((scan->fs->flags & EXT2_FLAG_SWAP_BYTES) ||
                    (scan->fs->flags & EXT2_FLAG_SWAP_BYTES_READ))
                        ext2fs_swap_inode_full(scan->fs,
@@ -603,7 +605,7 @@ errcode_t ext2fs_read_inode_full(ext2_fi
            (fs->flags & EXT2_FLAG_SWAP_BYTES_READ))
                ext2fs_swap_inode_full(fs, (struct ext2_inode_large *) inode,
                                       (struct ext2_inode_large *) inode,
-                                      0, length);
+                                      0, bufsize);
 #endif

        /* Update the inode cache */

Re: [PATCH] Endianness bugs in e2fsck

[Theodore Tso]

On Wed, Jun 20, 2007 at 03:03:08PM +0530, Kalpak Shah wrote:
> In ext2fs_swap_inode_full() if to and from inodes are not the same
> (which is the case when called from e2fsck_get_next_inode_full),
> then e2fsck cannot recognize any in-inode EAs since the un-swabbed
> i_extra_isize was being used. So corrected that to use swabbed
> values all the time.

> Also in ext2fs_read_inode_full(), ext2fs_swap_inode_full() should be
> called with bufsize instead of with length argument. length was
> coming out to be 128 even with 512 byte inodes thus leaving the rest
> of the inode unswabbed.

Hi Kalpak, 

	In the future it would be really helpful if you split up your
patches so that each different thing is done in separate patches.

	Also, note there is a recent bug fix in this area, and the
byte-swapping extended attributes.  The issues involved here are
subtle; please see the discussion here:

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=232663

So before your patches go in, we need to do a careful audit to make
sure they don't interact properly with this patch which is already in
e2fsprogs mainline.

						- Ted

# HG changeset patch
# User tytso@mit.edu
# Date 1176573631 14400
# Node ID aa8d65921c8922dfed73dd05027a097cc5946653
# Parent  4b2e34b5f7506f9f74b3fadf79280316d57e47d5
Correct byteswapping for fast symlinks with xattrs

Fix a problem byte-swapping fast symlinks inodes that contain extended
attributes.

Addresses Red Hat Bugzilla: #232663
Addresses LTC Bugzilla: #27634

Signed-off-by: "Bryn M. Reeves" <breeves@redhat.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff -r 4b2e34b5f750 -r aa8d65921c89 e2fsck/ChangeLog
--- a/e2fsck/ChangeLog	Sat Apr 14 12:01:39 2007 -0400
+++ b/e2fsck/ChangeLog	Sat Apr 14 14:00:31 2007 -0400
@@ -1,4 +1,10 @@ 2007-04-14  Theodore Tso  <tytso@mit.edu
 2007-04-14  Theodore Tso  <tytso@mit.edu>
+
+	* pass2.c (e2fsck_process_bad_inode): Remove special kludge that
+		dealt with long symlinks on big endian systems.  It turns
+		out this was a workaround to a bug described in Red Hat
+		Bugzilla #232663, with an odd twist.  See comment #12 for
+		more details.
 
 	* pass1.c, pass2.c, util.c: Add better ehandler_operation()
 		markers so it is clearer what e2fsck was doing when an I/O
diff -r 4b2e34b5f750 -r aa8d65921c89 e2fsck/pass2.c
--- a/e2fsck/pass2.c	Sat Apr 14 12:01:39 2007 -0400
+++ b/e2fsck/pass2.c	Sat Apr 14 14:00:31 2007 -0400
@@ -1202,22 +1202,6 @@ extern int e2fsck_process_bad_inode(e2fs
 	    !(fs->super->s_feature_compat & EXT2_FEATURE_COMPAT_EXT_ATTR)) {
 		if (fix_problem(ctx, PR_2_FILE_ACL_ZERO, &pctx)) {
 			inode.i_file_acl = 0;
-#ifdef EXT2FS_ENABLE_SWAPFS
-			/* 
-			 * This is a special kludge to deal with long
-			 * symlinks on big endian systems.  i_blocks
-			 * had already been decremented earlier in
-			 * pass 1, but since i_file_acl hadn't yet
-			 * been cleared, ext2fs_read_inode() assumed
-			 * that the file was short symlink and would
-			 * not have byte swapped i_block[0].  Hence,
-			 * we have to byte-swap it here.
-			 */
-			if (LINUX_S_ISLNK(inode.i_mode) &&
-			    (fs->flags & EXT2_FLAG_SWAP_BYTES) &&
-			    (inode.i_blocks == fs->blocksize >> 9))
-				inode.i_block[0] = ext2fs_swab32(inode.i_block[0]);
-#endif
 			inode_modified++;
 		} else
 			not_fixed++;
diff -r 4b2e34b5f750 -r aa8d65921c89 lib/ext2fs/ChangeLog
--- a/lib/ext2fs/ChangeLog	Sat Apr 14 12:01:39 2007 -0400
+++ b/lib/ext2fs/ChangeLog	Sat Apr 14 14:00:31 2007 -0400
@@ -1,3 +1,9 @@ 2007-04-06  Theodore Tso  <tytso@mit.edu
+2007-04-14  Theodore Tso  <tytso@mit.edu>
+
+	* swapfs.c (ext2fs_swap_inode_full): Fix a problem byte-swapping 
+		fast symlinks inodes that contain extended attributes.
+		(Addresses Red Hat Bugzilla #232663, LTC bugzilla #27634)
+
 2007-04-06  Theodore Tso  <tytso@mit.edu>
 
 	* icount.c (ext2fs_create_icount_tdb): Add support for using TDB
diff -r 4b2e34b5f750 -r aa8d65921c89 lib/ext2fs/swapfs.c
--- a/lib/ext2fs/swapfs.c	Sat Apr 14 12:01:39 2007 -0400
+++ b/lib/ext2fs/swapfs.c	Sat Apr 14 14:00:31 2007 -0400
@@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys 
 			    struct ext2_inode_large *f, int hostorder,
 			    int bufsize)
 {
-	unsigned i;
+	unsigned i, has_data_blocks;
 	int islnk = 0;
 	__u32 *eaf, *eat;
 
@@ -150,11 +150,17 @@ void ext2fs_swap_inode_full(ext2_filsys 
 	t->i_dtime = ext2fs_swab32(f->i_dtime);
 	t->i_gid = ext2fs_swab16(f->i_gid);
 	t->i_links_count = ext2fs_swab16(f->i_links_count);
+	if (hostorder)
+		has_data_blocks = ext2fs_inode_data_blocks(fs, 
+					   (struct ext2_inode *) f);
 	t->i_blocks = ext2fs_swab32(f->i_blocks);
+	if (!hostorder)
+		has_data_blocks = ext2fs_inode_data_blocks(fs, 
+					   (struct ext2_inode *) t);
 	t->i_flags = ext2fs_swab32(f->i_flags);
 	t->i_file_acl = ext2fs_swab32(f->i_file_acl);
 	t->i_dir_acl = ext2fs_swab32(f->i_dir_acl);
-	if (!islnk || ext2fs_inode_data_blocks(fs, (struct ext2_inode *)t)) {
+	if (!islnk || has_data_blocks ) {
 		for (i = 0; i < EXT2_N_BLOCKS; i++)
 			t->i_block[i] = ext2fs_swab32(f->i_block[i]);
 	} else if (t != f) {

Re: [PATCH] Endianness bugs in e2fsck

[Kalpak Shah]

On Wed, 2007-06-20 at 11:09 -0400, Theodore Tso wrote:
> Hi Kalpak, 
> 
> 	In the future it would be really helpful if you split up your
> patches so that each different thing is done in separate patches.

Sure. Do you want me to split this patch too and resend?

> 
> 	Also, note there is a recent bug fix in this area, and the
> byte-swapping extended attributes.  The issues involved here are
> subtle; please see the discussion here:
> 
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=232663
> 
> So before your patches go in, we need to do a careful audit to make
> sure they don't interact properly with this patch which is already in
> e2fsprogs mainline.

This fix is already in our tree. We periodically keep syncing our tree
with the mainline changes. So I have tested my changes with this patch
present. 

Actually I had created test images for the expand-extra-isize patches
which failed on big-endian machines which brought these bugs to my
notice.

Thanks,
Kalpak.

Re: [PATCH] Endianness bugs in e2fsck

[Theodore Tso]

On Wed, Jun 20, 2007 at 03:03:08PM +0530, Kalpak Shah wrote:
> In ext2fs_swap_inode_full() if to and from inodes are not the same
> (which is the case when called from e2fsck_get_next_inode_full),
> then e2fsck cannot recognize any in-inode EAs since the un-swabbed
> i_extra_isize was being used. So corrected that to use swabbed
> values all the time.

Actually, this is wrong.  Remember, the swab functions are used to
swap *from* and *to* host byte order and on-disk order.  So yes, there
was a bug in that we were also using the f->i_extra_size, but using
t->i_extra_size is also going to be wrong in some cases.  That's what
the hostorder parameter to ext2fs_swap_inode_full() is for.

					- Ted

Re: [PATCH] Endianness bugs in e2fsck

[Theodore Tso]

Here's what I've committed into the e2fsprogs tree.

						- Ted

# HG changeset patch
# User tytso@mit.edu
# Date 1182556401 14400
# Node ID deeb424beb36d9fb1ff401aca7a5761a451436b8
# Parent  702632e66380e459f60b238570edd1e911dd46bc
Fix byte-swapping issues for the i_extra_size field

Thanks to Andreas Dilger and Kalpak Shah for spotting this problem.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff -r 702632e66380 -r deeb424beb36 lib/ext2fs/ChangeLog
--- a/lib/ext2fs/ChangeLog	Fri Jun 22 02:22:38 2007 -0400
+++ b/lib/ext2fs/ChangeLog	Fri Jun 22 19:53:21 2007 -0400
@@ -1,3 +1,8 @@ 2007-06-12  Theodore Tso  <tytso@mit.edu
+2007-06-22  Theodore Tso  <tytso@mit.edu>
+
+	* swapfs.c (ext2fs_swap_inode_full): Fix byte-swapping issues for
+		i_extra_size field.
+
 2007-06-12  Theodore Tso  <tytso@mit.edu>
 
 	* openfs.c (ext2fs_open2): We now set EXT2_FLAG_MASTER_SB_ONLY
diff -r 702632e66380 -r deeb424beb36 lib/ext2fs/swapfs.c
--- a/lib/ext2fs/swapfs.c	Fri Jun 22 02:22:38 2007 -0400
+++ b/lib/ext2fs/swapfs.c	Fri Jun 22 19:53:21 2007 -0400
@@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys 
 			    struct ext2_inode_large *f, int hostorder,
 			    int bufsize)
 {
-	unsigned i, has_data_blocks;
+	unsigned i, has_data_blocks, extra_isize;
 	int islnk = 0;
 	__u32 *eaf, *eat;
 
@@ -214,31 +214,35 @@ void ext2fs_swap_inode_full(ext2_filsys 
 	if (bufsize < (int) (sizeof(struct ext2_inode) + sizeof(__u16)))
 		return; /* no i_extra_isize field */
 
+	if (hostorder)
+		extra_isize = f->i_extra_isize;
 	t->i_extra_isize = ext2fs_swab16(f->i_extra_isize);
-	if (t->i_extra_isize > EXT2_INODE_SIZE(fs->super) -
+	if (!hostorder)
+		extra_isize = t->i_extra_isize;
+	if (extra_isize > EXT2_INODE_SIZE(fs->super) -
 				sizeof(struct ext2_inode)) {
 		/* this is error case: i_extra_size is too large */
 		return;
 	}
 
-	i = sizeof(struct ext2_inode) + t->i_extra_isize + sizeof(__u32);
+	i = sizeof(struct ext2_inode) + extra_isize + sizeof(__u32);
 	if (bufsize < (int) i)
 		return; /* no space for EA magic */
 
 	eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
-					f->i_extra_isize);
+					extra_isize);
 
 	if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
 		return; /* it seems no magic here */
 
 	eat = (__u32 *) (((char *) t) + sizeof(struct ext2_inode) +
-					f->i_extra_isize);
+					extra_isize);
 	*eat = ext2fs_swab32(*eaf);
 
 	/* convert EA(s) */
 	ext2fs_swap_ext_attr((char *) (eat + 1), (char *) (eaf + 1),
 			     bufsize - sizeof(struct ext2_inode) -
-			     t->i_extra_isize - sizeof(__u32), 0);
+			     extra_isize - sizeof(__u32), 0);
 
 }
 

Re: [PATCH] Endianness bugs in e2fsck

[Theodore Tso]

Also applied into the e2fsprogs tree.

					- Ted

# HG changeset patch
# User tytso@mit.edu
# Date 1182565963 14400
# Node ID 98d5fa14e7dedde4754cd42f4d2af2622c4ba3ee
# Parent  deeb424beb36d9fb1ff401aca7a5761a451436b8
Fix ext2fs_read_inode_full() so that the whole inode is byte-swapped

Signed-off-by: Kalpak Shah <kalpak@clusterfs.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff -r deeb424beb36 -r 98d5fa14e7de lib/ext2fs/ChangeLog
--- a/lib/ext2fs/ChangeLog	Fri Jun 22 19:53:21 2007 -0400
+++ b/lib/ext2fs/ChangeLog	Fri Jun 22 22:32:43 2007 -0400
@@ -1,4 +1,7 @@ 2007-06-22  Theodore Tso  <tytso@mit.edu
 2007-06-22  Theodore Tso  <tytso@mit.edu>
+
+	* inode.c (ext2fs_read_inode_full): Pass in bufsize so the full
+		inode is byte-swapped.
 
 	* swapfs.c (ext2fs_swap_inode_full): Fix byte-swapping issues for
 		i_extra_size field.
diff -r deeb424beb36 -r 98d5fa14e7de lib/ext2fs/inode.c
--- a/lib/ext2fs/inode.c	Fri Jun 22 19:53:21 2007 -0400
+++ b/lib/ext2fs/inode.c	Fri Jun 22 22:32:43 2007 -0400
@@ -586,7 +586,7 @@ errcode_t ext2fs_read_inode_full(ext2_fi
 	    (fs->flags & EXT2_FLAG_SWAP_BYTES_READ))
 		ext2fs_swap_inode_full(fs, (struct ext2_inode_large *) inode, 
 				       (struct ext2_inode_large *) inode, 
-				       0, length);
+				       0, bufsize);
 #endif
 
 	/* Update the inode cache */

Re: [PATCH] Endianness bugs in e2fsck

[Theodore Tso]

On Wed, Jun 20, 2007 at 03:03:08PM +0530, Kalpak Shah wrote:
> In ext2fs_swap_inode_full() only the first (GOOD_OLD_INODE_SIZE +
> i_extra_isize)bytes are copied into inode. The rest of the inode is
> not zeroed. So memset the inode to zero if swapfs is enabled. 

This was due to the bug where we weren't dealing with the i_extra_size
correctly, right?  ext2fs_swap_inode_full *should* be swapping the
extra fields and copying it into the inode.  If not, that's should be
the real bug, and adding the memset(inode, 0, bufset) doesn't seem to
be useful.

Am I missing something?

						- Ted

Re: [PATCH] Endianness bugs in e2fsck

[Kalpak Shah]

On Fri, 2007-06-22 at 20:36 -0400, Theodore Tso wrote:
> On Wed, Jun 20, 2007 at 03:03:08PM +0530, Kalpak Shah wrote:
> > In ext2fs_swap_inode_full() only the first (GOOD_OLD_INODE_SIZE +
> > i_extra_isize)bytes are copied into inode. The rest of the inode is
> > not zeroed. So memset the inode to zero if swapfs is enabled. 
> 
> This was due to the bug where we weren't dealing with the i_extra_size
> correctly, right?  ext2fs_swap_inode_full *should* be swapping the
> extra fields and copying it into the inode.  If not, that's should be
> the real bug, and adding the memset(inode, 0, bufset) doesn't seem to
> be useful.
> 
> Am I missing something?

Hi Ted,

In e2fsck_pass1(), a buffer is allocated for the scratch inode,
inode = (struct ext2_inode *) e2fsck_allocate_memory(ctx, inode_size,   
						"scratch inode");

Now on big-endian systems, while swapping, ext2fs_swap_inode_full()
swaps only 128+extra_isize bytes and the EAs if they are present. Now if
inode N has EAs, (and this is the inode in the "scratch inode") then
inode N+1 also carries seems to have them since the "scratch inode" was
never zeroed. In ext2fs_swap_inode_full(), this occurs:

        if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
                return; /* it seems no magic here */

So as I said only the first 128+extra_isize bytes are actually copied
into the *to* inode.

Thanks,
Kalpak.

> 
> 						- Ted

Re: [PATCH] Endianness bugs in e2fsck

[Eric Sandeen]

Kalpak Shah wrote:
...

> Index: e2fsprogs-1.39/lib/ext2fs/inode.c
> ===================================================================
> --- e2fsprogs-1.39.orig/lib/ext2fs/inode.c      2007-06-19 22:31:21.000000000 -0700
> +++ e2fsprogs-1.39/lib/ext2fs/inode.c   2007-06-20 01:06:18.017788976 -0700
> @@ -471,6 +471,7 @@ errcode_t ext2fs_get_next_inode_full(ext
>                 scan->bytes_left -= scan->inode_size - extra_bytes;
> 
>  #ifdef EXT2FS_ENABLE_SWAPFS
> +               memset(inode, 0, bufsize);
>                 if ((scan->fs->flags & EXT2_FLAG_SWAP_BYTES) ||
>                     (scan->fs->flags & EXT2_FLAG_SWAP_BYTES_READ))
>                         ext2fs_swap_inode_full(scan->fs,
> @@ -485,6 +486,7 @@ errcode_t ext2fs_get_next_inode_full(ext
>                 scan->scan_flags &= ~EXT2_SF_BAD_EXTRA_BYTES;
>         } else {
>  #ifdef EXT2FS_ENABLE_SWAPFS
> +               memset(inode, 0, bufsize);
>                 if ((scan->fs->flags & EXT2_FLAG_SWAP_BYTES) ||
>                     (scan->fs->flags & EXT2_FLAG_SWAP_BYTES_READ))
>                         ext2fs_swap_inode_full(scan->fs,


This is making "make check" fail for me on ppc64:
(git-bisect claims 1ed49d2c2ab7fdb02158d5feeb86288ece7eb17c is the first
bad commit...)  Any ideas?  Looking into it now.

--- ./f_clear_xattr/expect.1    2007-06-30 12:58:35.000000000 +0000
+++ f_clear_xattr.1.log 2007-07-17 19:48:17.000000000 +0000
@@ -1,10 +1,10 @@
 Pass 1: Checking inodes, blocks, and sizes
-Inode 14, i_blocks is 2, should be 0.  Fix? yes
-
 Inode 12, i_blocks is 4, should be 2.  Fix? yes

 Inode 13, i_blocks is 2, should be 0.  Fix? yes

+Inode 14, i_blocks is 2, should be 0.  Fix? yes
+
 Inode 15, i_blocks is 4, should be 2.  Fix? yes

 Pass 2: Checking directory structure
@@ -17,6 +17,9 @@
 i_file_acl for inode 14 (/symlink) is 22, should be zero.
 Clear? yes

+Symlink /symlink (inode #14) is invalid.
+Clear? yes
+
 i_file_acl for inode 15 (/long-symlink) is 23, should be zero.
 Clear? yes

@@ -34,5 +37,5 @@


 test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
-test_filesys: 15/16 files (0.0% non-contiguous), 22/100 blocks
+test_filesys: 14/16 files (0.0% non-contiguous), 22/100 blocks
 Exit status is 1
--- ./f_clear_xattr/expect.2    2007-06-30 12:58:35.000000000 +0000
+++ f_clear_xattr.2.log 2007-07-17 19:48:17.000000000 +0000
@@ -3,5 +3,5 @@
 Pass 3: Checking directory connectivity
 Pass 4: Checking reference counts
 Pass 5: Checking group summary information
-test_filesys: 15/16 files (0.0% non-contiguous), 22/100 blocks
+test_filesys: 14/16 files (0.0% non-contiguous), 22/100 blocks
 Exit status is 0

Re: [PATCH] Endianness bugs in e2fsck

[Eric Sandeen]

Eric Sandeen wrote:

> Kalpak Shah wrote:
> ...
>
>   
>> Index: e2fsprogs-1.39/lib/ext2fs/inode.c
>> ===================================================================
>> --- e2fsprogs-1.39.orig/lib/ext2fs/inode.c      2007-06-19 22:31:21.000000000 -0700
>> +++ e2fsprogs-1.39/lib/ext2fs/inode.c   2007-06-20 01:06:18.017788976 -0700
>> @@ -471,6 +471,7 @@ errcode_t ext2fs_get_next_inode_full(ext
>>                 scan->bytes_left -= scan->inode_size - extra_bytes;
>>
>>  #ifdef EXT2FS_ENABLE_SWAPFS
>> +               memset(inode, 0, bufsize);
>>                 if ((scan->fs->flags & EXT2_FLAG_SWAP_BYTES) ||
>>                     (scan->fs->flags & EXT2_FLAG_SWAP_BYTES_READ))
>>                         ext2fs_swap_inode_full(scan->fs,
>> @@ -485,6 +486,7 @@ errcode_t ext2fs_get_next_inode_full(ext
>>                 scan->scan_flags &= ~EXT2_SF_BAD_EXTRA_BYTES;
>>         } else {
>>  #ifdef EXT2FS_ENABLE_SWAPFS
>> +               memset(inode, 0, bufsize);
>>                 if ((scan->fs->flags & EXT2_FLAG_SWAP_BYTES) ||
>>                     (scan->fs->flags & EXT2_FLAG_SWAP_BYTES_READ))
>>                         ext2fs_swap_inode_full(scan->fs,
>>     
>
>
> This is making "make check" fail for me on ppc64:
> (git-bisect claims 1ed49d2c2ab7fdb02158d5feeb86288ece7eb17c is the first
> bad commit...)  Any ideas?  Looking into it now.
>
>   
Ok, I think this is the deal... ext2fs_get_next_inode_full zeros out 
"inode" which is the "t" (->to) inode that is sent to
ext2fs_swap_inode_full, with hostorder==0, which does this:

        if (hostorder)  /* "from" in hostorder */
                has_data_blocks = ext2fs_inode_data_blocks(fs,
                                           (struct ext2_inode *) f);
        t->i_blocks = ext2fs_swab32(f->i_blocks);
        if (!hostorder) /* "to" (will be) in hostorder, zeroed by caller */
                /* ext2fs_inode_data_blocks checks t->i_file_acl! */
                has_data_blocks = ext2fs_inode_data_blocks(fs,
                                           (struct ext2_inode *) t);
        t->i_flags = ext2fs_swab32(f->i_flags);
        t->i_file_acl = ext2fs_swab32(f->i_file_acl); /* finally set! */

so in the !hostorder case, ext2fs_inode_data_blocks checks t->i_file_acl,
which has been cleared by the caller, and isn't set until *after* it is
tested in ext2fs_get_next_inode_full.

So I'm a little lost in the order of things here, but it looks to me
like we need to set t->i_file_acl before we try to test 
ext2fs_inode_data_blocks for that "to" inode...

Seems fair?  At least it passes "make check" on both x86 and ppc
with the following change...

-Eric

---------

set t->i_file_acl before we test it in 
ext2fs_inode_data_blocks

Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Index: e2fsprogs-1.40.2/lib/ext2fs/swapfs.c
===================================================================
--- e2fsprogs-1.40.2.orig/lib/ext2fs/swapfs.c
+++ e2fsprogs-1.40.2/lib/ext2fs/swapfs.c
@@ -150,6 +150,7 @@ void ext2fs_swap_inode_full(ext2_filsys 
 	t->i_dtime = ext2fs_swab32(f->i_dtime);
 	t->i_gid = ext2fs_swab16(f->i_gid);
 	t->i_links_count = ext2fs_swab16(f->i_links_count);
+	t->i_file_acl = ext2fs_swab32(f->i_file_acl);
 	if (hostorder)
 		has_data_blocks = ext2fs_inode_data_blocks(fs, 
 					   (struct ext2_inode *) f);
@@ -158,7 +159,6 @@ void ext2fs_swap_inode_full(ext2_filsys 
 		has_data_blocks = ext2fs_inode_data_blocks(fs, 
 					   (struct ext2_inode *) t);
 	t->i_flags = ext2fs_swab32(f->i_flags);
-	t->i_file_acl = ext2fs_swab32(f->i_file_acl);
 	t->i_dir_acl = ext2fs_swab32(f->i_dir_acl);
 	if (!islnk || has_data_blocks ) {


 		for (i = 0; i < EXT2_N_BLOCKS; i++)

Re: [PATCH] Endianness bugs in e2fsck

[Kalpak Shah]

On Tue, 2007-07-17 at 20:40 -0500, Eric Sandeen wrote:
> Eric Sandeen wrote:
> ---------
> 
> set t->i_file_acl before we test it in 
> ext2fs_inode_data_blocks
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Index: e2fsprogs-1.40.2/lib/ext2fs/swapfs.c
> ===================================================================
> --- e2fsprogs-1.40.2.orig/lib/ext2fs/swapfs.c
> +++ e2fsprogs-1.40.2/lib/ext2fs/swapfs.c
> @@ -150,6 +150,7 @@ void ext2fs_swap_inode_full(ext2_filsys 
>  	t->i_dtime = ext2fs_swab32(f->i_dtime);
>  	t->i_gid = ext2fs_swab16(f->i_gid);
>  	t->i_links_count = ext2fs_swab16(f->i_links_count);
> +	t->i_file_acl = ext2fs_swab32(f->i_file_acl);
>  	if (hostorder)
>  		has_data_blocks = ext2fs_inode_data_blocks(fs, 
>  					   (struct ext2_inode *) f);
> @@ -158,7 +159,6 @@ void ext2fs_swap_inode_full(ext2_filsys 
>  		has_data_blocks = ext2fs_inode_data_blocks(fs, 
>  					   (struct ext2_inode *) t);
>  	t->i_flags = ext2fs_swab32(f->i_flags);
> -	t->i_file_acl = ext2fs_swab32(f->i_file_acl);
>  	t->i_dir_acl = ext2fs_swab32(f->i_dir_acl);
>  	if (!islnk || has_data_blocks ) {
> 
> 
>  		for (i = 0; i < EXT2_N_BLOCKS; i++)

This means that on big-endian machines, t->i_file_acl was uninitialized
when it was being accessed here. This certainly seems to be the correct
fix for this.

Thanks,
Kalpak.