[PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

[PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

[Eric Sandeen]

After the fix for resize2fs's inode mover losing in-inode
extended attributes, the regression test I wrote caught 
that the attrs were still getting lost on powerpc.

Looks like the problem is that ext2fs_swap_inode_full()
isn't paying attention to whether or not the EA magic is
in hostorder, so it's not recognized (and not swapped)
on BE machines.  Patch below seems to fix it.

Yay for regression tests.  ;)

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

Index: e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
===================================================================
--- e2fsprogs-1.40.7.orig/lib/ext2fs/swapfs.c
+++ e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
@@ -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, extra_isize;
+	unsigned i, has_data_blocks, extra_isize, attr_magic;
 	int islnk = 0;
 	__u32 *eaf, *eat;
 
@@ -231,13 +231,17 @@ void ext2fs_swap_inode_full(ext2_filsys 
 
 	eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
 					extra_isize);
-
-	if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
-		return; /* it seems no magic here */

Re: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

[Matthias Koenig]

Eric Sandeen <sandeen@redhat.com> writes:

> After the fix for resize2fs's inode mover losing in-inode
> extended attributes, the regression test I wrote caught 
> that the attrs were still getting lost on powerpc.
>
> Looks like the problem is that ext2fs_swap_inode_full()
> isn't paying attention to whether or not the EA magic is
> in hostorder, so it's not recognized (and not swapped)
> on BE machines.  Patch below seems to fix it.
>
> Yay for regression tests.  ;)
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> Index: e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
> ===================================================================
> --- e2fsprogs-1.40.7.orig/lib/ext2fs/swapfs.c
> +++ e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
> @@ -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, extra_isize;
> +	unsigned i, has_data_blocks, extra_isize, attr_magic;
>  	int islnk = 0;
>  	__u32 *eaf, *eat;
>  
> @@ -231,13 +231,17 @@ void ext2fs_swap_inode_full(ext2_filsys 
>  
>  	eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
>  					extra_isize);
> -
> -	if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
> -		return; /* it seems no magic here */
> -
>  	eat = (__u32 *) (((char *) t) + sizeof(struct ext2_inode) +
>  					extra_isize);
> +
> +	if (hostorder)
> +		attr_magic = *eaf;
>  	*eat = ext2fs_swab32(*eaf);
> +	if (!hostorder)
> +		attr_magic = *eat;
> +
> +	if (attr_magic != EXT2_EXT_ATTR_MAGIC)
> +		return; /* it seems no magic here */
>  
>  	/* convert EA(s) */
>  	ext2fs_swap_ext_attr((char *) (eat + 1), (char *) (eaf + 1),
>

This patch seems to be missing in the current e2fsprogs 1.40.8 release
(though announced in the release notes that this issue should be fixed
on BE machines).
Check r_inline_xattr still fails on ppc and s390 architectures without 
this patch. It builds fine with this patch applied.
Could you please check (also the git tag for the 1.40.8 release seems to
be missing).

Thanks,
Matthias

Re: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

[Theodore Tso]

On Fri, Mar 14, 2008 at 02:09:06PM +0100, Matthias Koenig wrote:
> (also the git tag for the 1.40.8 release seems to be missing).

Oops, forgot to do a "git push --tags" again.  Fixed.

> Eric Sandeen <sandeen@redhat.com> writes:
> 
> > After the fix for resize2fs's inode mover losing in-inode
> > extended attributes, the regression test I wrote caught 
> > that the attrs were still getting lost on powerpc.
> >
> > Looks like the problem is that ext2fs_swap_inode_full()
> > isn't paying attention to whether or not the EA magic is
> > in hostorder, so it's not recognized (and not swapped)
> > on BE machines.  Patch below seems to fix it.
>
> This patch seems to be missing in the current e2fsprogs 1.40.8 release
> (though announced in the release notes that this issue should be fixed
> on BE machines).
> Check r_inline_xattr still fails on ppc and s390 architectures without 
> this patch. It builds fine with this patch applied.

This patch is in the repo, which I thought superceded the one which
Eric sent.  (The authorship is wrong; and that was my fault for not
including '--author="Eric Sandeen <sandeen@redhat.com>"' on the commit
command-line.  I've already apologized to Eric privately; now I'm
apologizing publically.  :-)

But looking at it again, I believe I misunderstood, and the two
patches are attacking quite separate problems.  OK, i'll get this one
in as well in the next go-around.

						- Ted

commit edfd9b0a9fe7b90f56da981ca26d5233cc3749d6
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Sat Mar 8 20:20:40 2008 -0500

    resize2fs: Fix resizing filesystems with large inodes
    
    Use ext2fs_get_next_inode_full() in resize2fs and clean up large inode
    handling; previous attempt was not properly handling all cases, and
    was incorrectly setting i_extra_isize.  This caused some extended
    attributes to get removed or randomly assigned to other inodes as a
    result of the resize, which can be unfortunate on systems using
    SELinux.
    
    The previous commit didn't fix things completely on big-endian systems
    like PowerPC.
    
    Addresses-Red-Hat-Bugzilla: #434893
    
    Signed-off-by: Eric Sandeen <sandeen@redhat.com>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/resize/resize2fs.c b/resize/resize2fs.c
index dd26089..6dec3eb 100644
--- a/resize/resize2fs.c
+++ b/resize/resize2fs.c
@@ -1109,8 +1109,7 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 {
 	struct process_block_struct	pb;
 	ext2_ino_t		ino, new_inode;
-	struct ext2_inode 	inode, *buf = NULL;
-	struct ext2_inode_large	*large_inode;
+	struct ext2_inode 	*inode = NULL;
 	ext2_inode_scan 	scan = NULL;
 	errcode_t		retval;
 	int			group;
@@ -1154,12 +1153,12 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 	}
 	ext2fs_set_inode_callback(scan, progress_callback, (void *) rfs);
 	pb.rfs = rfs;
-	pb.inode = &inode;
+	pb.inode = inode;
 	pb.error = 0;
 	new_inode = EXT2_FIRST_INODE(rfs->new_fs->super);
 	inode_size = EXT2_INODE_SIZE(rfs->new_fs->super);
-	buf = malloc(inode_size);
-	if (!buf) {
+	inode = malloc(inode_size);
+	if (!inode) {
 		retval = ENOMEM;
 		goto errout;
 	}
@@ -1168,29 +1167,29 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 	 * elsewhere in the inode table
 	 */
 	while (1) {
-		retval = ext2fs_get_next_inode(scan, &ino, &inode);
+		retval = ext2fs_get_next_inode_full(scan, &ino, inode, inode_size);
 		if (retval) goto errout;
 		if (!ino)
 			break;
 
-		if (inode.i_links_count == 0 && ino != EXT2_RESIZE_INO)
+		if (inode->i_links_count == 0 && ino != EXT2_RESIZE_INO)
 			continue; /* inode not in use */
 
-		pb.is_dir = LINUX_S_ISDIR(inode.i_mode);
+		pb.is_dir = LINUX_S_ISDIR(inode->i_mode);
 		pb.changed = 0;
 
-		if (inode.i_file_acl && rfs->bmap) {
+		if (inode->i_file_acl && rfs->bmap) {
 			new_block = ext2fs_extent_translate(rfs->bmap, 
-							    inode.i_file_acl);
+							    inode->i_file_acl);
 			if (new_block) {
-				inode.i_file_acl = new_block;
-				retval = ext2fs_write_inode(rfs->old_fs, 
-							    ino, &inode);
+				inode->i_file_acl = new_block;
+				retval = ext2fs_write_inode_full(rfs->old_fs, 
+							    ino, inode, inode_size);
 				if (retval) goto errout;
 			}
 		}
 		
-		if (ext2fs_inode_has_valid_blocks(&inode) &&
+		if (ext2fs_inode_has_valid_blocks(inode) &&
 		    (rfs->bmap || pb.is_dir)) {
 			pb.ino = ino;
 			retval = ext2fs_block_iterate2(rfs->old_fs,
@@ -1221,23 +1220,19 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 			}
 		}
 		ext2fs_mark_inode_bitmap(rfs->new_fs->inode_map, new_inode);
-		memcpy(buf, &inode, sizeof(struct ext2_inode));
-		large_inode = (struct ext2_inode_large *)buf;
-		large_inode->i_extra_isize = sizeof(struct ext2_inode_large) -
-			EXT2_GOOD_OLD_INODE_SIZE;
 		if (pb.changed) {
 			/* Get the new version of the inode */
 			retval = ext2fs_read_inode_full(rfs->old_fs, ino,
-						buf, inode_size);
+						inode, inode_size);
 			if (retval) goto errout;
 		}
-		inode.i_ctime = time(0);
+		inode->i_ctime = time(0);
 		retval = ext2fs_write_inode_full(rfs->old_fs, new_inode,
-						buf, inode_size);
+						inode, inode_size);
 		if (retval) goto errout;
 
 		group = (new_inode-1) / EXT2_INODES_PER_GROUP(rfs->new_fs->super);
-		if (LINUX_S_ISDIR(inode.i_mode))
+		if (LINUX_S_ISDIR(inode->i_mode))
 			rfs->new_fs->group_desc[group].bg_used_dirs_count++;
 		
 #ifdef RESIZE2FS_DEBUG
@@ -1263,8 +1258,8 @@ errout:
 		ext2fs_close_inode_scan(scan);
 	if (block_buf)
 		ext2fs_free_mem(&block_buf);
-	if (buf)
-		free(buf);
+	if (inode)
+		free(inode);
 	return retval;
 }
 

Re: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

[Eric Sandeen]

Theodore Tso wrote:

> This patch is in the repo, which I thought superceded the one which
> Eric sent.  (The authorship is wrong; and that was my fault for not
> including '--author="Eric Sandeen <sandeen@redhat.com>"' on the commit
> command-line.  I've already apologized to Eric privately; now I'm
> apologizing publically.  :-)

And I've realized that I really should start using git to make it easier
on Ted.  :)

