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

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

[Tiger Yang]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]

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

[-- Attachment #2: xattr.patch --]
[-- Type: text/x-patch, Size: 1323 bytes --]

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 <tiger.yang@oracle.com>

diff --git a/fs/ext3/xattr.c b/fs/ext3/xattr.c
index a6ea4d6..e1af9bd 100644
--- a/fs/ext3/xattr.c
+++ b/fs/ext3/xattr.c
@@ -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;
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index d796213..182a7a2 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -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;

Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Andrew Morton]

On Mon, 12 May 2008 11:24:40 +0800
Tiger Yang <tiger.yang@oracle.com> 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 <tiger.yang@oracle.com>
> 
> diff --git a/fs/ext3/xattr.c b/fs/ext3/xattr.c
> index a6ea4d6..e1af9bd 100644
> --- a/fs/ext3/xattr.c
> +++ b/fs/ext3/xattr.c
> @@ -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;

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?

Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Eric Sandeen]

Tiger Yang wrote:
> 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

Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Tiger Yang]

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

Andrew Morton wrote:
> On Mon, 12 May 2008 11:24:40 +0800
> Tiger Yang <tiger.yang@oracle.com> 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 <tiger.yang@oracle.com>
>>
>> diff --git a/fs/ext3/xattr.c b/fs/ext3/xattr.c
>> index a6ea4d6..e1af9bd 100644
>> --- a/fs/ext3/xattr.c
>> +++ b/fs/ext3/xattr.c
>> @@ -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;
>>     
>
> 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?
>
>   

Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

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

Best regards,
tiger

Eric Sandeen wrote:
> Tiger Yang wrote:
>   
>> 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
>   

Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Kalpak Shah]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

Hi Eric,

On Mon, 2008-05-12 at 19:18 -0500, Eric Sandeen wrote:
> Tiger Yang wrote:
> > 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.

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

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

[-- Attachment #2: reproducer.sh --]
[-- Type: application/x-shellscript, Size: 1018 bytes --]

Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Eric Sandeen]

Tiger Yang wrote:
> 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

Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Andreas Dilger]

On May 13, 2008  10:31 +0800, Tiger Yang wrote:
> 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 <adilger@sun.com>

> Andrew Morton wrote:
>> On Mon, 12 May 2008 11:24:40 +0800
>> Tiger Yang <tiger.yang@oracle.com> 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 <tiger.yang@oracle.com>
>>>
>>> diff --git a/fs/ext3/xattr.c b/fs/ext3/xattr.c
>>> index a6ea4d6..e1af9bd 100644
>>> --- a/fs/ext3/xattr.c
>>> +++ b/fs/ext3/xattr.c
>>> @@ -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;
>>>     
>>
>> That sounds fairly bad.
>>
>> 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).

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

Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Andreas Gruenbacher]

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 <agruen@suse.de>


Could it please be added to -stable?

Andreas

Re: [stable] [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Greg KH]

On Wed, May 14, 2008 at 12:56:42PM +0200, Andreas Gruenbacher wrote:
> 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 <agruen@suse.de>
> 
> 
> Could it please be added to -stable?

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

Re: [stable] [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Andrew Morton]

On Wed, 14 May 2008 09:00:36 -0700 Greg KH <greg@kroah.com> wrote:

> On Wed, May 14, 2008 at 12:56:42PM +0200, Andreas Gruenbacher wrote:
> > 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 <agruen@suse.de>
> > 
> > 
> > Could it please be added to -stable?
> 
> 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.

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

Re: [stable] [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Theodore Tso]

On Wed, May 14, 2008 at 10:28:09AM -0700, Andrew Morton wrote:
> 
> 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

Re: [stable] [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

[Andrew Morton]

On Wed, 14 May 2008 18:30:03 -0400
Theodore Tso <tytso@mit.edu> wrote:

> On Wed, May 14, 2008 at 10:28:09AM -0700, Andrew Morton wrote:
> > 
> > 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.

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