e2fsprogs: Offsets of EAs in inode need not be sorted

PATCH Applied Kalpak Shah 2 participants 6 messages Apr 19, 2007 – May 8, 2007

lore

Share & tools

lore

Kalpak Shah

author

2007-04-19 12:03 UTC #0 lore raw

PATCH 1/1 e2fsprogs: Offsets of EAs in inode need not be sorted

Hi,

This patch removes a code snippet from check_ea_in_inode() in pass1 which checks if the EA values in the inode are sorted or not. The comments in fs/ext*/xattr.c state that the EA values in the external EA block are sorted but those in the inode need not be sorted. I have also attached a test image which has unsorted EAs in the inodes. The current e2fsck wrongly clears the EAs in the inode.

Signed-off-by Kalpak Shah <[email protected]>

e2fsck/pass1.c +1 ~~-12~~

Index: e2fsprogs-1.40/e2fsck/pass1.c
===================================================================
@@ -246,7 +246,7 @@ static void check_ea_in_inode(e2fsck_t c
 	struct ext2_inode_large *inode;
 	struct ext2_ext_attr_entry *entry;
 	char *start, *end;
-	unsigned int storage_size, remain, offs;
+	unsigned int storage_size, remain;
 	int problem = 0;
 
 	inode = (struct ext2_inode_large *) pctx->inode;

@@ -261,7 +261,6 @@ static void check_ea_in_inode(e2fsck_t c
 
 	/* take finish entry 0UL into account */
 	remain = storage_size - sizeof(__u32); 
-	offs = end - start;
 
 	while (!EXT2_EXT_IS_LAST_ENTRY(entry)) {

@@ -285,15 +284,6 @@ static void check_ea_in_inode(e2fsck_t c
 			goto fix;
 		}
 
-		/* check value placement */
-		if (entry->e_value_offs + 
-		    EXT2_XATTR_SIZE(entry->e_value_size) != offs) {
-			printf("(entry->e_value_offs + entry->e_value_size: %d, offs: %d)\n", entry->e_value_offs + entry->e_value_size, offs);
-			pctx->num = entry->e_value_offs;
-			problem = PR_1_ATTR_VALUE_OFFSET;
-			goto fix;
-		}
-	
 		/* e_value_block must be 0 in inode's ea */
 		if (entry->e_value_block != 0) {
 			pctx->num = entry->e_value_block;

@@ -309,7 +299,6 @@ static void check_ea_in_inode(e2fsck_t c
 		}
 
 		remain -= entry->e_value_size;
-		offs -= EXT2_XATTR_SIZE(entry->e_value_size);
 
 		entry = EXT2_EXT_ATTR_NEXT(entry);
 	}

Thanks,
Kalpak Shah.
<[email protected]>

Theodore Tso

2007-04-20 12:38 UTC #1 lore raw

Quoted 8 lines · Kalpak Shah

Hi,
This patch removes a code snippet from check_ea_in_inode() in pass1 which checks if the EA values in the inode are sorted or not. The comments in fs/ext*/xattr.c state that the EA values in the external EA block are sorted but those in the inode need not be sorted. I have also attached a test image which has unsorted EAs in the inodes. The current e2fsck wrongly clears the EAs in the inode.

Hmm, have you been able to create test images that have unsorted EA's in inodes using a standard ext3 kernel implementat? If so, then this is a patch which we should push to the distro's since it could cause data loss, and cause serious malfunctions, especially for people who have SELinux enabled and are using large inodes for the EA-in-inode feature....

- Ted

Kalpak Shah

author

2007-04-20 13:04 UTC #2 lore raw

Quoted 10 lines · Theodore Tso

Quoted 8 lines · Kalpak Shah
Hi,
This patch removes a code snippet from check_ea_in_inode() in pass1 which checks if the EA values in the inode are sorted or not. The comments in fs/ext*/xattr.c state that the EA values in the external EA block are sorted but those in the inode need not be sorted. I have also attached a test image which has unsorted EAs in the inodes. The current e2fsck wrongly clears the EAs in the inode.

Quoted 6 lines

Hmm, have you been able to create test images that have unsorted EA's in inodes using a standard ext3 kernel implementat? If so, then this is a patch which we should push to the distro's since it could cause data loss, and cause serious malfunctions, especially for people who have SELinux enabled and are using large inodes for the EA-in-inode feature....

Hi,

I saw this problem when I was running a script which created a random number of EAs for a file of random sizes. If you mount the image I have given, all the EAs are displayed and they can be used/modified/deleted without any problems so its not a bug in ext3 code.

Thanks,
Kalpak.

Quoted 6 lines

- Ted
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Theodore Tso

2007-04-20 14:10 UTC #3 lore raw

Quoted 5 lines · Kalpak Shah

I saw this problem when I was running a script which created a random number of EAs for a file of random sizes. If you mount the image I have given, all the EAs are displayed and they can be used/modified/deleted without any problems so its not a bug in ext3 code.

Not a bug in the ext3 code, but if a RHEL5 or SLES 10 or Debian etch user uses EA-in-Inode, with SELinux enabled, and then uses the e2fsck shipped with their distro, some of the unsorted EA's would get deleted.... right?

If that's correct, then that's a pretty nasty data corruption problem (that with SELinux enabled could cause the system to become completely non-functional, further contributing to SELinux's bad reputation...) and we'll need to push your patch to the various distro's as a relatively high priority bug fix.

- Ted

Kalpak Shah

author

2007-04-20 19:15 UTC #4 lore raw

Quoted 11 lines · Theodore Tso

Quoted 5 lines · Kalpak Shah
I saw this problem when I was running a script which created a random number of EAs for a file of random sizes. If you mount the image I have given, all the EAs are displayed and they can be used/modified/deleted without any problems so its not a bug in ext3 code.
Not a bug in the ext3 code, but if a RHEL5 or SLES 10 or Debian etch user uses EA-in-Inode, with SELinux enabled, and then uses the e2fsck shipped with their distro, some of the unsorted EA's would get deleted.... right?

Yes thats what I meant, it is certainly not a bug in ext3. And actually the effects are worse, _all_ the EAs in the inode get deleted. I think not many people have been using > 128 byte inodes and hence this problem might not have cropped up until now.

Thanks,
Kalpak Shah.
<[email protected]>

Quoted 8 lines

If that's correct, then that's a pretty nasty data corruption problem (that with SELinux enabled could cause the system to become completely non-functional, further contributing to SELinux's bad reputation...) and we'll need to push your patch to the various distro's as a relatively high priority bug fix.
- Ted

Theodore Tso

Applied

2007-05-08 05:09 UTC #5 lore raw

In reply to Kalpak Shah

Quoted 6 lines · Kalpak Shah

This patch removes a code snippet from check_ea_in_inode() in pass1 which checks if the EA values in the inode are sorted or not. The comments in fs/ext*/xattr.c state that the EA values in the external EA block are sorted but those in the inode need not be sorted. I have also attached a test image which has unsorted EAs in the inodes. The current e2fsck wrongly clears the EAs in the inode.

Applied.

- Ted