> But looking at it again, I believe I misunderstood, and the two
> patches are attacking quite separate problems.  OK, i'll get this one
> in as well in the next go-around.

Yep, there were 2 problems.

One was that inode_scan_and_fix() wasn't moving all of large inodes.

The other was that ext2fs_swap_inode_full was doing bad things for
in-inode attrs on big endian boxes, that was the patch in this thread
which isn't in the repo yet, but is in fact needed.

I suppose that the iterative nature of my fix for this problem didn't
help Ted keep things straight, sorry.

-Eric

Re: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

[Theodore Tso]

On Fri, Mar 14, 2008 at 09:13:03AM -0500, Eric Sandeen wrote:
> Yep, there were 2 problems.
> 
> One was that inode_scan_and_fix() wasn't moving all of large inodes.
> 
> The other was that ext2fs_swap_inode_full was doing bad things for
> in-inode attrs on big endian boxes, that was the patch in this thread
> which isn't in the repo yet, but is in fact needed.
> 
> I suppose that the iterative nature of my fix for this problem didn't
> help Ted keep things straight, sorry.

I actually *prefer* multiple patches, actually, but what works best
when there is a series of patches is to resend the whole patch stack
using git-format-patch or git-send-email --- or set up your own git
repository on the network (or use repo.or.cz) and send me a pull
request.  But that requires much more use of git.  :-)

