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
--

Hi Eric,



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,

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

--

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
--

On Mon, 12 May 2008 11:24:40 +0800

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?

--

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

--

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

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>


Could it please be added to -stable?

Andreas
--

Can someone actually _send_ the patch to stable@kernel.org?  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
--

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
--

On Wed, 14 May 2008 18:30:03 -0400


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.
--

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


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

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.

--