ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

PATCH In review Tiger Yang 8 participants 13 messages May 12, 2008 – May 14, 2008

Subsystems (2)

VFS ext4 file system

lore

Share & tools

lore

Tiger Yang

author

2008-05-12 03:24 UTC #0 lore raw

PATCH 1/1 ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

Hi,
I met a bug when I try to replace a xattr entry in ibody with a big size value. But in ibody there has no space for the new value. So it should set new xattr entry in block and remove the old xattr entry in ibody.

Best regards,
tiger

This fix the uninitialized bs when we try to replace a xattr entry in ibody with the new value which require more than free space.

Signed-off-by Tiger Yang <[email protected]>

fs/ext3/xattr.c +5 -0

index a6ea4d6..e1af9bd 100644
@@ -1000,6 +1000,11 @@ ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 			i.value = NULL;
 			error = ext3_xattr_block_set(handle, inode, &i, &bs);
 		} else if (error == -ENOSPC) {
+			if (EXT3_I(inode)->i_file_acl && !bs.s.base) {
+				error = ext3_xattr_block_find(inode, &i, &bs);
+				if (error)
+					goto cleanup;
+			}
 			error = ext3_xattr_block_set(handle, inode, &i, &bs);
 			if (error)
 				goto cleanup;

fs/ext4/xattr.c +5 -0

index d796213..182a7a2 100644
@@ -1011,6 +1011,11 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 			i.value = NULL;
 			error = ext4_xattr_block_set(handle, inode, &i, &bs);
 		} else if (error == -ENOSPC) {
+			if (EXT4_I(inode)->i_file_acl && !bs.s.base) {
+				error = ext4_xattr_block_find(inode, &i, &bs);
+				if (error)
+					goto cleanup;
+			}
 			error = ext4_xattr_block_set(handle, inode, &i, &bs);
 			if (error)
 				goto cleanup;

Andrew Morton

2008-05-13 00:02 UTC #1 lore raw

Quoted 19 lines · On Mon, 12 May 2008 11:24:40 +0800 Tiger Yang <[email protected]> wrote:

I met a bug when I try to replace a xattr entry in ibody with a big size value. But in ibody there has no space for the new value. So it should set new xattr entry in block and remove the old xattr entry in ibody.
Best regards,
tiger
[xattr.patch text/x-patch (1.3KB)]
This fix the uninitialized bs when we try to replace a xattr entry in ibody with the new value which require more than free space.
Signed-off-by Tiger Yang <[email protected]>
fs/ext3/xattr.c +0 -0
index a6ea4d6..e1af9bd 100644
@@ -1000,6 +1000,11 @@ ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 			i.value = NULL;

fs/ext3/xattr.c ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, · line 1010

Quoted 10 lines

fs/ext3/xattr.c +5 -0

 			error = ext3_xattr_block_set(handle, inode, &i, &bs);
 		} else if (error == -ENOSPC) {
+			if (EXT3_I(inode)->i_file_acl && !bs.s.base) {
+				error = ext3_xattr_block_find(inode, &i, &bs);
+				if (error)
+					goto cleanup;
+			}
 			error = ext3_xattr_block_set(handle, inode, &i, &bs);
 			if (error)
 				goto cleanup;

That sounds fairly bad.

What are the consequences of this bug, when someone hits it?

It appears that we should backport this fix into 2.6.25.x (and perhaps earlier). What do you think?

Tiger Yang

author

2008-05-13 02:31 UTC #2 lore raw

Hi, Andrew

This situation only happens we format ext3/4 with inode size more than 128 and we have put xattr entries both in ibody and block.
The consequences about this bug is we will lost the xattr block which pointed by i_file_acl with all xattr entires in it. We will alloc a new xattr block and put that large value entry in it. The old xattr block will become orphan block.

Best regards,
tiger

Quoted 41 lines · Andrew Morton

Quoted 18 lines · On Mon, 12 May 2008 11:24:40 +0800 Tiger Yang <[email protected]> wrote:
I met a bug when I try to replace a xattr entry in ibody with a big size value. But in ibody there has no space for the new value. So it should set new xattr entry in block and remove the old xattr entry in ibody.
Best regards,
tiger
[xattr.patch text/x-patch (1.3KB)]
This fix the uninitialized bs when we try to replace a xattr entry in ibody with the new value which require more than free space.
Signed-off-by Tiger Yang <[email protected]>
fs/ext3/xattr.c +0 -0
index a6ea4d6..e1af9bd 100644
@@ -1000,6 +1000,11 @@ ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 			i.value = NULL;
fs/ext3/xattr.c ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, · line 1010
Quoted 10 lines
fs/ext3/xattr.c +5 -0
 			error = ext3_xattr_block_set(handle, inode, &i, &bs);
 		} else if (error == -ENOSPC) {
+			if (EXT3_I(inode)->i_file_acl && !bs.s.base) {
+				error = ext3_xattr_block_find(inode, &i, &bs);
+				if (error)
+					goto cleanup;
+			}
 			error = ext3_xattr_block_set(handle, inode, &i, &bs);
 			if (error)
 				goto cleanup;
That sounds fairly bad.
What are the consequences of this bug, when someone hits it?
It appears that we should backport this fix into 2.6.25.x (and perhaps earlier). What do you think?

Andreas Dilger

2008-05-13 20:00 UTC #3 lore raw

Quoted 6 lines · Tiger Yang

This situation only happens we format ext3/4 with inode size more than 128 and we have put xattr entries both in ibody and block.
The consequences about this bug is we will lost the xattr block which pointed by i_file_acl with all xattr entires in it. We will alloc a new xattr block and put that large value entry in it. The old xattr block will become orphan block.

Tiger, thanks for finding this bug, and the patch (which fixes the problem in our testing).

Signed-off-by Andreas Dilger <[email protected]>

Quoted 38 lines

Quoted 37 lines · Andrew Morton
Quoted 20 lines · On Mon, 12 May 2008 11:24:40 +0800 Tiger Yang <[email protected]> wrote:
I met a bug when I try to replace a xattr entry in ibody with a big size value. But in ibody there has no space for the new value. So it should set new xattr entry in block and remove the old xattr entry in ibody.
Best regards,
tiger
[xattr.patch text/x-patch (1.3KB)]
This fix the uninitialized bs when we try to replace a xattr entry in ibody with the new value which require more than free space.
Signed-off-by Tiger Yang <[email protected]>
fs/ext3/xattr.c +0 -0
index a6ea4d6..e1af9bd 100644
@@ -1000,6 +1000,11 @@ ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 			i.value = NULL;
fs/ext3/xattr.c ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, · line 1010
Quoted 10 lines
fs/ext3/xattr.c +5 -0
 			error = ext3_xattr_block_set(handle, inode, &i, &bs);
 		} else if (error == -ENOSPC) {
+			if (EXT3_I(inode)->i_file_acl && !bs.s.base) {
+				error = ext3_xattr_block_find(inode, &i, &bs);
+				if (error)
+					goto cleanup;
+			}
 			error = ext3_xattr_block_set(handle, inode, &i, &bs);
 			if (error)
 				goto cleanup;
That sounds fairly bad.

Quoted 1 line

Quoted 1 line
What are the consequences of this bug, when someone hits it?

The EAs in the external block (except the one being added) are lost, and some blocks (or shared EA block references) are leaked. In most cases this is not fatal, but for Lustre I developed a test case where this causes the file data to be lost (because the file layout is stored in the external block if it is too large to store in the inode).

Quoted 2 lines

Quoted 2 lines
It appears that we should backport this fix into 2.6.25.x (and perhaps earlier). What do you think?

Code inspection shows this bug goes back to when the fast EA-in-inode support was added to the vanilla kernel, at least 2.6.12 (when Git history begins).

Sadly, the bug was NOT in the original CFS EA-in-inode patches that we made for kernels 2.6.5 (SLES 9) and 2.6.9 (RHEL 4) (and still use today) that were in 2.6.11-rc1-mm1, but were added during the later rewrite of this code.

I suspect the reasons this bug hasn't been reported even when large inodes are enabled (which is the default for newer e2fsprogs) are:
- it uncommon to have multiple EAs on a file (usually SELinux is the only common one and it is relatively small)
- one of the EAs must already be too large to fit in the inode
- increasing the size of any EA after it is created is rare

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Andreas Gruenbacher

2008-05-14 10:56 UTC #4 lore raw

In reply to Tiger Yang

Quoted 8 lines · On Tuesday 13 May 2008 04:31:04 Tiger Yang wrote:

Hi, Andrew
This situation only happens we format ext3/4 with inode size more than 128 and we have put xattr entries both in ibody and block. The consequences about this bug is we will lost the xattr block which pointed by i_file_acl with all xattr entires in it. We will alloc a new xattr block and put that large value entry in it. The old xattr block will become orphan block.

The patch looks good, and it obviously fixes the described problem. Thanks!

Signed-off-by Andreas Gruenbacher <[email protected]>

Could it please be added to -stable?

Andreas

Greg KH

2008-05-14 16:00 UTC #5 lore raw

Quoted 10 lines · Andreas Gruenbacher

Quoted 8 lines · On Tuesday 13 May 2008 04:31:04 Tiger Yang wrote:
Hi, Andrew
This situation only happens we format ext3/4 with inode size more than 128 and we have put xattr entries both in ibody and block. The consequences about this bug is we will lost the xattr block which pointed by i_file_acl with all xattr entires in it. We will alloc a new xattr block and put that large value entry in it. The old xattr block will become orphan block.

Quoted 6 lines

The patch looks good, and it obviously fixes the described problem. Thanks!
Signed-off-by Andreas Gruenbacher <[email protected]>
Could it please be added to -stable?

Can someone actually _send_ the patch to [email protected]? I haven't seen it yet :)

And is it in Linus's tree? We need to wait until it is there before we can add it to -stable.

thanks,

greg k-h

Andrew Morton

2008-05-14 17:28 UTC #6 lore raw

Quoted 16 lines · On Wed, 14 May 2008 09:00:36 -0700 Greg KH <[email protected]> wrote:

Quoted 15 lines · Andreas Gruenbacher
Quoted 8 lines · On Tuesday 13 May 2008 04:31:04 Tiger Yang wrote:
Hi, Andrew
This situation only happens we format ext3/4 with inode size more than 128 and we have put xattr entries both in ibody and block. The consequences about this bug is we will lost the xattr block which pointed by i_file_acl with all xattr entires in it. We will alloc a new xattr block and put that large value entry in it. The old xattr block will become orphan block.
The patch looks good, and it obviously fixes the described problem. Thanks!
Signed-off-by Andreas Gruenbacher <[email protected]>

Quoted 7 lines

Quoted 1 line
Could it please be added to -stable?
Can someone actually _send_ the patch to [email protected]? I haven't seen it yet :)
And is it in Linus's tree? We need to wait until it is there before we can add it to -stable.

It's in -mm and I just added the "Cc:stable" tag to it.

Theodore Tso

maintainer

2008-05-14 22:30 UTC #7 lore raw

Quoted 2 lines · Andrew Morton

It's in -mm and I just added the "Cc:stable" tag to it.

This patch has both ext3 and ext4 changes in it, so Andrew, I'm assuming that you'll push it directly to Linus in the near future? That's why I didn't suck it into the ext4 tree....

- Ted

Andrew Morton

2008-05-14 22:54 UTC #8 lore raw

Quoted 6 lines · On Wed, 14 May 2008 18:30:03 -0400 Theodore Tso <[email protected]> wrote:

Quoted 2 lines · Andrew Morton
It's in -mm and I just added the "Cc:stable" tag to it.
This patch has both ext3 and ext4 changes in it, so Andrew, I'm assuming that you'll push it directly to Linus in the near future?

Yes, today.

Quoted 1 line

That's why I didn't suck it into the ext4 tree....

Yup. Usually I have to split these things up but this time I decided to leave it as a single patch. Not sure why, really.

Eric Sandeen

2008-05-13 00:18 UTC #9 lore raw

In reply to Tiger Yang

Quoted 8 lines · Tiger Yang

Hi,
I met a bug when I try to replace a xattr entry in ibody with a big size value. But in ibody there has no space for the new value. So it should set new xattr entry in block and remove the old xattr entry in ibody.
Best regards,
tiger

Tiger, do you have a testcase handy to demonstrate this?

Is the new, large out-of-inode xattr unique so that it does not match any existing attribute block, I assume?

Thanks,

-Eric

Tiger Yang

author

2008-05-13 07:48 UTC #10 lore raw

Hi, Eric,

I don't have tesecase about this bug. I did the test manually. I use khexedit to confirm the attributes whether in inody or block.
The problem about this bug is we want to replace an existing attribute in ibody with big size value which larger than free space in ibody.
Because we didn't do block_find(), so the struct bs have not been initialized. Then when we try to set attribute in block by block_set(), we find bs->base is empty, we need alloc a new block for attributes. The old block pointed by i_file_acl will lost with attributes in it.

Best regards,
tiger

Quoted 19 lines · Eric Sandeen

Quoted 7 lines · Tiger Yang
Hi,
I met a bug when I try to replace a xattr entry in ibody with a big size value. But in ibody there has no space for the new value. So it should set new xattr entry in block and remove the old xattr entry in ibody.
Best regards,
tiger
Tiger, do you have a testcase handy to demonstrate this?
Is the new, large out-of-inode xattr unique so that it does not match any existing attribute block, I assume?
Thanks,
-Eric

Eric Sandeen

2008-05-13 12:48 UTC #11 lore raw

Quoted 10 lines · Tiger Yang

Hi, Eric,
I don't have tesecase about this bug. I did the test manually. I use khexedit to confirm the attributes whether in inody or block. The problem about this bug is we want to replace an existing attribute in ibody with big size value which larger than free space in ibody. Because we didn't do block_find(), so the struct bs have not been initialized. Then when we try to set attribute in block by block_set(), we find bs->base is empty, we need alloc a new block for attributes. The old block pointed by i_file_acl will lost with attributes in it.

Thanks, I'll go for a reproducer. We use xattrs a lot for selinux in Red Hat and Fedora, so a little surprised I haven't seen this bug... or maybe it explains some bugs I haven't yet figured out ... :)

Thanks,
-Eric

Kalpak Shah

2008-05-13 08:48 UTC #12 lore raw

In reply to Eric Sandeen

Hi Eric,

Quoted 11 lines · Eric Sandeen

Quoted 8 lines · Tiger Yang
Hi,
I met a bug when I try to replace a xattr entry in ibody with a big size value. But in ibody there has no space for the new value. So it should set new xattr entry in block and remove the old xattr entry in ibody.
Best regards,
tiger
Tiger, do you have a testcase handy to demonstrate this?

Attached is a simple script to reproduce the problem.

Quoted 3 lines

Is the new, large out-of-inode xattr unique so that it does not match any existing attribute block, I assume?

I don't quite understand what you mean but the problem is that in ext3_xattr_set_handle(), the EA being replaced is found in the inode-body (by function ext3_xattr_ibody_find) and hence ext3_xattr_block_find() is not called initially. So in this test-case when we have to delete an EA from the inode and add it into the external block, bs turns out to be uninitialized and therefore a new EA block gets allocated instead of the existing one being used.

Thanks,
Kalpak

Quoted 8 lines

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