In any case, I'll pull in the last change, and this time I promise
I'll be careful about remembering to use --author.  That *is*
something I should be doing, but I certainly appreciate people using
git's facilities to make my life easier, thanks.

      		    	    	 	 - Ted

Re: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

[Theodore Tso]

On Fri, Feb 29, 2008 at 04:10:21PM -0600, Eric Sandeen wrote:
> +
> +	if (hostorder)
> +		attr_magic = *eaf;
>  	*eat = ext2fs_swab32(*eaf);
> +	if (!hostorder)
> +		attr_magic = *eat;
> +
> +	if (attr_magic != EXT2_EXT_ATTR_MAGIC)
> +		return; /* it seems no magic here */

The problem with this is that if the magic isn't there, we still end
up writing 4 bytes into the destination.  So I think this is a better
patch....

						- Ted

>From 91a2fbe36ff5578c46f637687a56d7a0d169a807 Mon Sep 17 00:00:00 2001
From: Eric Sandeen <sandeen@redhat.com>
Date: Fri, 29 Feb 2008 16:10:21 -0600
Subject: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

After the fix for resize2fs's inode mover losing in-inode
extended attributes, the regression test I wrote caught
that the attrs were still getting lost on powerpc.

Looks like the problem is that ext2fs_swap_inode_full()
isn't paying attention to whether or not the EA magic is
in hostorder, so it's not recognized (and not swapped)
on BE machines.  Patch below seems to fix it.

Yay for regression tests.  ;)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 lib/ext2fs/swapfs.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/ext2fs/swapfs.c b/lib/ext2fs/swapfs.c
index 6576c59..e07e87c 100644
--- a/lib/ext2fs/swapfs.c
+++ b/lib/ext2fs/swapfs.c
@@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
 			    struct ext2_inode_large *f, int hostorder,
 			    int bufsize)
 {
-	unsigned i, has_data_blocks, extra_isize;
+	unsigned i, has_data_blocks, extra_isize, attr_magic;
 	int islnk = 0;
 	__u32 *eaf, *eat;
 
@@ -232,7 +232,11 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
 	eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
 					extra_isize);
 
-	if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
+	attr_magic = *eaf;
+	if (!hostorder)
+		attr_magic = ext2fs_swab32(attr_magic);
+
+	if (attr_magic != EXT2_EXT_ATTR_MAGIC)
 		return; /* it seems no magic here */
 
 	eat = (__u32 *) (((char *) t) + sizeof(struct ext2_inode) +
-- 
1.5.4.1.144.gdfee-dirty

Re: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

[Eric Sandeen]

Theodore Tso wrote:
> On Fri, Feb 29, 2008 at 04:10:21PM -0600, Eric Sandeen wrote:
>> +
>> +	if (hostorder)
>> +		attr_magic = *eaf;
>>  	*eat = ext2fs_swab32(*eaf);
>> +	if (!hostorder)
>> +		attr_magic = *eat;
>> +
>> +	if (attr_magic != EXT2_EXT_ATTR_MAGIC)
>> +		return; /* it seems no magic here */
> 
> The problem with this is that if the magic isn't there, we still end
> up writing 4 bytes into the destination.  So I think this is a better
> patch....
> 

Seems good to me.

Thanks,
-Eric