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]>
Index: e2fsprogs-1.40/e2fsck/pass1.c
===================================================================
--- e2fsprogs-1.40.orig/e2fsck/pass1.c
+++ 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]>
On Thu, Apr 19, 2007 at 05:35:36PM +0530, Kalpak Shah wrote:
> 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
On Fri, 2007-04-20 at 08:38 -0400, Theodore Tso wrote:
> On Thu, Apr 19, 2007 at 05:35:36PM +0530, Kalpak Shah wrote:
> > 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....
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.
>
> - 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
On Fri, Apr 20, 2007 at 06:35:41PM +0530, Kalpak Shah wrote:
>
> 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
On Fri, 2007-04-20 at 10:10 -0400, Theodore Tso wrote:
> On Fri, Apr 20, 2007 at 06:35:41PM +0530, Kalpak Shah wrote:
> >
> > 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]>
>
> 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
On Thu, Apr 19, 2007 at 05:35:36PM +0530, Kalpak Shah wrote:
> 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