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
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]>
>
> 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?
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
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 <[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]>
>>
>> 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?
>
>
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
>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 <[email protected]>
> Andrew Morton wrote:
>> 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]>
>>>
>>> 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.
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
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 <[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
On Wed, 14 May 2008 09:00:36 -0700 Greg KH <[email protected]> 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 <[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.
It's in -mm and I just added the "Cc:stable" tag to it.
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
On Wed, 14 May 2008 18:30:03 -0400
Theodore Tso <[email protected]> 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.